Repository navigation
Conversation
reubenyap
left a comment
There was a problem hiding this comment.
Review at 543fbb74
Not ready to merge — two must-fix items. The fund-safety core is much stronger than #1445 and I found no way to lose funds. The serious problems are stuck states: one FIRO failure path leaves a swap permanently stuck while the UI reports it as sent.
Static review only: nothing was compiled or run. Checked against the pinned dependency sources (coinlib fork, web3dart 3.0.1, electrum_adapter, json_rpc_2) and read-only probes of Rosen's API, the Ergo explorer, eth2.stackwallet.com and firo.stackwallet.com. No CI checks have run on the branch.
Must fix
1. FIRO recovery never works when the deposit isn't on the network (high)
transactionExists()treats a tx as absent only onNoSuchTransactionException, and_requiresAttentiononly recognisesJsonRpcException.- The real client never throws either. JSON-RPC errors arrive as
json_rpc_2'sRpcException, whichElectrumXClient.requestrethrows unchanged. - Confirmed live:
firo.stackwallet.comanswers an unknown txid with a JSON-RPC error.paynym_interface.dart:1741already string-matches that format. - Effect: a FIRO broadcast fails (network drop or node rejection), recovery throws, and
confirmSendreturns the txid anyway. The user sees the success animation and the swap sits at "Confirming" forever. _recoverOtherFundingthen blocks every later Rosen deposit from that wallet, and no UI clears it. No funds move.- The tests miss it because the fake Electrum client throws the app types directly.
- Fix:
- Treat any error containing
No such mempool or blockchain transactionas absent. - Have
_requiresAttentionfall back toerror.toString(). - Revert the
electrumx_client.dartchange; that branch is never reached. - Make the fake throw
RpcException.
- Treat any error containing
2. Rebase onto staging, and fix mobile Cancel while doing it (high once rebased)
- The only textual conflict is
send_from_view.dart. - There is also a conflict git won't flag. Staging's #1469 removed the self-pop from
BuildingTransactionDialog, and the PR'sonCancelrelies on that self-pop on mobile. - After a rebase, mobile Cancel leaves a non-dismissible dialog for every provider.
- Fix:
onCancel: () { wasCancelled = true; closeBuildingDialog(); }on both platforms, and keep only the PR side of the catch-block conflict.
Should fix
3. "Refresh quote" loops forever from a stale Trade (medium). refreshTrade starts with currentUnfunded(trade), which throws quoteChanged for exactly the stale copy it is meant to refresh. After one real refresh, Step 4, the desktop model and sibling wallet cards all hold stale copies and can never refresh. The workaround is reopening the swap from trade history. Fix: load the persisted trade by uuid and call requireUnfunded, as #1445 did.
4. Uncertain sends look like success, and stuck deposits have no exit (medium).
- Any failure after the deposit is saved that recovery can't resolve returns a txid, and the confirm screen shows the normal success tick.
- Deposits flagged
rejected,invalidorunexpectedResponseare never retried. Those flaggedfeesretry only if the fee schedule reverts; Rosen's base fee went from about 11.3 to 8.33 FIRO between 2026-09-17 and 2026-10-08. - In all these states the wallet cannot start another Rosen deposit. The only exit is spending the saved inputs or nonce elsewhere and waiting 10 or 50 confirmations, which the UI doesn't say.
- Suggest a distinct "saved, will retry, do not send again" result, plus an abandon action or a documented manual exit.
5. The ETH send path changed for every ETH and ERC-20 send (medium). EthereumWallet.confirmSend now hashes locally and throws if the node returns a different hash, after the tx is already broadcast. The signed bytes match web3dart 3.0.1's own sendTransaction, so this only bites on a non-compliant node. Gate the new path on beforeBroadcast != null. A bump to web3dart 3.0.3 or later would double-prefix, because signTransaction adds the 0x02 prefix itself there.
6. _preparing drops taps after Cancel, all providers (medium, unfixed from #1445). The guard clears only when the abandoned build settles. Firo "private balance", Cancel, "public balance" does nothing until the Spark build finishes. Clear it in onCancel.
7. Test gaps on the fund-moving path. RosenFunding.confirmSend is tested only on its pre-signing rejections; nothing drives it through save, broadcast and mark, or through the failure fallback. prepareSend happy paths and refreshTrade are untested. The "coin selection" test builds coinlib's CoinSelection itself and would pass with the production change reverted.
Minor
- Next stays disabled: a wallet-initiated rsFIRO → FIRO swap prefills the recipient but never re-enables Next (step_2_view.dart, unfixed).
- Raw error for low rsFIRO: insufficient balance surfaces as
RPCError: got code 3 with msg "execution reverted".The wallet list shows only the ETH balance, and its label is missing a space. - Recovery calls: they have no timeout while holding the per-wallet lock, and ETH recovery makes clearnet calls even with Tor on.
- Event status: Rosen's
statuscan be an object orMULTIPLE_FLOWS, which leaves the swap at "Exchanging";fraudis terminal although a later flow can succeed (unfixed). - Dropped deposits: one already marked
broadcastthat later drops stays "Confirming" forever. - Changes not gated to Rosen:
- the initial step-4 status label;
- an immediate
_checkTrades()at startup; - the
cacheVersionbump re-arming the "Updating exchange data" overlay; - case-insensitive ETH contract matching.
- Backup restore: it uses
TradesService.add(), bypassing the Rosen merge (unfixed). - Docs: they say ambiguous submissions "remain Verifying", but they stay "Confirming".
Simplifications
- ETH conflict detection: the binary search and block scan can be
countAt(safeHeight) > nonce. The caller'stransactionExists()already tells our tx from a conflicting one. That removes about 80 lines, theunknownstate and the archive-node dependency. - Duplicate and dead code:
_markFundingObservedduplicates_markFundingState, and theEstimate.warningMessagestrings are never rendered. - Fee-box fetch: every estimate re-downloads the 58-box list; a short cache would do.
- FIRO parser: the hand-rolled parser exists because the pinned coinlib fork rewinds two bytes instead of one for legacy transactions. The parser is sound, but the fork bug deserves its own fix.
Fixed since #1445
- The three missing-import compile errors.
- The deposit is saved before broadcast.
- Reverted ETH deposits become Failed after 50 confirmations.
registerTokenis awaited with errors surfaced.- The fee check is a real
flutter test.
Checked and found correct
- No send without metadata: every entry point routes through
RosenFunding, which re-validates the signed bytes. The lock address and QR are hidden on step 4 and trade details. - Config: pinned identifiers match Rosen 7.1.1, and the live fee box decodes with exactly one FIRO candidate.
- Fees: the OP_RETURN size is included in fee planning on every reachable branch.
- Compile: a static pass over the changed files found no unresolved symbols or invalid overrides.
543fbb7 to
e29928c
Compare
|
Verified the review against the pinned dependencies and added regression tests before accepting the fixes. Updated in Addressed the numbered findings:
Minor findings addressed:
Suggestions retained/deferred after verification:
Validation: 162 focused tests passed on the final tree, including existing swap providers/forms and current staging's desktop fee regressions. All 49 changed Dart files pass formatting; analysis reports no errors (existing warnings/style notices remain). The PR remains draft: no live transfer or complete native/platform build has been performed. The review is one general comment; there are no inline review threads to mark resolved. |
Adds native FIRO ↔ rsFIRO (Ethereum mainnet) swaps through Rosen Bridge using Stack's existing swap wizard, wallet selection, authentication, signing, trade history, and polling.
Review order: the wallet primitives and their tests are extracted into prerequisite #1472. Its nine files are identical to this branch and account for 1,166 additions. Merge that prerequisite first, then rebase this PR to remove those files from the bridge diff; they remain visible here until then.
FIRO deposits use transparent funds and validated OP_RETURN metadata. rsFIRO deposits use ERC20 transfer calldata with appended metadata and require ETH for gas. Fees use exact integer arithmetic and are rechecked before funding; changed quotes require another review. Signed bytes are saved before submission, and interrupted or ambiguous broadcasts recover using only those bytes. Backup restore preserves signed journals and wallet ownership. Uncertain deposits remain Verifying and block a second deposit until safely resolved.
The bridge reuses StackOkDialog and the existing desktop Step 4 layout. Rosen hides ordinary address/memo/QR deposit instructions and retains pending/submitted/attention messages. Shared test fixtures use the existing bundled Isar initializer. The cleanup removes 97 production lines and 119 lines of repeated test setup net; expanded mobile/desktop regression coverage keeps the overall line count flat. No recovery validation or assertions were dropped.
Scope remains this route. No new dependencies or CI changes. The app-specific rsFIRO catalog and migration from #1448 are retained. Protocol configuration, quote validity, and safe recovery guidance are documented in docs/rosen_bridge.md.
Validation on b8fde75: 147 tests passed across existing exchange providers/forms, Rosen protocol/API/fees/funding/recovery, backup journal preservation, mobile/desktop confirmation and cancellation, desktop bridge/ordinary deposit instructions, token registration, FIRO coin selection and raw transaction validation, signing/broadcast ordering and deadlines, Ethereum fee caps/replacement detection, Electrum/MWEB fees, and fee-entry UI regressions. The isolated prerequisite passed 42 wallet regression tests. Changed Dart files pass formatting; analysis reports no errors (existing warnings/style notices remain). Local tests use Flutter 3.47.2 / Dart 3.13.2 with data-only asset mode (
FLUTTER_NATIVE_ASSETS=false FLUTTER_DART_DATA_ASSETS=true); no production source or dependency was substituted.Draft pending full native/platform builds, interactive mobile/desktop checks, and a small real-funds transfer in each direction. No live transfer was performed.