feat(sdk): preserve proposal-embedded notes on the switch-guardian path (#417) - #441
Conversation
…th (#417) Pending proposals do not survive a guardian switch: the new GUARDIAN is registered with bare account state, so the notes embedded in pending consume-notes proposals — the one recovery source device-loss recover_notes can no longer reach once the old GUARDIAN is left behind — must be imported while the old GUARDIAN still serves them. Both SDKs now run the proposal-import slice of the recovery flow on the online switch path, before the switch transaction executes (summary-binding re-verification only reproduces before the account state advances) and before anything switch-related lands on the old GUARDIAN. The step is best-effort and bounded by a 30s timeout: failures, per-note losses, and timeouts are logged, never raised — an unreachable or hung old GUARDIAN must not block the switch. The transport drain and public backfill are deliberately skipped on this path (the switch runs against an intact local store, and both the note transport and the node are configured independently of the GUARDIAN), and the offline switch flow deliberately skips the import (it exists to avoid contacting the GUARDIAN); both decisions are documented in place. The manual repoint seams (set_guardian_endpoint / setGuardianClient) document how to request the same preservation by hand, and the TS switch slice is a named exhaustive preset (GUARDIAN_SWITCH_RECOVERY_OPTIONS) mirroring NoteRecoveryOptions::for_guardian_switch so a future strategy cannot silently join the switch path. Also hoists the TS stale-nonce check above the per-proposal binding re-execution, matching the Rust listing's order.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Comment |
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
Automatic preservation on the online SwitchGuardian execution path is the right default: this is the last point where pending proposals on the old Guardian remain available. The public/manual methods are still useful for direct client repointing and optional preparation before offline execution.
I found the following issues to address before merging:
1. Missing continuity coverage
Issue #417 requires proving that a note received before the switch remains consumable afterward. The Rust test only exercises the recovery-options slice against an unreachable Guardian, while the TypeScript test mocks importNotesFromProposals. Neither verifies post-switch consumability.
Recommendation: add one continuity test on at least one SDK (ideally both): a distinct v2 consume_notes proposal → switch → note still in the store and consumable. Do not reuse the shared AQID summary.
2. TypeScript timeout leaves recovery running
Rust’s timeout cancels the recovery future and returns Option<NoteRecoveryReport>. TypeScript returns void and uses Promise.race, so the recovery promise stays active after timeout. That leftover still uses the same Multisig instance — later this.guardian reads can hit the new client after setGuardianClient(), and import/store access can interleave with submitAt() on the shared WASM client.
Recommendation: preservePreSwitchProposalNotes should return Promise<NoteRecoveryReport | undefined> (mirror Rust). Abort in-flight listing/import on timeout instead of flow.catch(() => {}). Do not start submitAt until that work is dead.
3. Aliased TypeScript fixture
The pending delta reuses the switch proposal’s AQID summary, so it inherits cached switch metadata. A real importNotesFromProposals invocation would skip it rather than exercise a consume_notes proposal.
Recommendation: use a distinct summary with genuine consume_notes metadata and note bytes, and assert the import received proposalType: 'consume_notes'.
4. Missing Rust execute-path wiring test
There is no Rust test proving that execute_proposal() invokes preservation specifically for SwitchGuardian. Removing that call would currently go unnoticed.
Recommendation: add an execute_proposal(SwitchGuardian) wiring test that the listing/import ran, matching the TypeScript order test (import → execute → register).
5. Public API documentation
Please document preserve_pre_switch_proposal_notes, NoteRecoveryOptions::for_guardian_switch(), preservePreSwitchProposalNotes, and GUARDIAN_SWITCH_RECOVERY_OPTIONS in both package READMEs and docs/MULTISIG_SDK.md. The docs should clarify:
- automatic for online switch execution
- explicit before direct client repointing
- deliberately skipped during offline execution
No critical issue found, but the timeout lifecycle and missing end-to-end continuity coverage should be resolved before merge.
…e and docs (#417) Review feedback on PR #441, all five points: 1+4 (continuity + Rust wiring): guardian-client's in-process mock GUARDIAN gRPC server is now reachable from downstream test suites via a 'testing' feature (with explicit tokio/net deps), extended with a cross-endpoint call log, push capture, and re-armable persistent responses behind a shared one-shot -> persistent -> default precedence helper. Two new Rust integration tests drive the real pipeline over it: a genuine pending consume-notes v2 proposal (summary produced by the same abort-execution binding verification replays, from the note-less state every verifier can reproduce) is listed from mock guardian A, binding-verified, and its embedded private note imported into a store that never held it, then shown proof-backed after the repoint to a mock guardian B that serves nothing; and execute_proposal on a real offline-created switch proposal provably runs the import listing before the switch-delta push (asserted from the mock's own call log; execution runs through proving and is allowed to fail only on the known MockRpcApi submission limit). 2 (TS timeout lifecycle): preservePreSwitchProposalNotes now returns NoteRecoveryReport | undefined, mirroring Rust. Cancellation is typed (RecoveryCancelledError, with deliberately non-transient wording so the RPC retry classifier can never retry it) and owned by runNoteRecovery: the orchestrator checks the token before each step, records one non-retryable problem at the interruption point instead of misattributing it as a listing failure cascade, and stops. Checkpoints cover the import prologue, each retry attempt, each candidate, and the trailing reclassification; on timeout the switch waits a bounded settle grace for the single uninterruptible in-flight operation so it cannot overlap the switch transaction on the shared WASM client. Docs now state exactly that. 3 (fixture): the headline TS test's pending proposal no longer aliases the cached switch proposal — a distinct summary flows through the real metadata parse -> binding re-execution -> import pipeline and the import is asserted to receive proposalType 'consume_notes'. The cache-merge aliasing branch the old fixture happened to cover gets its own dedicated test (listing nonce wins, cached metadata inherited, stale-nonce filter applied to the listing nonce). 5 (docs): docs/MULTISIG_SDK.md and both package READMEs document the switch-path slice: automatic on online switch execution, explicit before manual repointing, deliberately skipped offline (now also warned at run time on the offline execute path). Also keys the switch-only side effects in execute_proposal on the SwitchGuardian type itself rather than on 'needs no guardian ack'.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
LGTM
Let's revisit inline comments. They seem big and repetitive. Keeping them short and moving rationale to separate /docs document to document recovery mechanism makes sense to me.
…set type, shorter comments (#417) - The wrapper (preserve_pre_switch_proposal_notes / preservePreSwitchProposalNotes) is the one public way to request switch preservation: NoteRecoveryOptions::for_guardian_switch() is pub(crate) and GUARDIAN_SWITCH_RECOVERY_OPTIONS is no longer exported from the package index. The preset path bypasses the wrapper's timeout, cancellation, and warning contract, so advertising it created an unsafe second path. All docs and README examples now show only the wrapper. - The TS preset uses 'satisfies Required<Omit<RecoverNotesOptions, 'fromBlock' | 'toBlock'>>', so adding a recovery strategy fails to compile until the switch path explicitly decides about it — Pick could never flag a key it was not told about. - Method docs and inline comments trimmed to the contract plus a pointer; the full rationale (strategy selection, pre-execution binding constraint, timeout/cancellation/settle-grace semantics, stale-nonce boundary) lives in the 'Preserving Notes Across a Guardian Switch' sections of docs/MULTISIG_SDK.md.
Closes #417 (last active sub-issue of #357; #418 remains deferred).
What
Pending proposals do not survive a guardian switch — the new GUARDIAN is registered with bare account state — so the notes embedded in pending consume-notes proposals are the one recovery source a later device-loss
recover_notes/recoverNotescan no longer reach once the old GUARDIAN is left behind. Both SDKs now run the proposal-embedded note import (#415 primitive) on the online switch path, against the old GUARDIAN, before the switch transaction executes.Why only the proposal import
Per the issue's "document what is skipped" criterion: the transport drain and public backfill are deliberately skipped on the switch path. A switch executes against an intact local store (chain cursor, transport cursor, and note records all persist), and both the note transport and the node are configured independently of the GUARDIAN — a switch loses nothing those primitives rescan. They remain device-loss recovery tools and work unchanged against the new GUARDIAN. The offline switch flow also skips the import (it exists to avoid contacting the GUARDIAN); its docs now say so and point at the manual call.
Design notes
failed/invalidoutcomes are warned (this is the last moment the notes are reachable, so "retryable" cannot help), and the whole flow runs under a 30s timeout (PRE_SWITCH_IMPORT_TIMEOUT/PRE_SWITCH_IMPORT_TIMEOUT_MS) — neither guardian client applies request deadlines, so a hung old GUARDIAN must not stall the switch.NoteRecoveryOptions::for_guardian_switch(), exportedGUARDIAN_SWITCH_RECOVERY_OPTIONS), so adding a recovery strategy forces an explicit decision about the switch path.preserve_pre_switch_proposal_notes/preservePreSwitchProposalNotesare public, and the manual repoint seams (set_guardian_endpoint,setGuardianClient) document calling them first.Known boundaries