feat(sdk): preserve proposal-embedded notes on the switch-guardian path (#417) - #441
feat(sdk): preserve proposal-embedded notes on the switch-guardian path (#417)#441haseebrabbani wants to merge 1 commit into
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: Pro Plus 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.
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