Feat/faster qr codes - #599
Conversation
and overall send flow
for testing fast scanning
n13
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES (opinion): the performance work builds and CI passes, but production air-gap, QR-capacity, and Keystone payload-lifetime blockers remain on 9ced530789f77f602cc0ec2c101de1f78e352f85.
Findings:
-
[P1] Keep the network-lock override out of production cold-wallet builds (
cold-wallet-app/lib/screens/settings_screen.dart:42-63,cold-wallet-app/lib/widgets/connectivity_guard.dart:38-46). The release UI now lets any user persistently enable an Override button and then dismiss the guard while the signer is online. That removes the air-gap enforcement which is the cold wallet's core security boundary; a warning and banner do not restore it. Debug builds already bypass the guard. Compile this control only into an explicit non-production test flavor/build flag, and keep release builds fail-closed. -
[P1] Bound the fragment control by the encoded QR capacity, not raw fragment bytes (
cold-wallet-app/lib/providers/settings_providers.dart:10-12,cold-wallet-app/lib/components/animated_ur_qr.dart:37-46). The permitted 1,500-byte setting encodes the real 7,219-byte ML-DSA signature-plus-public-key payload into five UR frames of 2,954 characters each. Version-40 QR at error-correction L accepts 2,953 characters; constructing the frame throwsQrInputTooLongException: 23652 > 23648, so selecting the advertised maximum breaks the signature screen after the transaction has been signed. Lower the bound or dynamically validate the final UR strings againstQrCodecapacity, and add a regression test using the real payload size. -
[P1] Do not carry a nearly expired 16-block payload through the hardware-signing screens (
quantus_sdk/lib/src/constants/app_constants.dart:53-58,mobile-app/lib/v2/screens/send/keystone_sign_cache.dart:9-21,mobile-app/lib/v2/screens/send/keystone_sign_screen.dart:47-75). The cache accepts an entry until two blocks before expiry, butKeystoneSignScreencopies the payload/frames once and never checks their age again; the verify and signature-scan screens continue using that same value. With the new 16-block era, a cache hit can enter the QR screen with only about 24 seconds left, and the review-screen timer's later refresh only replaces provider state, not the QR already displayed. A normal scan/review/sign round trip can therefore submit an expired transaction. Revalidate before advancing/submitting and regenerate the displayed payload when needed, or retain a sufficiently long era while adding an explicit expiry guard. -
[P2] Refresh runtime version when constructing a signing payload (
quantus_sdk/lib/src/services/substrate_service.dart:33-49). A five-minute process-wide cache can straddle a runtime upgrade. During that window both local and Keystone payloads declare the old spec/transaction version, the cold signer can miss the intended spec-drift warning, and the chain rejects the resulting extrinsic until the cache expires or the app restarts. Keep connection/genesis reuse, but fetch the runtime version for each new signing context or invalidate it on an authoritative runtime-change signal and retry stale-version failures.
Validation:
- GitHub
Analyzeworkflow — passed, including formatting, analysis, Mobile App tests, and Quantus SDK tests. cargo test --locked— passed (14 tests, including configurable UR round trips).- Repository-pinned Dart formatting check — passed with zero changed files.
git diff --check d5fc4d6572ebc2244e95009a33f5da5048935867...9ced530789f77f602cc0ec2c101de1f78e352f85— passed.- Targeted UR/QR probe — reproduced 2,954-character frames at the 1,500-byte setting and the QR library's one-character overflow exception.
- Bound animated-QR fragments by encoded UR string length against the 2,953-char version-40 QR capacity (encodeUrForQr), lower the cold wallet slider max to 1400 bytes, and add a native regression test sweeping every fragment setting with the real 7,219-byte ML-DSA payload. - Never carry a nearly expired mortal-era payload through the Keystone screens: cached payloads now expire once less than a 10-block round-trip reserve remains, the sign screen regenerates the displayed QR when its reserve is consumed and refuses to advance stale, and submission checks the live block height against the era window with a dedicated error message. - Clear the cached genesis hash and runtime version when the chain rejects a submitted extrinsic, so a runtime upgrade inside the cache window recovers on the next payload build. - Keep the cold wallet Wi-Fi lock override in release builds intentionally (pre-app-store release testing) and document that in code and in the setting copy, which now says debugging-only and unsafe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review findings in d4d69d1: [P1] Network-lock override in release builds — kept intentionally. The cold wallet is not yet distributed through app stores, and testing release builds on real devices without the override means toggling radios for every flow. The control stays fail-closed by default: the Override button only exists behind a persistent settings opt-in now labelled "debugging only / not safe", an override lasts a single session, and a red banner stays on screen while active. Code comments in [P1] Fragment bound vs QR capacity — fixed. New [P1] Nearly expired era through the signing screens — fixed, conservatively. Cached payloads now expire once less than a 10-block round-trip reserve (~120 s at 12 s blocks) of era lifetime remains, the sign screen re-checks every 5 s and regenerates the displayed QR when the reserve is consumed, advancing to verify is refused with a stale payload (QR regenerates instead), and submission checks the live block height against the era window (2-block margin) and fails with a dedicated "transaction expired" message instead of broadcasting. [P2] Runtime version cache vs runtime upgrade — minimal fix as discussed: when the chain rejects a submitted extrinsic, the cached genesis hash and runtime version are dropped, so the next payload build re-fetches chain state. Connection/genesis reuse on the happy path is unchanged. |
n13
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES (opinion): d4d69d1ae3c90e5c3a74539af01b6655511b1a19 addresses the QR-capacity, Keystone-era, and runtime-cache findings, but the PR is not mergeable with current main, so there is no tested merged result yet.
Blocking finding:
- [P1] Rebase onto current
mainwithout undoing the merged crypto/artifact upgrade (quantus_sdk/rust/Cargo.toml:11-31,quantus_sdk/rust/Cargo.lock,quantus_sdk/rust/src/api/ur.rs:3-25). Currentmainisc1f980b0464be359eae9d2f8d66b91b51666cf40(including #598), while this branch still forks fromd5fc4d6572ebc2244e95009a33f5da5048935867. GitHub reportsCONFLICTING, andgit merge-treeshows unresolved conflicts in all three files. In particular, the PR side ofCargo.tomlstill has the pre-#598qp-rusty-crystals3.0.1 / wormhole 3.1.0 set, whereasmainhas crystals 4.1.0 / wormhole 4.2.0 plus the versioned prover-artifact contract. Resolve by retainingmain's dependency and artifact changes, preserving this PR's configurableencode_ur/encode_bytes_with_optionsAPI, regeneratingCargo.lock, and rerunning the native QR regression and full CI on that resolved head. The current head has no GitHub checks, so taking either conflict side without that validation could silently regress #598 or lose the QR fix.
Prior findings re-evaluated:
- QR capacity — fixed on this head.
encodeUrForQrmeasures final UR strings against the 2,953-character limit; the native regression sweeps 300-1,500-byte settings with the 7,219-byte signature payload, constructs the longest QR, and round-trips successfully. - Keystone era lifetime — fixed on this head. Step 1 refreshes stale payloads and refuses stale advancement, and submission checks the live chain height with a two-block margin and a dedicated expiry error.
- Runtime-version cache — fixed sufficiently for retry behavior. A rejected extrinsic clears the cached runtime/genesis context, so the next payload rebuild refetches chain state.
- Release network override — explicit temporary product/security exception. Given the stated pre-app-store testing scope, I am not treating the deliberately gated, session-only override as an additional code defect in this verdict. It should be removed or build-flag-gated before production distribution.
Validation on the isolated PR head:
cargo test --locked— passed, 14 tests.flutter test --tags native test/ur_qr_frame_test.dart— passed; this regression is excluded by repository CI.flutter test test/unit/keystone_sign_cache_test.dart— passed, 12 tests.flutter analyze .— clean forquantus_sdk,mobile-app, andcold-wallet-app(each under the repository's 20-second limit).- Repository-pinned Dart formatting check — zero changed files.
git diff --check— passed against both the original base and currentmain.- GitHub checks for
d4d69d1a— none reported; the PR is currently conflicting.
Keeps main's qp-rusty-crystals 4.1.0 / wormhole 4.2.0 / poseidon 3.1.0 / plonky2 1.5.5 set and versioned prover-artifact contract while preserving this branch's configurable encode_ur / encode_bytes_with_options API. FRB bindings regenerated on the merged tree: the textual auto-merge had dropped #598's zkCircuitsVersion binding and left the Dart/Rust content hashes inconsistent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Merged current
Validation on the merged head:
|
n13
left a comment
There was a problem hiding this comment.
APPROVE (opinion): no blocking findings remain on 65759a798373c2a04d2888d5bad0b4eb2b59bf70.
The prior merge blocker is resolved correctly:
- The head has
d4d69d1aand currentmain(c1f980b0, including #598) as its two parents; GitHub now reportsMERGEABLEand theAnalyzeworkflow passes. quantus_sdk/rust/Cargo.toml,Cargo.lock, the wormhole artifact implementation, andcircuit_manager.dartmatchmainbyte-for-byte, preserving the crystals 4.1.0 / wormhole 4.2.0 dependency set and versioned artifact cleanup.- The configurable
encode_ur/encode_bytes_with_optionsAPI is preserved. Regenerated FRB bindings include both that API and #598'szkCircuitsVersion; Dart and Rust content hashes both equal300623511, andRustLib.initsucceeds against a freshly rebuilt release dylib.
The earlier QR-capacity, Keystone-era, and runtime-cache findings remain fixed. The release network-lock override remains an explicit temporary pre-app-store testing exception; it should still be removed or build-flag-gated before production distribution, but it is not a blocker for the stated scope.
Validation:
- GitHub
Analyzeworkflow — passed on the reviewed head. cargo test --locked— passed, 40 tests on the merged 4.1/4.2 dependency set../test/build_library.shfollowed byflutter test --tags native— passed, 15 tests; this verifies the regenerated bridge and the 7,219-byte QR-capacity regression against the rebuilt dylib.flutter test test/unit/keystone_sign_cache_test.dart— passed, 12 tests.flutter analyze .— clean forquantus_sdk,mobile-app, andcold-wallet-app, each within the repository's 20-second limit.- Repository-pinned Dart formatting check and
git diff --check origin/main...HEAD— passed.
No blocking findings.
Summary
quantus_urto1.6.0, regenerate Flutter Rust Bridge bindings, and bump the mobile app to1.5.10+124and cold wallet to1.1.0+3.Validation
cargo test --lockedpasses all 14 native SDK tests, including configurable UR fragment round trips.git diff --checkpasses for the complete PR.