Repository navigation
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
mossbot55
left a comment
There was a problem hiding this comment.
Reviewed the candidate at 6c88114fc6df1f4df831ef631a39e59f9e9bff23 on base 5e193821 (the #552 branch).
The platform encodings match the vendored Argent account: trigger_escape_guardian(None) with calldata 0x1, escape_guardian() with empty calldata, the 5 STRK escape cap, the 7 day default security period, the get_escape_and_status read (ready_at first, escape_type second, status last, with the variable Option in between) and the 0..3 status codes all check out against vendor/argent-contracts. The new messages join every registry the project keeps (wallet-ops dispatch, key-boundary catalog and collaborators, runtime-rpc, response validators, privileged-sender gate). A full bun run check is green at the head (root 2449 pass / 0 fail across 258 files; 22 workspace checks, 0 fail), with the new argent-recovery, background-signability and sidepanel-home suites passing.
Material blockers, detail inline:
- The submit does not enforce the reviewed fee as a ceiling; only the account's 5 STRK cap bounds it.
- A broadcast-hash mismatch is reported as a determinate failure instead of through the repo's indeterminate-write path (
matchingAcceptedRpcWriteHash). - The new submit handler does not translate a lost-reply write into a
submissionUnknownresponse, unlike the transfer, swap and STRK20 lanes. - A completed recovery step does not force a signability refresh, so the Home flag can lag the action by up to five minutes.
Non-blocking notes inline: the quoted delay is the hardcoded 7 day default rather than the account's live security period; a guardian backup would surface a raw contract error instead of a curated refusal; and the review fee quote signs a hash with the real key where the repo's estimate path avoids that.
Overlap: this PR is stacked on #552, which carries the same three write-safety gaps for the Braavos sibling, so they can be addressed together. Other open PRs share touched files (runtime.ts, runtime-types.ts, runtime-rpc.ts, responses.ts, home-view.ts, account-signability.ts): #546, #548, #550, #525, #528, #531, #540, #450. No competing implementation of issue #544 is open.
Limitations: I did not run the vendored contract on a devnet (the encoding claims come from the Cairo sources, the vendor's own JS helper and its integration tests), and I did not do a browser pass of the flag's three states (the rendering evidence is the unit markup tests plus source inspection).
Time: 1h 6m 23s. Approx. API cost: $1.08.
| const spend = reviewedSpendFor(estimate, "standard"); | ||
| if (spend.unit !== reviewedFee.unit || BigInt(spend.overallFee) > MAX_ESCAPE_FEE_FRI) { | ||
| throw new WalletError("The recovery fee exceeds the account's on-chain escape cap"); | ||
| } |
There was a problem hiding this comment.
The submission never compares the re-estimated spend against reviewedFee.overallFee; only the unit and the account's MAX_ESCAPE_FEE_FRI cap are checked, so a fee that moved up between review and submit is signed anyway, up to 5 STRK. Every other chain-write lane treats the reviewed fee as the ceiling the write may sign under (the transfer handler in apps/extension/src/background/accounts/wallet-ops.ts, MCW-V09-119), and the sibling Braavos lane refuses a re-estimate above the reviewed fee ('The recovery fee changed. Review it and try again.'). Consider the same bound, or a re-review when the estimate moves. Separately, the error text claims the on-chain escape cap was exceeded even when only the unit mismatched.
| const transactionHash = acceptedRpcWriteHash(submitted.transaction_hash); | ||
| if (signedHash === undefined || BigInt(signedHash) !== BigInt(transactionHash)) { | ||
| throw new WalletError( | ||
| "RPC accepted a different transaction hash. Check on-chain state before retrying.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
acceptedRpcWriteHash only validates the shape; the comparison below throws a plain WalletError when the accepted hash differs from the hash handed to the signer. That mismatch is a post-broadcast ambiguity (the node may already hold the write), which is exactly what packages/platform/starknet/src/rpc/provider.ts provides for: matchingAcceptedRpcWriteHash(submitted.transaction_hash, signedHash) throws IndeterminateRpcWriteError with this same message. Using it keeps the outcome classified as unknown rather than a determinate failure.
| return { error: "account recovery unavailable" }; | ||
| } | ||
| const authorized = captureSessionAuthorization(deps.session); | ||
| const submitted = await deps.runtimeRpc.submitArgentRecovery({ |
There was a problem hiding this comment.
A chain write whose reply was lost should reach the panel as a submissionUnknown response, as the transfer, swap and STRK20 lanes do; here submitArgentRecovery is awaited with no IndeterminateRpcWriteError catch, so the error propagates to the dispatcher (apps/extension/src/background/routing/messages.ts) and is flattened to a plain { error }. The panel already recognises the shape (responseError builds a SubmissionUnknownError), so only the background side is missing. This needs a way to carry the signed hash out of submitArgentRecovery for the response (the transfer lane uses an onSigned callback).
| await (kind === "argent" | ||
| ? deps.wallet.recoverArgentAccount(fee) | ||
| : deps.wallet.recoverBraavosAccount(fee)); |
There was a problem hiding this comment.
This success path re-renders via deps.render, which carries no refresh reason, so hydrate calls signability.load(..., force=false) and the cached verdict is reused for up to five minutes (app/sync/signability-cache.ts, VERDICT_FRESH_MS = 300_000). Nothing else forces it: wallet.recoverArgentAccount (and wallet.reviewArgentRecovery) are absent from MUTATING_MESSAGES/CHAIN_SETTLING_MESSAGES in app/sync/runtime-refresh.ts, and only the burst path (chain-settling) promotes the refresh to action, which is what sets force. So right after the trigger the flag keeps offering the first step instead of the new pending state, and the user's next press fails with 'Guardian removal is still in its waiting period'. Registering the recovery submit as a chain-settling mutation fixes it.
| rpcUrls, | ||
| signHash: accountHashSigner(deps, account, context.vaultPassphrase), | ||
| }); | ||
| return { accountId: account.id, delaySeconds: ARGENT_ESCAPE_DELAY_SECONDS, fee, ok: true }; |
There was a problem hiding this comment.
Minor: this quotes the hardcoded DEFAULT_ARGENT_ESCAPE_SECURITY_PERIOD_SECONDS, but the account can configure its own period (get_escape_security_period, minimum 10 minutes), and the Braavos sibling reads its account's live delay (returning null when unavailable). The authoritative pending time still comes from the chain (readyAt), so only the 'usable in N days' line can be wrong.
| if (restriction === null) { | ||
| throw new WalletError("This account has no guardian to remove"); | ||
| } | ||
| if (restriction !== "guardian") { |
There was a problem hiding this comment.
Minor: requireRecoverableArgent reads the class, owner, owner type and guardian but not get_guardian_backup. The contract reverts trigger_escape_guardian(None) when a backup is set (argent/backup-should-be-null), so a guardian account that also has a backup passes review and then fails at estimation with a raw contract error instead of a curated refusal.
| makeProvider, | ||
| ); | ||
| const call = nextCall(account.address, status); | ||
| const estimate = await makeAccount(provider, account, signHash).estimateInvokeFee(call, { |
There was a problem hiding this comment.
Minor: this review estimate passes the real phrase signer with skipValidate: false, so pressing review signs one hash with the private key. The repo's estimate path (packages/platform/starknet/src/rpc/rpc-estimate.ts) gets a validated Argent estimate from argentQueryAccount without reaching for key material, which would keep the quote free of private signing. Not blocking (the query version is not broadcastable).
A Ready (Argent) account with a guardian was view-only with no way forward from the phrase. Argent's escape is the symmetric feature to Braavos' deferred removal: the owner key alone may remove the guardian, in the two steps the account's escape carve-out accepts from a single owner signature. A new platform module runs that flow. trigger_escape_guardian(None), a single self-call with the owner signature, starts the security period (seven days by default); after it, escape_guardian() removes the guardian and the account signs normally again — #508's checks already pass once get_guardian is zero. It deliberately bypasses the guardian refusal that guards every other submission, mirroring the account's carve-out, and refuses anything but a deployed guardian-restricted account whose owner is the wallet key. The escape fee is bounded by the account at five STRK, so that protocol cap is the ceiling the submission honours; the reviewed fee is the quote the user saw. Estimates run with validation on, because the escape call does real work in __validate__ (the anti-DoS rate-limit bookkeeping) that a skipped-validation estimate under-quotes — the first mainnet attempt reverted out of gas on exactly that and the module now estimates correctly. The escape state is read through the account's own get_escape_and_status, whose status reflects the chain's clock. The background exposes privileged review and submit handlers and the signability verdict carries the escape stage, so the read-only Home flag renders the Ready states alongside Braavos: a guardian account offers "Recover account"; a running escape shows when it unlocks and that the guardian can still cancel; once the delay passes it offers "Complete recovery"; and it clears when the guardian is gone. Cancelling needs the owner and guardian together, so the phrase cannot stop it alone. Proven on mainnet through the wallet's own review/submit path: ready-matrix/20 (guardian set) accepted the escape trigger and reads pending, completable after the seven-day security period. Fixes #544. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
5e19382 to
886002a
Compare
6c88114 to
75dd885
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Closing in favour of #559, which brings this change into v0.10.5 with your commits unchanged (only an import-line rebase conflict resolved), together with the fixes from four independent senior reviews as separate labelled commits, so the whole stack ships with one review. Thanks, Henri! |
Fixes #544. Stacked on #552; retarget down the stack as it merges. The Ready twin of the Braavos recovery.
Problem
A Ready (Argent) account with a guardian was view-only with no way forward from the phrase. Argent's escape is the symmetric feature to Braavos' deferred 2FA removal — the owner key alone removes the guardian.
Solution
argent-recovery.ts): the two-step owner-only escape.trigger_escape_guardian(Option::None)(a single self-call, owner signature) starts the security period (7 days by default); after it,escape_guardian()removes the guardian and Find Ready and Braavos accounts on the ladders their apps actually use #508's checks pass onceget_guardianis zero. Bypasses the guardian refusal, mirroring the account's carve-out; refuses anything but a deployed guardian-restricted account owned by the wallet key.__validate__work (anti-DoS rate-limit bookkeeping) that a skipped-validation estimate under-quotes. The first mainnet attempt reverted out of gas on exactly this; the module now estimates correctly. (Also learned on-chain: a reverted escape still consumes the account's 12h rate-limit slot, since__validate__writes persist.)get_escape_and_status(status on the chain's clock), surfaced as the verdict's recoverystage.Verification
ready-matrix/20(guardian set) accepted the escape trigger; it reads pending, completable after the 7-day period.LC_ALL=en_US.UTF-8 bun run checkpasses end to end.🤖 Generated with Claude Code