refactor(sso): serve inter-host requests through typed handlers - #628
refactor(sso): serve inter-host requests through typed handlers#628pgherveou wants to merge 8 commits into
Conversation
|
@lore-bot-app review |
|
Reading the diff and checking what the record says. Back in a few minutes. |
There was a problem hiding this comment.
TL;DR
Replaces the hand-matched SSO request/response plumbing with SsoWire/SsoResponse derives and an #[sso_service]-generated dispatcher, and binds resource consent to the request's signing session. 0 blocking, 2 minor concerns, 3 open questions.
Summary
v1::RemoteMessage becomes the single wire spec: the derive pairs each *Request variant with its response, emits classify()/name()/responding_to(), and the trait macro generates dispatch() so a missing handler is a compile error. The pairing host sends through one generic call(request) whose reply matcher fails fast on a mistyped reply instead of waiting for the timeout. Message types are renamed to TrUAPI method stems (wire indices unchanged, host-papp 0.8.11 fixtures still pinned). On the signing host, serve_resource_allocation re-checks require_current_session around every allocation step so consent given before an account switch or reactivation cannot release keys, with three tests covering swap, same-account reactivation, and disconnect.
What the record says
- This is PR #628. Lore's summary notes the battery run passed but "some legacy account failures persist due to design constraints rather than the refactor". The diff does not say which.
- PR #618 (merged 2026-09-07) introduced binding signing and paired SSO work to the wallet generation via
require_current_session. This PR extends the same mechanism to resource consent, consistent with that decision. - No recorded discussion covers transcript request-name stability, the placement of server-only derives in
truapi-macros, or the item-failure detail in allocation transcripts. Searches on all three returned nothing relevant.
Concerns
- Allocation transcripts lose the failure cause.
rust/crates/truapi-server/src/runtime/signing_host/sso_service.rs:238now onlywarn!s the per-item error and maps it toNotAvailable. The oldallocation_result_with_failuresappended those reasons to the transcriptreason;resource_allocation_outcomeatrust/crates/truapi-server/src/host_logic/sso/messages.rs:547has no input for them. The CLI renders thatreasonas the activity detail (rust/crates/truapi-host-cli/src/terminal_ui.rs:2695) and its test atterminal_ui.rs:4253expects text like "rejected: timed out waiting for Bulletin authorization". After this PR a user sees only "Requested resource is not available". The CLI test still passes because it is fed a synthetic event, so nothing catches the drop. - README overstates the naming.
README.md:112says transcripts use "the service method names shared with the typed client".sign,resource_allocation, andproduct_subtreehave no same-named TrUAPI method (rust/crates/truapi/src/api/signing.rs:230and:267aresign_raw/sign_payload). Either tighten the sentence or rename those three stems.
Questions for the author
Cargo.lockchanged (272 chars, not shown to me). Is that onlytruapi-macrosadded totruapi-server's dependency list, with no version bumps?truapi-macrosis a dependency of the canonicaltruapicrate (rust/crates/truapi/Cargo.toml:14), and the new derives hardcodecrate::host_logic::sso::wirepaths (rust/crates/truapi-macros/src/sso.rs:17). Was atruapi-server-local proc-macro crate considered, so the protocol crate does not carry runtime-only macro code?- Which legacy-account battery failures remain after this change, and are they tracked anywhere?
🤖 Reviewed by Lore (Parity knowledge base) · 34 agent turns · 356.0s · knowledge as of 2026-09-08
|
@lore-bot-app review |
|
Reading the diff and checking what the record says. Back in a few minutes. |
There was a problem hiding this comment.
Since last review
- Resolved: allocation transcripts keep the per-item failure cause again.
serve_resource_allocationcollects them andSsoReply::finishappends them to the outcome reason (rust/crates/truapi-server/src/runtime/sso_service.rs:95), covered byallocation_failure_details_reach_the_response_transcript. - Resolved: the README sentence now says "typed SSO client", which is accurate.
- Answered:
Cargo.lockonly addstruapi-macrostotruapi-server's dependency list. No version bumps. - Still open: macro placement in
truapi-macroswith hardcodedcrate::runtime::...paths, and which legacy-account battery failures remain. The record has nothing on either. - New in this revision:
#[sso_service]is also applied to the signing-host impl and wraps bodies intoSsoReply;SsoRequestContextcarries the call context and the session resolved before the handler runs; the five*AuthorityRequeststructs are gone andProductAuthoritynow takes the SSO message request types directly; three new waiter tests pin batch-order semantics.
TL;DR
Typed SSO RPC layer: derives replace hand-matched pairing, dispatch, correlation and transcript classification, and consent is bound to the request's session. 0 blocking, 5 minor.
Summary
v1::RemoteMessage stays the hand-written wire spec (indices unchanged, host-papp 0.8.11 fixtures still pinned). SsoWire generates classification and correlation helpers from it, SsoResponse reads each response's Result payload, and #[sso_service] on the trait derives the request-to-response pairing and an exhaustive dispatch() from method signatures. On the impl it wraps ordinary Result bodies into SsoReply, so handlers keep ?. The pairing host sends everything through one generic call, whose matcher fails immediately on a reply of the wrong kind instead of waiting for the timeout. Resource consent re-validates the captured session around every allocation step, with tests for account swap, same-account reactivation, and disconnect.
What the record says
- PR #618 (merged) bound signing, SSO and allowance work to the wallet generation via
require_current_session. This PR extends that to SSO resource consent. The same-account reactivation test only works becauselocal_session_validation_idmixes inactivation_generation(signing_host.rs:1144). - PR #584 (merged) made peer-initiated disconnect cleanup local and sent exactly one
Disconnected. The new batch semantics fit: a waiter that matches a reply earlier in the same batch does not see a laterDisconnected, but the separate disconnect monitor still does. - Lore's summary of PR #628 records 1,285 passing tests and host-papp/Android fixture coverage. It no longer mentions the legacy-account battery failures the earlier revision reported.
- Nothing in the record covers transcript request-name stability, the
actionspan field, or where server-only derives should live. Owner by expertise: pgherveou.
Concerns
- Pairing-host tracing loses signing granularity.
rust/crates/truapi-server/src/runtime/pairing_host/sso_channel.rs:148setsaction = R::NAME, sosign_payload,sign_raw, and legacy-accountsign_payloadall log assign, and the sixAuthorityRequestKindlabels are gone. No in-repo consumer reads the field, so this is a debuggability regression, not a break. - The pure layer now depends on the runtime layer. The generated
v1::RemoteMessage::name()calls<Payload as SsoRequest>::NAME, and those impls are emitted by#[sso_service]inruntime/sso_service.rs.rust/crates/truapi-server/README.md:259still sayshost_logicstays pure, and line 261 says pairing is "derived from the wire enum" when it is derived from the trait signatures. Fix the text, or emitNAMEfrom the wire derive. - Top-level README carries macro internals.
README.md:111to:122add eight sentences aboutSsoReply, dispatch and correlation to the wallet-host paragraph, duplicating the crate README and CLAUDE.md. The consent-binding sentence belongs there; the rest does not. - Misattached doc comment.
rust/crates/truapi-server/src/test_support.rs:265now describessso_host_and_responder_sessionswith the sentence that belonged tosso_session_info(:316), which is left undocumented. - Stale design doc.
docs/rfcs/0024-personhood-as-product.md:332still namesRingVrfProofRequest,RingVrfAliasRequestandRingVrfSignRequest. CLAUDE.md asks for design docs to follow renames.
Questions for the author
truapi-macrosis a dependency of the canonicaltruapicrate, yetsso.rshardcodescrate::host_logic::ssoandcrate::runtime::sso_servicepaths. Was atruapi-server-local proc-macro crate considered?host_logicispubattruapi-server/src/lib.rs:30, so the removed builders,SsoRemoteResponse, and the renamed message types are public API changes. Does anything outside this workspace import them?- Which legacy-account battery failures remain, and are they tracked?
🤖 Reviewed by Lore (Parity knowledge base) · 29 agent turns · 435.8s · knowledge as of 2026-09-08 · re-review
465d0e1 to
fc8c4ce
Compare
069bea2 to
2f515d1
Compare
13a2742 to
261a771
Compare
|
@lore-bot-app review |
|
Reading the diff and checking what the record says. Back in a few minutes. |
|
Lore couldn't complete this review: Nothing was changed. Try |
261a771 to
281eae4
Compare
Use typed SSO handlers with generated dispatch and shared canonical payloads, removing duplicate request/response structs. Resource consent stays bound to the signing session.
SSO encoding remains compatible. Product–host signing now uses
OptionBool; clients and hosts must update together.Stack 2/2; depends on #651. Further encoding simplification: #654.
Validation: Rust and TypeScript tests, workspace Clippy, WASM build, and Swift binding generation passed.