feat(#142): restore sessions & trades from mnemonic handshake - #204
feat(#142): restore sessions & trades from mnemonic handshake#204codaMW wants to merge 7 commits into
Conversation
… UNRESOLVED Compiling: _recover fix, restore_session builder, PendingRequestKind::Restore, DaemonReply::Restored, take_matching_restore, restore_session() send-half. BLOCKED: key-choice (trade-key vs master-key) + await path. Send-half currently subscribes on the wrong key until resolved. Refs MostroP2P#142
…a Dart API restore_session returns mostro-core RestoreSessionInfo which FRB can't bridge; it's called internally by import_from_mnemonic, not from Dart. Refs MostroP2P#142
…t daemon Verified send→receive: app publishes RestoreSession (kind-14, identity in Seal, trade key as sender), daemon looks up by master key and replies RestoreData to the trade key, dispatcher resolves the waiter. 0-order case confirmed. Refs MostroP2P#142
Create identity A, create a Sell order under A (daemon-confirmed), then
restore_session() from A: daemon looks up by master key and returns the order
made under a different trade index — confirms master-key correlation on the
wire. restore_orders=[{order_id, trade_index:1, status:pending}]. Refs MostroP2P#142
…_mine)
restore_session plants order_id -> trade_index mappings for each recovered
order; ingest_order_event marks is_mine=true when an order_id is known via
restore. Compiles clean; existing suite green.
INCOMPLETE — recovered orders don't yet reach My Trades: list_trades() reads
TradeInfo records from the DB, but restore yields only {order_id, trade_index,
status} and the order-book is_mine flag is a separate store. Needs a design
decision on TradeInfo synthesis vs. richer RestoreData. Refs MostroP2P#142
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughMnemonic recovery now invokes a restore-session API, which exchanges a restore request and reply with Mostro, stores recovered trade mappings, and identifies restored orders as owned. Privacy-mode identities reject recovery. ChangesRestore Session Recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust/src/api/orders.rs (1)
1773-1802: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo log for a late (post-timeout)
RestoreDatareply.Every other correlated arm (
NewOrder, take, add-invoice) logs the case wherepending.txisNone(detached after timeout) for observability. Here, iftake_matching_restorereturns a record whosetxis alreadyNone, theif let Some(tx) = pending.txsilently does nothing — no log at all, unlike the sibling arms.🩹 Suggested fix
if let Some(pending) = take_matching_restore(trade_pubkey_hex) { if let Some(tx) = pending.tx { let _ = tx.send(DaemonReply::Restored(info.clone())); crate::api::logging::blog_info("gift-wrap", format!( "RestoreData: notified waiting restore_session ({} orders, {} disputes)", info.restore_orders.len(), info.restore_disputes.len() )); + } else { + crate::api::logging::blog_info("gift-wrap", format!( + "RestoreData: late reply for timed-out restore on trade={}", + &trade_pubkey_hex[..8] + )); } } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/src/api/orders.rs` around lines 1773 - 1802, Update the RestoreSession handling around take_matching_restore so a matching pending record with pending.tx set to None emits an informational log for the late post-timeout RestoreData reply. Preserve the existing notification path when a sender is present and the existing no-waiting-caller log when no pending record is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/api/identity.rs`:
- Around line 167-178: Update import_from_mnemonic to read the authoritative
privacy mode via reputation::get_privacy_mode() before calling
load_identity_from_mnemonic, pass that value instead of false, and use the same
value for the recover guard so PrivacyModeRecoveryUnavailable is enforced
consistently.
In `@rust/src/mostro/actions.rs`:
- Around line 462-479: Correct the `restore_session` doc comment to state that
the request is sent from the identity key while the daemon routes its reply to
the trade key derived from the rumor sender. Keep the payload and NIP-59 Gift
Wrap descriptions unchanged, and align the outer documentation with the inline
comments and existing correlation logic.
---
Nitpick comments:
In `@rust/src/api/orders.rs`:
- Around line 1773-1802: Update the RestoreSession handling around
take_matching_restore so a matching pending record with pending.tx set to None
emits an informational log for the late post-timeout RestoreData reply. Preserve
the existing notification path when a sender is present and the existing
no-waiting-caller log when no pending record is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0a999d0-821a-44bf-a16d-9ec44897711d
📒 Files selected for processing (3)
rust/src/api/identity.rsrust/src/api/orders.rsrust/src/mostro/actions.rs
- import_from_mnemonic: source privacy_mode from reputation::get_privacy_mode() instead of hardcoded false. The guard previously read info.privacy_mode (always false), so PrivacyModeRecoveryUnavailable could never fire while restore_session() read the real flag — a Full-Privacy user could have triggered restore. Now sourced once and used for both load and guard. - actions::restore_session: fix doc comment — the daemon addresses its reply to the trade key (event.sender), not the identity key. Doc now matches the inline comment and take_matching_restore correlation logic. - dispatcher RestoreData arm: log the post-timeout late reply (pending.tx = None) for parity with the NewOrder/take/add-invoice arms. Refs MostroP2P#142
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Strict review — changes requested
The RestoreSession handshake is a useful foundation, but the recovery flow described by #142 and the repository contract is not end-to-end functional yet.
Blocking findings
-
The recovery path is unreachable from the app. The only production mnemonic import path still calls
importFromMnemonic(..., recover: false)(lib/core/services/identity_service.dart:74-76), and the Account import UI routes through that method. No production caller passesrecover: true, so reinstall/import never sends this RestoreSession request. -
The restore result does not reconstruct a usable trade/session.
restore_session()only writesorder_id -> trade_indexmappings (rust/src/api/orders.rs:3400-3411). It does not createTradeInforows orSessionobjects, restore disputes/chat, or fetch enough order data to do so.list_trades()reads only the trades DB, therefore recovered trades remain absent from My Trades and session-dependent actions/chat still fail. This leaves steps 4-6 ofspecs/004-mostro-p2p-client/contracts/identity.md:36-42unimplemented. -
The derivation counter is not resynchronized. Import starts with index 0 and the restore request consumes index 1, but processing
RestoreDatanever raisesIdentityInfo.trade_key_indexto the maximum recovered index. The next order can therefore derive an already-used key and be rejected withInvalidTradeIndex. This also prevents the long-lived Kind-14 subscription from covering recovered keys becausebuild_trade_key_map()only derives through the stale counter (rust/src/api/orders.rs:2520-2537). -
The 10-second waiter discards valid slow restores. The daemon's restore worker allows up to one hour, and #142 explicitly calls for visible progress because replay can take time. Here the client detaches after 10 seconds (
rust/src/api/orders.rs:3390-3397); whenRestoreDataarrives later, the dispatcher removes the pending restore but drops the payload becausepending.txisNone(rust/src/api/orders.rs:1780-1796). The new log makes the loss observable but does not apply any mappings or durable recovery state. -
The
is_minemapping is not applied retroactively.ingest_order_eventconsults restore mappings only while ingesting an event (rust/src/api/orders.rs:2620-2626). If the order book replayed Kind 38383 before RestoreData planted those mappings—which is the normal import-after-startup ordering—the already-cached orders remainis_mine = false; restore does not re-fetch or reprocess them.
Required verification
Please add a deterministic reinstall-style test that clears local state, imports the mnemonic with recovery enabled, receives restored indices greater than 1, and proves: the counter advances, recovered trades/sessions are persisted and visible, subscriptions cover their keys, and a delayed RestoreData result is still applied. The two new E2E tests are ignored, hardcode a local daemon, and restore in the same process, so they do not exercise these failure modes.
Local verification on this exact head: cargo test --lib passed (107 passed, 8 ignored). CI is green, but the new recovery E2E tests are among the ignored tests.
|
Thanks for the thorough review these are all fair, and several are exactly the gaps I want to get right rather than rush. Agreed on the framing: this PR wires and proves the RestoreSession handshake (send/correlate/reply verified E2E against a regtest daemon), but the full recovery flow isn't end-to-end yet. Specifically: (1) unreachable path correct; the production recover: false is intentional for now (I reverted my test-only flip). The UI wiring comes once reconstruction lands. (2) no TradeInfo/Session reconstruction this is the open design question in the PR description. RestoredOrdersInfo gives {order_id, trade_index, status} only, so building TradeInfo/Session needs either 38383-hydration + synthesis (deriving role/step from status) or richer RestoreData. Would like a direction call here before building it. (4) 10s waiter vs. 1h daemon window agreed; restore needs the progress/async handling #142 calls for, not a fixed short timeout. The late-reply log I added only makes the loss visible; it needs to actually apply the result. I'll add a deterministic reinstall-style test as you describe. Before I build (2)/(4), I'd value a steer on the intended reconstruction + progress model @grunch, this connects to the design question I raised. Happy to hop on a call or take it in the issue. |
|
Thanks — this matches my reading, and I agree that (2) and (4) need a contract decision before more client code is built. My recommended direction is: ReconstructionDo not synthesize a session from
Applying the final snapshot should be one idempotent recovery operation: validate the whole payload; upsert trades/sessions/disputes; persist every Progress / late repliesTreat restore as a durable background operation, not a foreground RPC with a longer timeout. Give it an explicit Until the daemon exposes real progress, the UI should show indeterminate progress plus truthful local phases ( Suggested implementation order: protocol/core + daemon snapshot contract first, then transactional/idempotent client reconstruction, then UI wiring, then the deterministic clean-install test. That test should include a recovered index greater than 1, a delayed response beyond 10 seconds, an order already present in the cache, restart during recovery, and verification that the next new trade uses a fresh index. I would keep this PR non-mergeable for issue #142 until that path works end to end. If the goal is to merge only the handshake foundation first, I suggest splitting/re-scoping it explicitly as infrastructure and removing any partial recovery side effects that could be mistaken for completed restore behavior. |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Great work!
It works very well. There are two improvements we could make. When the user enters the words and goes to the screen where the user must confirm them, the user can see four choices. Each time the user makes a mistake, the user is told that the word is incorrect. The user could fail three times and guess the word by process of elimination. The user can repeat this until have "backed up" the seed without actually doing so.
I propose that if the user fails a second time, on each attempt the user should be redirected to the screen displaying the 12 words with a message saying the user have failed a second time and it should back up the words and start the verification process again.
In the account screen, secret words section remains the same; this needs to be modified. I've attached images of how it should look before and after the backup.
before backup
after backup
|
Thanks @grunch, this is exactly the direction I needed, and the idempotent-reconstruction + durable-background-restore contract makes sense. I'll build it in the order you laid out (core/daemon snapshot contract -> transactional client reconstruction -> UI -> clean-install test). One scoping question before I start: given the snapshot contract spans mostro-core + daemon + client + UI, would you prefer I re-scope this PR as the RestoreSession handshake infrastructure (strip the partial reconstruction side-effects so nothing looks like completed restore, merge the foundation), then build the full recovery as a follow-up series? Or keep this PR open until #142 works end-to-end? I lean toward landing the handshake as infrastructure first so it's a clean reviewable unit, but happy either way. Separately, thanks for the backup-verification feedback. I'll address the fail-twice -> re-show-words flow and the Account "Secret Words" section as part of this (or a small separate PR, your call). |
|
Closing this one — not because the work was wrong, but because the review surfaced that the The handshake you proved against regtest is preserved as its own issue and is unblocked — please take #215 first, it's essentially this PR minus the reconstruction side effects, and your implementation here is the reference. Also unblocked and independent of the contract discussion: #217 (the The contract itself is #216 (mostro-core + mostrod); everything heavy (#218, #220, #221, #222) waits on it. Separately, #223 is the backup-verification UX (fail-twice -> re-show words, Account "Secret Words") — split out so it isn't stuck behind any of this. Feel free to take it in parallel. Full breakdown and the agreed design constraints are in #142. Thanks for the E2E work here — it's what made the contract gap concrete. |
Adds widget tests covering the requested cases: first wrong pick stays on step 2 with feedback; second wrong pick on the same word returns to step 1 with the SnackBar; after restart one wrong pick is tolerated (reset works); all correct confirms the backup. Review fixes: - per-word miss budget: _failCount resets on each correct pick (was per-round); field doc updated, comment no longer overclaims 'brute-force' - _backToWords() owns the counter reset (belt-and-braces with _startVerification) - _failCount++ wrapped in setState - stale MostroP2P#204 references corrected to MostroP2P#223 Adds @VisibleForTesting debugWords + debugCorrectWordForActiveSlot seams so the verification flow is widget-testable without the Rust bridge. Refs MostroP2P#223
2 participants
What this does
Implements the client side of restore sessions & trades from mnemonic (#142): after a reinstall, entering the 12-word phrase restores not just the identity but the user's active trades.
Before this work, importing a mnemonic restored only the identity keys the
recoverflag was silently ignored (_recover), so noRestoreSessionwas ever sent and trades were lost. This PR wires the fullRestoreSessionhandshake, proves it end-to-end against a regtest daemon, and lays the reconstruction foundation.How it works
Restore correlates differently from every other action, and this was the core of the work:
RestoreSessionfrom a freshly-derived trade key. The gift-wrap Seal is signed with the identity key, so on the daemon sideevent.identity= master key andevent.sender= trade key.restore_session.rs:master_key = event.identity) and replies withPayload::RestoreDataaddressed to the trade key (event.sender).RestoreSessioncarries norequest_id, so the reply is correlated purely by pubkey a dedicatedtake_matching_restore(pubkey)that skips the nonce gate the order path uses.Action::RestoreSessionarm that decodesRestoreData, resolves the waiting caller, and returns theRestoreSessionInfo.This mirrors
create_order's send/await pattern wholesale, minus the order payload and request_id.Changes
identity.rsimport_from_mnemonicnow honorsrecover(was a_recoverstub) and callsrestore_session().mostro/actions.rsrestore_sessionbuilder (Message::new_restore(None)), wraps with identity key in the Seal + derived trade key as sender.api/orders.rsPendingRequestKind::Restore+DaemonReply::Restored(RestoreSessionInfo)take_matching_restore(pubkey)pubkey-only correlation (no request_id)restore_session()derive trade key, send, subscribe, register pending, await replyAction::RestoreSessiondispatcher arm#[flutter_rust_bridge::frb(ignore)]onrestore_session()(internal returns a mostro-core type FRB can't bridge; called byimport_from_mnemonic, never from Dart)restore_sessionplantsorder_id -> trade_indexmappings for recovered orders;ingest_order_eventmarksis_mine = truewhen anorder_idis known via restore.Testing E2E proven against a regtest daemon
Two integration tests (
#[ignore], require a live regtest stack onws://localhost:7000), run individually:restore_session_roundtripbare handshake round-trips: app publishesRestoreSession(kind-14) -> daemon receives and repliesRestoreDatato the trade key -> dispatcher resolves the waiter. ReturnsOkwith an empty result for a fresh identity (0 trades).trade_then_restore_recovers_orderreal recovery: create a Sell order under identity A (daemon-confirmed), then restore as A. The daemon looks up by master key and returns the order created under a different trade index, proving master-key correlation on the wire:Existing suite:
107 passed; 0 failed(cargo test --lib).cargo buildclean.Shaking this out end-to-end also surfaced two regtest-env issues unrelated to the code my daemon was on
transport = "gift-wrap"(v1, kind-1059) instead ofnip44(v2, kind-14), and thetransportkey was under[nostr]instead of[mostro]insettings.toml. Both fixed locally; noting in case it helps others' v2 setup.Open design question (the one thing I need a call on)
Recovered orders don't yet appear in My Trades.
RestoredOrdersInfocarries only{order_id, trade_index, status}, butlist_trades()reads fullTradeInforecords from the DB which need the full order data plusrole/current_step/counterparty. The order-bookis_mineflag (fed by Kind-38383) is a separate store from that DB.So the final reconstruction step needs a direction:
TradeInfo(derivingrole/current_stepfromstatus), then writes it to the trades DB.RestoreData/RestoredOrdersInfocarries richer order data so the client can build a completeTradeInfodirectly (cross-repo change to mostro-core + daemon).I've committed the mapping +
is_mine-by-order-id foundation; I'll build theTradeInfosynthesis once the approach is confirmed, to avoid guessing the intended data model.Notes for review
#[ignore]d (won't run in CI) and one hardcodes my regtest daemon pubkey. Happy to make them env-configurable or drop them from the PR whichever you prefer.Refs #142
Summary by CodeRabbit
New Features
Bug Fixes