Fix keystone flow#554
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b5960c8. Configure here.
n13
left a comment
There was a problem hiding this comment.
Summary
Reviewed the Keystone send-flow changes: transaction details (amount, recipient, checkphrase) on the sign screen, a Riverpod keystoneSignCacheProvider that caches the unsigned payload + UR QR parts keyed by account/recipient/amount so back-navigation doesn't rebuild the QR, cache resets at all send entry points and on logout, and the scan screen now receives blockHeight from the cached payload's blockNumber.
What's good: the WYSIWYS details on the sign screen, keying the cache so a hit always matches the on-screen amount/recipient, passing unsignedData.payloadToSign.blockNumber forward (keeps the pending-tx block height consistent with what was actually signed), and caching before the mounted check in _prepare so a mid-flight back-out still populates the cache. All four send entry points I could find (home Send, multisig propose, pay intent, shared-address sheet) do clear the cache. Tests pass (7/7) and flutter analyze is clean.
Findings
1. Cached payload can outlive its mortal era — dead-end retry loop (bug)
getUnsignedTransactionPayload builds the payload with eraPeriod: 64 (quantus_sdk/lib/src/services/substrate_service.dart:316) — at ~12s blocks that's a ~13-minute validity window. The cache entry (keystone_sign_cache.dart) has no TTL and is only cleared at send entry points/logout, so this sequence fails:
- User reaches the sign screen (payload cached), backs out to review/amount, gets interrupted (hardware flows are slow — device unlock, battery, etc.).
- Returns >13 min later, taps Confirm again →
_prepare(keystone_sign_screen.dart:67-75) serves the stale QR. - Keystone signs the expired-era payload; submission fails with a bad-era/stale error.
- The scan screen's error path (
keystone_scan_signature_screen.dart:131-144) just says "try scanning again" and restarts the scanner — but the cache is never invalidated, so every retry re-signs the same dead payload. The only escape is backing all the way out and starting a new send, which nothing tells the user to do.
Pre-PR, re-entering the sign screen rebuilt a fresh payload, so this dead end is new. Cheap fixes (either or both):
- Give the entry a TTL derived from the era (store a timestamp; treat entries older than ~
eraPeriod × avgBlockTimeminus a safety margin as a miss inlookup). - Clear the cache in the scan screen's submit-failure path, so backing up one screen rebuilds a fresh QR.
2. Cache isn't cleared on successful submission (robustness)
After a successful send, the entry — whose nonce is now consumed — stays cached (keystone_scan_signature_screen.dart _submit success path). Today every route into the send flow calls startNewSendSession(), so this is unreachable, but the invariant is "every current and future entry point must remember to reset," which is fragile. Clearing on successful submit is one line and removes the risk of a future entry point serving a consumed-nonce payload.
3. Minor
keystoneSignCacheProvideris a global non-autoDispose provider holdingUnsignedTransactionDataincluding the chainregistry; it lives until the next send/logout. Clearing on success (finding 2) also addresses this.KeystoneSignScreen.blockHeight(keystone_sign_screen.dart:29) is now unused — the scan screen gets the height from the payload. Consider dropping the parameter.startNewSendSession()andreset()are identical; one method (or one delegating to the other) would do.
4. Test coverage
keystone_sign_cache_test.dart covers the notifier mechanics (store/lookup/key equality/session reset) well, but not the risky behavior — nothing exercises staleness, because no staleness handling exists. A TTL/era-expiry test should accompany the fix for finding 1.
Verdict: Request changes — finding 1 is a user-visible dead end introduced by the caching and has a cheap fix; findings 2–4 are non-blocking.
n13
left a comment
There was a problem hiding this comment.
Re-review of fix commits (821c104, e81dd73, 589e943)
Re-reviewed the three commits addressing my earlier request-changes review, at head 589e943.
Prior findings
1. Stale-era dead-end — resolved. keystoneSignCacheMaxAge derives the TTL from the payload's own era: eraPeriod × avgBlockTimeSeconds minus a 2-block safety margin (64 × 12s − 24s ≈ 12.4 min), which is correctly conservative versus the ~64-block mortal window. Expiry is handled in lookup as a cache miss that also clears the entry, so _prepare rebuilds a fresh payload instead of erroring — no error loop. The scan screen's submit-failure path now calls reset(), so re-confirming from the review screen always yields a fresh QR. Block time is deduplicated into AppConstants.avgBlockTimeSeconds and MultisigService now uses it (589e943).
2. Cache not cleared on success — resolved. _submit calls startNewSendSession() right after a successful broadcast, before the mounted check, so it runs even if the user has left the screen. This also addresses the memory-lifetime note (registry no longer retained after a completed send).
3. Staleness test coverage — resolved. Two new tests cover the expiry boundary with injected clocks: exactly at max age → miss; one second inside the window → hit. The >= boundary is exercised directly.
New findings (non-blocking)
ref.readafterawaitbut before themountedcheck (keystone_scan_signature_screen.dart:124and:137): in Riverpod 3.x,ConsumerState.refthrows aStateErrorwhen the widget is unmounted (Using "ref" when a widget is about to or has been unmounted), and this is a hardthrow, not a debug assert. If the user pops the scan screen via system back/swipe whilesubmitExternallySignedTransferis in flight (the submitting overlay blocks the in-app back button but not the system gesture), the success-pathref.readat line 124 throws, gets caught by the failure handler, logs a spurious "Keystone signature processing failed" telemetry error for a transaction that actually succeeded, and then line 137 throws again (unhandled). Cheap fix: capturefinal cache = ref.read(keystoneSignCacheProvider.notifier);before theawait(the notifier itself is safe to use after unmount), or move the calls after themountedchecks.- Residual retry-loop within a live sign screen: the failure-path
reset()only benefits a freshKeystoneSignScreeninstance. If the user pops from the scan screen back to the still-alive sign screen, it renders the stale QR from its local_unsignedData/_urPartsstate (initState/_preparedon't re-run on pop-return), so an in-place retry re-signs the same expired payload. Recovery now only requires backing out to Review and confirming again — a big improvement over the previous dead end — but if you want to fully close it,_goToScancouldawaitthe push and re-run_prepareon return.
Test results
flutter test test/unit/keystone_sign_cache_test.dart: 9/9 passed (7 original + 2 new staleness tests).flutter analyzeonmobile-appandquantus_sdk: no issues found.
Verdict: Approve — both blocking findings are properly resolved with test coverage; the two notes above are worth a small follow-up but don't warrant blocking.

Summary
Intentionally didn't disable back button, because don't want to block the flow. User might be stuck if we disable back button.
Screenshots
Note
Medium Risk
Changes hardware-wallet signing payload reuse and navigation; incorrect session clearing could show a stale QR, though new sends explicitly reset the cache.
Overview
Improves the Keystone hardware send flow by caching the unsigned payload and UR QR parts for the current send, and by showing amount, recipient, and checkphrase on the sign screen.
A new Riverpod
keystoneSignCacheProviderholds a single entry keyed by account, recipient, and amount (block height and nonce are excluded so minor chain drift does not force a new QR while the user navigates back within the same send).KeystoneSignScreenreuses the cache on_preparewhen the key matches; otherwise it builds, stores, and displays as before.startNewSendSession()clears the cache when the user starts a fresh send from home (Send, multisig propose, pay intent) or from the shared-address “Send to this account” action so a new flow does not reuse an old QR.When continuing to signature scan, block height is taken from the cached unsigned payload’s
blockNumberinstead of the value passed into the sign screen. Unit tests cover cache keys (address trimming) and notifier store/lookup/session invalidation.Reviewed by Cursor Bugbot for commit b5960c8. Configure here.