Of the three options the issue named, this takes the third: refuse a second transaction approval while one is pending.
Re-populate at Confirm reintroduces exactly the gap #216 closed. The nonce on the approval screen would no longer be the nonce that gets signed, so the artifact could not be verified against the displayed object without either lying about what was displayed or dropping the nonce from the comparison.
Allocate around in-flight approvals makes the wallet's own bookkeeping the authority on a nonce the network has not accepted. An approval the user abandons — the window left open, the worker terminated — then leaves a hole that every later transaction queues behind until something reconciles it.
Refuse keeps the displayed object the verified object, holds no state the network can contradict, and fails the second request while the page is still waiting and nothing has been shown. For a wallet that signs from one address at a time, one unanswered transaction is the natural limit.
What changed
src/background/index.js
A single transaction-approval slot. It is taken inside handleSendTransaction(), after the connection and from checks and immediately before prepareApprovalTx(): a page the wallet was never going to serve must not be able to hold the slot and make the connected site's own transaction fail. The take is atomic because nothing awaits between its test and its set, not because of where it sits, so two requests delivered in the same tick still cannot both pass it. The second request is refused with EIP-1193 -32002 (resource unavailable, the standard code for "already pending") and never reaches prepareApprovalTx(): no second nonce, no second window. Signature approvals are not gated, consuming no nonce.
The slot is a handle carrying the id of the approval it was taken for, and it is freed inside settleApproval() — the single point an approval leaves pendingApprovals. Every path that retires an approval therefore ends the hold; the holder's finally is the backstop for the interval before an approval exists, and frees nothing if another request has since taken the slot.
Two paths that could leave an approval standing forever — and so hold the slot for the life of the worker — are closed. An approval whose window the user closed while an attempt owned it is marked as having lost its window; if that attempt then fails retryably, it is settled as the 4001 the closed window already meant rather than left with no window and no resolver. An approval whose window windowsApi.create never produced is settled at once with -32603, saying plainly that nothing was shown. An approval settled while its window was opening has that window closed instead of throwing.
Nonces this worker has broadcast are recorded per chain and address and checked before the artifact is handed to the node. A node's pending count can lag a transaction it has itself just accepted, and a request populated inside that window would otherwise be signed and sent at a nonce this wallet has already used. Nonce spaces are per chain and low nonces overlap across chains routinely, so the chain is part of the key; the chain is read once per attempt and used for verification, for the check and for the record write alike. The record dies with the worker, which is right: after a restart the node's count is the only answer available.
Each sendResponse now reports outcome.stage rather than the stage it passed in, because a broadcast failure the node blamed on the nonce is reclassified.
src/shared/approvalVerify.js
isNonceCollision() and a new TX_STAGE_NONCE. Classified from ethers' NONCE_EXPIRED / REPLACEMENT_UNDERPRICED codes and from the node's own words (nonce too low, nonce has already been used, invalid nonce, OldNonce, replacement transaction underpriced, replacement fee too low), including the nested info.error.message shape ethers hands up when it could not classify the error itself.
already known is deliberately not a collision: a node that says it knows the transaction has it, so it did reach the network and the existing ambiguous wording is the correct one for it.
The copy. A nonce collision produces "The transaction was not sent, because its nonce had already been used by another transaction." to the page, and in the popup that plus "The transaction did not reach the network. Please send it again from the site." The node's own fragment (nonce too low) is replaced rather than passed through: it is not a sentence, and it says less than the wallet knows.
Verification is untouched. The approval still carries the transaction the screen displayed, and the artifact is still compared against that object field for field.
The tests, failing first
Every test here was demonstrated failing against the tree it was written for and passing after the change.
Against next at 9dcd875 with all tests kept: 9 failed, 684 passed. The whole of the defect is visible in the first one — the second request's result is null, because it had raised its own approval window at the same nonce the first one is holding, instead of being answered.
The rework's own four tests, run against the pre-rework head 73db8ee with src/ reverted and the tests kept — 5 failed, 28 passed in tests/backgroundApproval.test.js:
● a nonce spent on one chain is not refused on another
Expected number of calls: 2
Received number of calls: 1
● an approval whose window closed under a failed attempt is answered, and
frees the next request
Expected: {"error": {"code": 4001, "message": "User rejected the request."}}
Received: null
● a request whose approval window cannot be opened is answered rather than
left waiting
Expected: {"error": {"code": -32603, "message": StringMatching /could not
open its approval window/}}
Received: null
● a request the wallet refuses does not take the slot from the connected site
expect(received).toBeNull()
Received: {"error": {"code": -32002, ...}}
The fifth is the existing concurrency test, red on the old refusal copy.
One existing assertion changed rather than being added to: the terminal-broadcast test looped over four node messages asserting the wallet passes each through verbatim, and replacement transaction underpriced is now one of the reclassified ones. It is still asserted terminal, in the new nonce test, with the new message.
Verification
make check — green: 29 suites, 715 tests, script/verify-build 18 cases, prettier --check clean. Also green containerized through script/cibuild, with make check executing as build step 7/8 in 18.8s rather than CACHED. make fmt run; the branch is rebased on next at d9d50f0.
Not covered, and knowingly so: the popup's own Send screen populates at send time and is outside the approval record, so it is not gated by this slot. A nonce it takes under an open dApp approval still collides — and that collision now reports accurately through both paths above, which is the part of it this issue asked for. An approval the user never answers holds the slot until the window is closed or the worker restarts; that is the same interval in which its nonce is allocated and unspent, so it is the intended behaviour rather than a leak. An attempt whose broadcast never settles holds the claim and the slot indefinitely, which is the pre-existing shape of attemptInFlight and unchanged here.
Closes [#271](https://git.eeqj.de/sneak/AutistMask/issues/271).
## The decision, and why
Of the three options the issue named, this takes the third: **refuse a second transaction approval while one is pending**.
- **Re-populate at Confirm** reintroduces exactly the gap [#216](https://git.eeqj.de/sneak/AutistMask/issues/216) closed. The nonce on the approval screen would no longer be the nonce that gets signed, so the artifact could not be verified against the displayed object without either lying about what was displayed or dropping the nonce from the comparison.
- **Allocate around in-flight approvals** makes the wallet's own bookkeeping the authority on a nonce the network has not accepted. An approval the user abandons — the window left open, the worker terminated — then leaves a hole that every later transaction queues behind until something reconciles it.
- **Refuse** keeps the displayed object the verified object, holds no state the network can contradict, and fails the second request while the page is still waiting and nothing has been shown. For a wallet that signs from one address at a time, one unanswered transaction is the natural limit.
## What changed
`src/background/index.js`
- A single transaction-approval slot. It is taken inside `handleSendTransaction()`, **after** the connection and `from` checks and immediately before `prepareApprovalTx()`: a page the wallet was never going to serve must not be able to hold the slot and make the connected site's own transaction fail. The take is atomic because nothing awaits between its test and its set, not because of where it sits, so two requests delivered in the same tick still cannot both pass it. The second request is refused with EIP-1193 `-32002` (`resource unavailable`, the standard code for "already pending") and never reaches `prepareApprovalTx()`: no second nonce, no second window. Signature approvals are not gated, consuming no nonce.
- The slot is a handle carrying the id of the approval it was taken for, and it is freed inside `settleApproval()` — the single point an approval leaves `pendingApprovals`. Every path that retires an approval therefore ends the hold; the holder's `finally` is the backstop for the interval before an approval exists, and frees nothing if another request has since taken the slot.
- Two paths that could leave an approval standing forever — and so hold the slot for the life of the worker — are closed. An approval whose window the user closed while an attempt owned it is marked as having lost its window; if that attempt then fails retryably, it is settled as the 4001 the closed window already meant rather than left with no window and no resolver. An approval whose window `windowsApi.create` never produced is settled at once with `-32603`, saying plainly that nothing was shown. An approval settled while its window was opening has that window closed instead of throwing.
- Nonces this worker has broadcast are recorded **per chain and address** and checked **before** the artifact is handed to the node. A node's pending count can lag a transaction it has itself just accepted, and a request populated inside that window would otherwise be signed and sent at a nonce this wallet has already used. Nonce spaces are per chain and low nonces overlap across chains routinely, so the chain is part of the key; the chain is read once per attempt and used for verification, for the check and for the record write alike. The record dies with the worker, which is right: after a restart the node's count is the only answer available.
- Each `sendResponse` now reports `outcome.stage` rather than the stage it passed in, because a broadcast failure the node blamed on the nonce is reclassified.
`src/shared/approvalVerify.js`
- `isNonceCollision()` and a new `TX_STAGE_NONCE`. Classified from ethers' `NONCE_EXPIRED` / `REPLACEMENT_UNDERPRICED` codes and from the node's own words (`nonce too low`, `nonce has already been used`, `invalid nonce`, `OldNonce`, `replacement transaction underpriced`, `replacement fee too low`), including the nested `info.error.message` shape ethers hands up when it could not classify the error itself.
- `already known` is deliberately **not** a collision: a node that says it knows the transaction has it, so it did reach the network and the existing ambiguous wording is the correct one for it.
- The copy. A nonce collision produces "The transaction was not sent, because its nonce had already been used by another transaction." to the page, and in the popup that plus "The transaction did not reach the network. Please send it again from the site." The node's own fragment (`nonce too low`) is replaced rather than passed through: it is not a sentence, and it says less than the wallet knows.
Verification is untouched. The approval still carries the transaction the screen displayed, and the artifact is still compared against that object field for field.
## The tests, failing first
Every test here was demonstrated failing against the tree it was written for and passing after the change.
Against `next` at `9dcd875` with all tests kept: **9 failed, 684 passed**. The whole of the defect is visible in the first one — the second request's result is `null`, because it had raised its own approval window at the same nonce the first one is holding, instead of being answered.
The rework's own four tests, run against the pre-rework head `73db8ee` with `src/` reverted and the tests kept — 5 failed, 28 passed in `tests/backgroundApproval.test.js`:
```
● a nonce spent on one chain is not refused on another
Expected number of calls: 2
Received number of calls: 1
● an approval whose window closed under a failed attempt is answered, and
frees the next request
Expected: {"error": {"code": 4001, "message": "User rejected the request."}}
Received: null
● a request whose approval window cannot be opened is answered rather than
left waiting
Expected: {"error": {"code": -32603, "message": StringMatching /could not
open its approval window/}}
Received: null
● a request the wallet refuses does not take the slot from the connected site
expect(received).toBeNull()
Received: {"error": {"code": -32002, ...}}
```
The fifth is the existing concurrency test, red on the old refusal copy.
One existing assertion changed rather than being added to: the terminal-broadcast test looped over four node messages asserting the wallet passes each through verbatim, and `replacement transaction underpriced` is now one of the reclassified ones. It is still asserted terminal, in the new nonce test, with the new message.
## Verification
`make check` — green: 29 suites, 715 tests, `script/verify-build` 18 cases, `prettier --check` clean. Also green containerized through `script/cibuild`, with `make check` executing as build step 7/8 in 18.8s rather than `CACHED`. `make fmt` run; the branch is rebased on `next` at `d9d50f0`.
Not covered, and knowingly so: the popup's own Send screen populates at send time and is outside the approval record, so it is not gated by this slot. A nonce it takes under an open dApp approval still collides — and that collision now reports accurately through both paths above, which is the part of it this issue asked for. An approval the user never answers holds the slot until the window is closed or the worker restarts; that is the same interval in which its nonce is allocated and unspent, so it is the intended behaviour rather than a leak. An attempt whose broadcast never settles holds the claim and the slot indefinitely, which is the pre-existing shape of `attemptInFlight` and unchanged here.
Populating the transaction in the background before the approval window
opens is what makes the displayed object the verified object. It also
fixes the nonce before the user has answered anything, so two
eth_sendTransaction calls populated concurrently took the same nonce
from a node that had seen neither of them broadcast, and the second
could never be sent: its approved nonce is spent, and the only way to
give it a fresh one is to populate it again after the user has read the
old one off the screen.
A second transaction approval is now refused while one is unanswered,
with EIP-1193 code -32002. The refusal happens before anything is
populated — no second nonce is allocated, no window opens — and the slot
is released when the requesting page has its answer. Signature approvals
are not gated; a signature consumes no nonce.
A collision that does happen is now reported for what it is. A broadcast
the node refused for the nonce, and an approval carrying a nonce this
worker has already broadcast (caught before the node is asked at all),
both report that the transaction did not reach the network and to send
it again, instead of the standing broadcast wording that warns it may
have sent. "already known" keeps that ambiguous wording deliberately: a
node that says it has the transaction has it.
Nothing about verification is weakened. The approval still carries the
transaction the screen displayed, and the artifact is still compared
against that object field for field.
FAIL — needs-rework. Three defects, all in src/background/index.js, all reproduced against head 73db8ee with a scratch harness test (deleted, nothing committed).
1. src/background/index.js:117-124 and :1130-1151 — the broadcast-nonce record is keyed by address only, so a nonce spent on one chain blocks that nonce on every other chain.broadcastNoncesFor(address) has no chain component, while the wallet switches networks (wallet_switchEthereumChain at :502-510, currentNetwork().chainId at :1101) and nonce spaces are per chain. Reproduced: broadcast at nonce 7 on chain 1, switch network, approve a transaction the node populated at nonce 7 on chain 11155111 → broadcastTransaction was never called for it (call count stayed at 1) and both the page and the popup were told "The transaction was not sent, because its nonce had already been used by another transaction." That is false — no transaction of this user's has used that nonce on the new chain — and it is unrecoverable: the copy says "Please send it again from the site", the resend repopulates the same nonce, and it is refused identically for the life of the worker. Low nonces overlap across chains routinely, so this blocks ordinary multi-chain use with an inaccurate message, in the exact direction the issue was filed to stop. Acceptable: key the record by chain id plus address, record under the chain the broadcast actually went out on, and check only the chain that is current.
2. :674-681 — the slot is released only when the approval promise resolves, and there are paths on which it never resolves; the wallet then refuses every eth_sendTransaction for the life of the worker.finally { releaseTxApprovalSlot(); } waits on requestTxApproval(). An approval whose attempt is claimed, whose window is then closed (:934-950, where settleApproval() correctly declines while attemptInFlight), and whose attempt then fails retryably (:1116releaseApproval(approval) — reached by any non-mismatch throw: loadState(), getActiveAddress(), an unparseable artifact) is left in pendingApprovals with no window and no resolver. Reproduced: after that sequence the first request's result is still null and the next eth_sendTransaction returns -32002 with zero windows opened, permanently. windowsApi.create handing back no window at :278-282 strands it the same way. Before this PR that sequence stranded one request; with the slot it denies the wallet's main function until the worker restarts. Acceptable: bind the slot to the approval id and free it whenever that approval leaves pendingApprovals or loses its window — including the window-closed path that currently declines to settle — rather than only on promise resolution.
3. :663-673 — the slot is taken before the authorization check, so any web page can make the user's single legitimate transaction fail with "already pending".reserveTxApprovalSlot() is the first statement of the branch, ahead of await getState() / getActiveAddress() and the 4100 Unauthorized return inside handleSendTransaction(). Reproduced: an unconnected origin's eth_sendTransaction (itself rejected 4100) held the slot while the connected dApp's own single request was refused with -32002 and no window opened. Any page can loop this and deny sending wallet-wide. TX_APPROVAL_PENDING_MESSAGE is also untrue in that window: nothing is "already waiting to be approved" and there is nothing for the user to answer. Acceptable: the take is atomic because nothing awaits between test and set, not because of where it sits in the function — move it after the authorization and from checks, immediately before prepareApprovalTx().
Verified and passing: make check green here (28 suites, 689 tests, script/verify-build 18 cases, prettier clean); the seven new/changed tests are load-bearing — reverting src/ to 9dcd875 with the tests kept gives 7 failed / 682 passed, matching the PR body; replacement transaction underpriced is correctly a collision (the node refused that artifact) and already known correctly is not; verification is untouched and the nonce check sits after verifySignedTx(), so #216 is not weakened; merges cleanly onto current next (0be20d7), no conflict with #282 today; commit title, TODO.md in the same commit, terminology and formatting all fine; no attribution trailers.
Disclosures: the head commit's CI check is still "Waiting to run", so CI green is unconfirmed — the local make check above is the evidence. Neither e2e suite was run (docker browser suites, not part of check). The disclosed popup-Send gap is waived against the issue's definition of done: the Send screen is not an eth_sendTransaction request, and finding 1 is the part of it that matters.
FAIL — `needs-rework`. Three defects, all in `src/background/index.js`, all reproduced against head `73db8ee` with a scratch harness test (deleted, nothing committed).
**1. `src/background/index.js:117-124` and `:1130-1151` — the broadcast-nonce record is keyed by address only, so a nonce spent on one chain blocks that nonce on every other chain.** `broadcastNoncesFor(address)` has no chain component, while the wallet switches networks (`wallet_switchEthereumChain` at `:502-510`, `currentNetwork().chainId` at `:1101`) and nonce spaces are per chain. Reproduced: broadcast at nonce 7 on chain 1, switch network, approve a transaction the node populated at nonce 7 on chain 11155111 → `broadcastTransaction` was never called for it (call count stayed at 1) and both the page and the popup were told "The transaction was not sent, because its nonce had already been used by another transaction." That is false — no transaction of this user's has used that nonce on the new chain — and it is unrecoverable: the copy says "Please send it again from the site", the resend repopulates the same nonce, and it is refused identically for the life of the worker. Low nonces overlap across chains routinely, so this blocks ordinary multi-chain use with an inaccurate message, in the exact direction the issue was filed to stop. Acceptable: key the record by chain id plus address, record under the chain the broadcast actually went out on, and check only the chain that is current.
**2. `:674-681` — the slot is released only when the approval promise resolves, and there are paths on which it never resolves; the wallet then refuses every `eth_sendTransaction` for the life of the worker.** `finally { releaseTxApprovalSlot(); }` waits on `requestTxApproval()`. An approval whose attempt is claimed, whose window is then closed (`:934-950`, where `settleApproval()` correctly declines while `attemptInFlight`), and whose attempt then fails retryably (`:1116` `releaseApproval(approval)` — reached by any non-mismatch throw: `loadState()`, `getActiveAddress()`, an unparseable artifact) is left in `pendingApprovals` with no window and no resolver. Reproduced: after that sequence the first request's result is still `null` and the next `eth_sendTransaction` returns `-32002` with zero windows opened, permanently. `windowsApi.create` handing back no window at `:278-282` strands it the same way. Before this PR that sequence stranded one request; with the slot it denies the wallet's main function until the worker restarts. Acceptable: bind the slot to the approval id and free it whenever that approval leaves `pendingApprovals` or loses its window — including the window-closed path that currently declines to settle — rather than only on promise resolution.
**3. `:663-673` — the slot is taken before the authorization check, so any web page can make the user's single legitimate transaction fail with "already pending".** `reserveTxApprovalSlot()` is the first statement of the branch, ahead of `await getState()` / `getActiveAddress()` and the `4100 Unauthorized` return inside `handleSendTransaction()`. Reproduced: an unconnected origin's `eth_sendTransaction` (itself rejected `4100`) held the slot while the connected dApp's own single request was refused with `-32002` and no window opened. Any page can loop this and deny sending wallet-wide. `TX_APPROVAL_PENDING_MESSAGE` is also untrue in that window: nothing is "already waiting to be approved" and there is nothing for the user to answer. Acceptable: the take is atomic because nothing awaits between test and set, not because of where it sits in the function — move it after the authorization and `from` checks, immediately before `prepareApprovalTx()`.
Verified and passing: `make check` green here (28 suites, 689 tests, `script/verify-build` 18 cases, prettier clean); the seven new/changed tests are load-bearing — reverting `src/` to `9dcd875` with the tests kept gives 7 failed / 682 passed, matching the PR body; `replacement transaction underpriced` is correctly a collision (the node refused that artifact) and `already known` correctly is not; verification is untouched and the nonce check sits after `verifySignedTx()`, so [#216](https://git.eeqj.de/sneak/AutistMask/issues/216) is not weakened; merges cleanly onto current `next` (`0be20d7`), no conflict with [#282](https://git.eeqj.de/sneak/AutistMask/pulls/282) today; commit title, `TODO.md` in the same commit, terminology and formatting all fine; no attribution trailers.
Disclosures: the head commit's CI check is still "Waiting to run", so CI green is unconfirmed — the local `make check` above is the evidence. Neither e2e suite was run (docker browser suites, not part of `check`). The disclosed popup-Send gap is waived against the issue's definition of done: the Send screen is not an `eth_sendTransaction` request, and finding 1 is the part of it that matters.
Reworked, head 4fafa21, rebased on next at d9d50f0 (next moved twice during this rework; make check was re-run green after each rebase, and the PR merges cleanly).
1. Broadcast-nonce record keyed by address only. Keyed by chain id plus address now (broadcastNoncesFor(chainId, address)). The chain is read once into a local right after loadState(), and that same value is used for verifySignedTx(), for the spent-nonce check and for the record write, so a network switch part-way through cannot make the check and the record disagree.
2. Slot released only on promise resolution. The slot is now a handle bound to the approval id, and it is freed inside settleApproval() — the single chokepoint every approval leaves pendingApprovals through — so every path that retires an approval releases the hold. The holder's finally remains as the backstop for the interval before an approval exists. The two paths that stranded an approval are closed rather than only leaking a slot:
Window closed under a claimed attempt: settleApproval() still declines (the attempt owns it and may broadcast), but the approval is marked as having lost its window. If that attempt then fails retryably, releaseApproval() settles it as the 4001 the closed window already meant instead of leaving it standing with no window and no resolver. Deferring to that moment rather than freeing on window-close keeps the slot held while a broadcast is in flight, which is the interval it exists for.
windowsApi.create handing back no window: the approval is settled immediately with -32603 and a full sentence saying nothing was shown, rather than holding the page's promise open forever. An approval already settled while its window was opening now has that window closed instead of throwing on pendingApprovals[id].windowId.
3. Slot taken before the authorization check. Moved into handleSendTransaction(), after the connection and from checks, immediately before prepareApprovalTx(). An unconnected origin gets 4100 without touching the slot. The take is still atomic — nothing awaits between its test and its set.
The refusal copy changed with it, because it was untrue for the interval between the take and the window: now "AutistMask handles one transaction at a time, and another one is already in progress, so this one was not sent. Please finish that transaction, then send this one again." Full sentences, true whether the other request is being populated or on screen.
Discrimination. Four new tests plus the reworded existing one, run against the reviewed head 73db8ee with src/ reverted and the tests kept: 5 failed / 28 passed in tests/backgroundApproval.test.js.
a nonce spent on one chain is not refused on another — broadcastTransaction calls: expected 2, received 1 (finding 1).
an approval whose window closed under a failed attempt is answered, and frees the next request — expected {code: 4001}, received null (finding 2).
a request whose approval window cannot be opened is answered rather than left waiting — expected {code: -32603}, received null (finding 2).
a request the wallet refuses does not take the slot from the connected site — the connected site's own request came back {code: -32002, ...} instead of raising an approval (finding 3).
a second eth_sendTransaction while one is pending is refused before it takes a nonce — old copy (finding 3's second half).
Against next's source (9dcd875) with all tests kept: 9 failed / 684 passed.
Verification.make check green on the pushed head: 29 suites, 715 tests, script/verify-build 18 cases, prettier clean. Also green containerized via script/cibuild — make check executed as build step 7/8 in 18.8s, not CACHED.
Not fixed, disclosed: an attempt whose broadcastTransaction never settles holds both the claim and the slot indefinitely. That is the pre-existing shape of attemptInFlight and is unchanged here; a hung broadcast is a case for a bounded send, not for this issue. Neither e2e suite was run (docker browser suites, not part of check). The popup Send screen remains outside the slot, as disclosed in the PR body.
Reworked, head `4fafa21`, rebased on `next` at `d9d50f0` (`next` moved twice during this rework; `make check` was re-run green after each rebase, and the PR merges cleanly).
**1. Broadcast-nonce record keyed by address only.** Keyed by chain id plus address now (`broadcastNoncesFor(chainId, address)`). The chain is read once into a local right after `loadState()`, and that same value is used for `verifySignedTx()`, for the spent-nonce check and for the record write, so a network switch part-way through cannot make the check and the record disagree.
**2. Slot released only on promise resolution.** The slot is now a handle bound to the approval id, and it is freed inside `settleApproval()` — the single chokepoint every approval leaves `pendingApprovals` through — so every path that retires an approval releases the hold. The holder's `finally` remains as the backstop for the interval before an approval exists. The two paths that stranded an approval are closed rather than only leaking a slot:
- Window closed under a claimed attempt: `settleApproval()` still declines (the attempt owns it and may broadcast), but the approval is marked as having lost its window. If that attempt then fails retryably, `releaseApproval()` settles it as the 4001 the closed window already meant instead of leaving it standing with no window and no resolver. Deferring to that moment rather than freeing on window-close keeps the slot held while a broadcast is in flight, which is the interval it exists for.
- `windowsApi.create` handing back no window: the approval is settled immediately with `-32603` and a full sentence saying nothing was shown, rather than holding the page's promise open forever. An approval already settled while its window was opening now has that window closed instead of throwing on `pendingApprovals[id].windowId`.
**3. Slot taken before the authorization check.** Moved into `handleSendTransaction()`, after the connection and `from` checks, immediately before `prepareApprovalTx()`. An unconnected origin gets `4100` without touching the slot. The take is still atomic — nothing awaits between its test and its set.
The refusal copy changed with it, because it was untrue for the interval between the take and the window: now "AutistMask handles one transaction at a time, and another one is already in progress, so this one was not sent. Please finish that transaction, then send this one again." Full sentences, true whether the other request is being populated or on screen.
**Discrimination.** Four new tests plus the reworded existing one, run against the reviewed head `73db8ee` with `src/` reverted and the tests kept: 5 failed / 28 passed in `tests/backgroundApproval.test.js`.
- `a nonce spent on one chain is not refused on another` — `broadcastTransaction` calls: expected 2, received 1 (finding 1).
- `an approval whose window closed under a failed attempt is answered, and frees the next request` — expected `{code: 4001}`, received `null` (finding 2).
- `a request whose approval window cannot be opened is answered rather than left waiting` — expected `{code: -32603}`, received `null` (finding 2).
- `a request the wallet refuses does not take the slot from the connected site` — the connected site's own request came back `{code: -32002, ...}` instead of raising an approval (finding 3).
- `a second eth_sendTransaction while one is pending is refused before it takes a nonce` — old copy (finding 3's second half).
Against `next`'s source (`9dcd875`) with all tests kept: 9 failed / 684 passed.
**Verification.** `make check` green on the pushed head: 29 suites, 715 tests, `script/verify-build` 18 cases, prettier clean. Also green containerized via `script/cibuild` — `make check` executed as build step 7/8 in 18.8s, not `CACHED`.
Not fixed, disclosed: an attempt whose `broadcastTransaction` never settles holds both the claim and the slot indefinitely. That is the pre-existing shape of `attemptInFlight` and is unchanged here; a hung broadcast is a case for a bounded send, not for this issue. Neither e2e suite was run (docker browser suites, not part of `check`). The popup Send screen remains outside the slot, as disclosed in the PR body.
FAIL — needs-rework. Re-review of 4fafa21 against #271. One blocker; the change itself is correct and the three earlier findings are genuinely closed.
1. Commit authorship. 4fafa21 is authored and committed as sneak <sneak@sneak.berlin>. Every commit on next is clawbot <clawbot@noreply.example.org>, and #186 records this exact recurrence twice today (#291, #286) as something being corrected in rework, not accepted. Identity inherited from the clone rather than set explicitly. It does not reach next — the squash rewrites it — but the branch commit misattributes machine-written wallet code to the owner, which is the thing #186 exists to stop. Acceptable: git -c user.name=clawbot -c user.email=clawbot@noreply.example.org commit --amend --reset-author and force-push. Nothing else needs to change.
Notes, not blocking:
src/background/index.js:263 — releaseTxApprovalSlotFor(id) inside settleApproval() is inert. Deleting that line leaves all 715 tests passing, because settling resolves the holder's promise and the pre-existing finally at :881 frees the slot on every reachable path. The chokepoint free is sound and harmless, but it is not what closed the leak and no test covers it. What actually closed it is the windowClosed settle at :324 and the !win settle at :378, and those two do discriminate.
src/background/index.js:1255-1270 — the spent-nonce record also refuses a deliberate same-nonce replacement (a dApp fee-bump or cancel; nonce is an accepted request field in approvalTx.js and populateTransaction() honours it), which a correctly-priced node would have taken. The copy then loops: "Please send it again from the site" produces the identical refusal until the worker restarts. Bounded by worker lifetime and a consequence of the mechanism exactly as planned on the issue, so raised rather than filed as a defect — but it is undocumented.
README.md:1180 and TODO.md:63 call -32002 an EIP-1193 code. EIP-1193 defines 4001/4100/4200/4900/4901; -32002 is EIP-1474 "resource unavailable", which the code comment at src/background/index.js:92 gets right.
Verified and passing: make check green in a fresh clone (29 suites, 715 tests, script/verify-build 18 cases, prettier clean); CI green on 4fafa21; mergeable onto next, no conflict; single commit, title ends (closes #271), TODO.md in it, both rebases' entries intact; copy is full sentences per RULES.md, and -32002/-32603/4001 all reach the page verbatim through inpage.js; no attribution trailers. Slot lifecycle walked end to end — no double free (the handle identity guard at :118 and the id match at :126 both hold across the settle/finally microtask gap), no new leak, and the orphaned-window path closed.
Discrimination re-derived independently by mutation, because 73db8ee is no longer on the remote: src/ reverted to next gives 9 failed / 706 passed; removing the windowClosed settle gives the window-closed test expected {code: 4001}, received null; taking the slot above the authorization checks gives the connected site {code: -32002} where null is expected; dropping the chain from the nonce key fails the cross-chain test alone.
Disclosures: neither e2e suite was run — #287 and #290 record load-sensitive flake, and the unit coverage is at the right level for this change. Discrimination required mutating src/ in a throwaway clone; nothing was committed or pushed.
FAIL — `needs-rework`. Re-review of `4fafa21` against [#271](https://git.eeqj.de/sneak/AutistMask/issues/271). One blocker; the change itself is correct and the three earlier findings are genuinely closed.
**1. Commit authorship. `4fafa21` is authored and committed as `sneak <sneak@sneak.berlin>`.** Every commit on `next` is `clawbot <clawbot@noreply.example.org>`, and [#186](https://git.eeqj.de/sneak/AutistMask/issues/186) records this exact recurrence twice today ([#291](https://git.eeqj.de/sneak/AutistMask/pulls/291), [#286](https://git.eeqj.de/sneak/AutistMask/pulls/286)) as something being corrected in rework, not accepted. Identity inherited from the clone rather than set explicitly. It does not reach `next` — the squash rewrites it — but the branch commit misattributes machine-written wallet code to the owner, which is the thing [#186](https://git.eeqj.de/sneak/AutistMask/issues/186) exists to stop. Acceptable: `git -c user.name=clawbot -c user.email=clawbot@noreply.example.org commit --amend --reset-author` and force-push. Nothing else needs to change.
Notes, not blocking:
- `src/background/index.js:263` — `releaseTxApprovalSlotFor(id)` inside `settleApproval()` is inert. Deleting that line leaves all 715 tests passing, because settling resolves the holder's promise and the pre-existing `finally` at `:881` frees the slot on every reachable path. The chokepoint free is sound and harmless, but it is not what closed the leak and no test covers it. What actually closed it is the `windowClosed` settle at `:324` and the `!win` settle at `:378`, and those two do discriminate.
- `src/background/index.js:1255-1270` — the spent-nonce record also refuses a deliberate same-nonce replacement (a dApp fee-bump or cancel; `nonce` is an accepted request field in `approvalTx.js` and `populateTransaction()` honours it), which a correctly-priced node would have taken. The copy then loops: "Please send it again from the site" produces the identical refusal until the worker restarts. Bounded by worker lifetime and a consequence of the mechanism exactly as planned on the issue, so raised rather than filed as a defect — but it is undocumented.
- `README.md:1180` and `TODO.md:63` call `-32002` an EIP-1193 code. EIP-1193 defines 4001/4100/4200/4900/4901; `-32002` is EIP-1474 "resource unavailable", which the code comment at `src/background/index.js:92` gets right.
Verified and passing: `make check` green in a fresh clone (29 suites, 715 tests, `script/verify-build` 18 cases, prettier clean); CI green on `4fafa21`; mergeable onto `next`, no conflict; single commit, title ends ` (closes #271)`, `TODO.md` in it, both rebases' entries intact; copy is full sentences per `RULES.md`, and `-32002`/`-32603`/`4001` all reach the page verbatim through `inpage.js`; no attribution trailers. Slot lifecycle walked end to end — no double free (the handle identity guard at `:118` and the id match at `:126` both hold across the settle/finally microtask gap), no new leak, and the orphaned-window path closed.
Discrimination re-derived independently by mutation, because `73db8ee` is no longer on the remote: `src/` reverted to `next` gives 9 failed / 706 passed; removing the `windowClosed` settle gives the window-closed test expected `{code: 4001}`, received `null`; taking the slot above the authorization checks gives the connected site `{code: -32002}` where `null` is expected; dropping the chain from the nonce key fails the cross-chain test alone.
Disclosures: neither e2e suite was run — [#287](https://git.eeqj.de/sneak/AutistMask/issues/287) and [#290](https://git.eeqj.de/sneak/AutistMask/issues/290) record load-sensitive flake, and the unit coverage is at the right level for this change. Discrimination required mutating `src/` in a throwaway clone; nothing was committed or pushed.
PASS. The sole blocking finding was commit authorship; the owner has ruled that sneak identity is correct (#186), so it is withdrawn. The three non-blocking notes stand as recorded and are not held against this unit. Squash-merging.
PASS. The sole blocking finding was commit authorship; the owner has ruled that `sneak` identity is correct (https://git.eeqj.de/sneak/AutistMask/issues/186), so it is withdrawn. The three non-blocking notes stand as recorded and are not held against this unit. Squash-merging.
clawbot
merged commit c06765ef8f into next2026-08-17 08:38:26 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #271.
The decision, and why
Of the three options the issue named, this takes the third: refuse a second transaction approval while one is pending.
What changed
src/background/index.jshandleSendTransaction(), after the connection andfromchecks and immediately beforeprepareApprovalTx(): a page the wallet was never going to serve must not be able to hold the slot and make the connected site's own transaction fail. The take is atomic because nothing awaits between its test and its set, not because of where it sits, so two requests delivered in the same tick still cannot both pass it. The second request is refused with EIP-1193-32002(resource unavailable, the standard code for "already pending") and never reachesprepareApprovalTx(): no second nonce, no second window. Signature approvals are not gated, consuming no nonce.settleApproval()— the single point an approval leavespendingApprovals. Every path that retires an approval therefore ends the hold; the holder'sfinallyis the backstop for the interval before an approval exists, and frees nothing if another request has since taken the slot.windowsApi.createnever produced is settled at once with-32603, saying plainly that nothing was shown. An approval settled while its window was opening has that window closed instead of throwing.sendResponsenow reportsoutcome.stagerather than the stage it passed in, because a broadcast failure the node blamed on the nonce is reclassified.src/shared/approvalVerify.jsisNonceCollision()and a newTX_STAGE_NONCE. Classified from ethers'NONCE_EXPIRED/REPLACEMENT_UNDERPRICEDcodes and from the node's own words (nonce too low,nonce has already been used,invalid nonce,OldNonce,replacement transaction underpriced,replacement fee too low), including the nestedinfo.error.messageshape ethers hands up when it could not classify the error itself.already knownis deliberately not a collision: a node that says it knows the transaction has it, so it did reach the network and the existing ambiguous wording is the correct one for it.nonce too low) is replaced rather than passed through: it is not a sentence, and it says less than the wallet knows.Verification is untouched. The approval still carries the transaction the screen displayed, and the artifact is still compared against that object field for field.
The tests, failing first
Every test here was demonstrated failing against the tree it was written for and passing after the change.
Against
nextat9dcd875with all tests kept: 9 failed, 684 passed. The whole of the defect is visible in the first one — the second request's result isnull, because it had raised its own approval window at the same nonce the first one is holding, instead of being answered.The rework's own four tests, run against the pre-rework head
73db8eewithsrc/reverted and the tests kept — 5 failed, 28 passed intests/backgroundApproval.test.js:The fifth is the existing concurrency test, red on the old refusal copy.
One existing assertion changed rather than being added to: the terminal-broadcast test looped over four node messages asserting the wallet passes each through verbatim, and
replacement transaction underpricedis now one of the reclassified ones. It is still asserted terminal, in the new nonce test, with the new message.Verification
make check— green: 29 suites, 715 tests,script/verify-build18 cases,prettier --checkclean. Also green containerized throughscript/cibuild, withmake checkexecuting as build step 7/8 in 18.8s rather thanCACHED.make fmtrun; the branch is rebased onnextatd9d50f0.Not covered, and knowingly so: the popup's own Send screen populates at send time and is outside the approval record, so it is not gated by this slot. A nonce it takes under an open dApp approval still collides — and that collision now reports accurately through both paths above, which is the part of it this issue asked for. An approval the user never answers holds the slot until the window is closed or the worker restarts; that is the same interval in which its nonce is allocated and unspent, so it is the intended behaviour rather than a leak. An attempt whose broadcast never settles holds the claim and the slot indefinitely, which is the pre-existing shape of
attemptInFlightand unchanged here.FAIL —
needs-rework. Three defects, all insrc/background/index.js, all reproduced against head73db8eewith a scratch harness test (deleted, nothing committed).1.
src/background/index.js:117-124and:1130-1151— the broadcast-nonce record is keyed by address only, so a nonce spent on one chain blocks that nonce on every other chain.broadcastNoncesFor(address)has no chain component, while the wallet switches networks (wallet_switchEthereumChainat:502-510,currentNetwork().chainIdat:1101) and nonce spaces are per chain. Reproduced: broadcast at nonce 7 on chain 1, switch network, approve a transaction the node populated at nonce 7 on chain 11155111 →broadcastTransactionwas never called for it (call count stayed at 1) and both the page and the popup were told "The transaction was not sent, because its nonce had already been used by another transaction." That is false — no transaction of this user's has used that nonce on the new chain — and it is unrecoverable: the copy says "Please send it again from the site", the resend repopulates the same nonce, and it is refused identically for the life of the worker. Low nonces overlap across chains routinely, so this blocks ordinary multi-chain use with an inaccurate message, in the exact direction the issue was filed to stop. Acceptable: key the record by chain id plus address, record under the chain the broadcast actually went out on, and check only the chain that is current.2.
:674-681— the slot is released only when the approval promise resolves, and there are paths on which it never resolves; the wallet then refuses everyeth_sendTransactionfor the life of the worker.finally { releaseTxApprovalSlot(); }waits onrequestTxApproval(). An approval whose attempt is claimed, whose window is then closed (:934-950, wheresettleApproval()correctly declines whileattemptInFlight), and whose attempt then fails retryably (:1116releaseApproval(approval)— reached by any non-mismatch throw:loadState(),getActiveAddress(), an unparseable artifact) is left inpendingApprovalswith no window and no resolver. Reproduced: after that sequence the first request's result is stillnulland the nexteth_sendTransactionreturns-32002with zero windows opened, permanently.windowsApi.createhanding back no window at:278-282strands it the same way. Before this PR that sequence stranded one request; with the slot it denies the wallet's main function until the worker restarts. Acceptable: bind the slot to the approval id and free it whenever that approval leavespendingApprovalsor loses its window — including the window-closed path that currently declines to settle — rather than only on promise resolution.3.
:663-673— the slot is taken before the authorization check, so any web page can make the user's single legitimate transaction fail with "already pending".reserveTxApprovalSlot()is the first statement of the branch, ahead ofawait getState()/getActiveAddress()and the4100 Unauthorizedreturn insidehandleSendTransaction(). Reproduced: an unconnected origin'seth_sendTransaction(itself rejected4100) held the slot while the connected dApp's own single request was refused with-32002and no window opened. Any page can loop this and deny sending wallet-wide.TX_APPROVAL_PENDING_MESSAGEis also untrue in that window: nothing is "already waiting to be approved" and there is nothing for the user to answer. Acceptable: the take is atomic because nothing awaits between test and set, not because of where it sits in the function — move it after the authorization andfromchecks, immediately beforeprepareApprovalTx().Verified and passing:
make checkgreen here (28 suites, 689 tests,script/verify-build18 cases, prettier clean); the seven new/changed tests are load-bearing — revertingsrc/to9dcd875with the tests kept gives 7 failed / 682 passed, matching the PR body;replacement transaction underpricedis correctly a collision (the node refused that artifact) andalready knowncorrectly is not; verification is untouched and the nonce check sits afterverifySignedTx(), so #216 is not weakened; merges cleanly onto currentnext(0be20d7), no conflict with #282 today; commit title,TODO.mdin the same commit, terminology and formatting all fine; no attribution trailers.Disclosures: the head commit's CI check is still "Waiting to run", so CI green is unconfirmed — the local
make checkabove is the evidence. Neither e2e suite was run (docker browser suites, not part ofcheck). The disclosed popup-Send gap is waived against the issue's definition of done: the Send screen is not aneth_sendTransactionrequest, and finding 1 is the part of it that matters.73db8eee43tof0441a913bReworked, head
4fafa21, rebased onnextatd9d50f0(nextmoved twice during this rework;make checkwas re-run green after each rebase, and the PR merges cleanly).1. Broadcast-nonce record keyed by address only. Keyed by chain id plus address now (
broadcastNoncesFor(chainId, address)). The chain is read once into a local right afterloadState(), and that same value is used forverifySignedTx(), for the spent-nonce check and for the record write, so a network switch part-way through cannot make the check and the record disagree.2. Slot released only on promise resolution. The slot is now a handle bound to the approval id, and it is freed inside
settleApproval()— the single chokepoint every approval leavespendingApprovalsthrough — so every path that retires an approval releases the hold. The holder'sfinallyremains as the backstop for the interval before an approval exists. The two paths that stranded an approval are closed rather than only leaking a slot:settleApproval()still declines (the attempt owns it and may broadcast), but the approval is marked as having lost its window. If that attempt then fails retryably,releaseApproval()settles it as the 4001 the closed window already meant instead of leaving it standing with no window and no resolver. Deferring to that moment rather than freeing on window-close keeps the slot held while a broadcast is in flight, which is the interval it exists for.windowsApi.createhanding back no window: the approval is settled immediately with-32603and a full sentence saying nothing was shown, rather than holding the page's promise open forever. An approval already settled while its window was opening now has that window closed instead of throwing onpendingApprovals[id].windowId.3. Slot taken before the authorization check. Moved into
handleSendTransaction(), after the connection andfromchecks, immediately beforeprepareApprovalTx(). An unconnected origin gets4100without touching the slot. The take is still atomic — nothing awaits between its test and its set.The refusal copy changed with it, because it was untrue for the interval between the take and the window: now "AutistMask handles one transaction at a time, and another one is already in progress, so this one was not sent. Please finish that transaction, then send this one again." Full sentences, true whether the other request is being populated or on screen.
Discrimination. Four new tests plus the reworded existing one, run against the reviewed head
73db8eewithsrc/reverted and the tests kept: 5 failed / 28 passed intests/backgroundApproval.test.js.a nonce spent on one chain is not refused on another—broadcastTransactioncalls: expected 2, received 1 (finding 1).an approval whose window closed under a failed attempt is answered, and frees the next request— expected{code: 4001}, receivednull(finding 2).a request whose approval window cannot be opened is answered rather than left waiting— expected{code: -32603}, receivednull(finding 2).a request the wallet refuses does not take the slot from the connected site— the connected site's own request came back{code: -32002, ...}instead of raising an approval (finding 3).a second eth_sendTransaction while one is pending is refused before it takes a nonce— old copy (finding 3's second half).Against
next's source (9dcd875) with all tests kept: 9 failed / 684 passed.Verification.
make checkgreen on the pushed head: 29 suites, 715 tests,script/verify-build18 cases, prettier clean. Also green containerized viascript/cibuild—make checkexecuted as build step 7/8 in 18.8s, notCACHED.Not fixed, disclosed: an attempt whose
broadcastTransactionnever settles holds both the claim and the slot indefinitely. That is the pre-existing shape ofattemptInFlightand is unchanged here; a hung broadcast is a case for a bounded send, not for this issue. Neither e2e suite was run (docker browser suites, not part ofcheck). The popup Send screen remains outside the slot, as disclosed in the PR body.f0441a913bto4fafa21ccaFAIL —
needs-rework. Re-review of4fafa21against #271. One blocker; the change itself is correct and the three earlier findings are genuinely closed.1. Commit authorship.
4fafa21is authored and committed assneak <sneak@sneak.berlin>. Every commit onnextisclawbot <clawbot@noreply.example.org>, and #186 records this exact recurrence twice today (#291, #286) as something being corrected in rework, not accepted. Identity inherited from the clone rather than set explicitly. It does not reachnext— the squash rewrites it — but the branch commit misattributes machine-written wallet code to the owner, which is the thing #186 exists to stop. Acceptable:git -c user.name=clawbot -c user.email=clawbot@noreply.example.org commit --amend --reset-authorand force-push. Nothing else needs to change.Notes, not blocking:
src/background/index.js:263—releaseTxApprovalSlotFor(id)insidesettleApproval()is inert. Deleting that line leaves all 715 tests passing, because settling resolves the holder's promise and the pre-existingfinallyat:881frees the slot on every reachable path. The chokepoint free is sound and harmless, but it is not what closed the leak and no test covers it. What actually closed it is thewindowClosedsettle at:324and the!winsettle at:378, and those two do discriminate.src/background/index.js:1255-1270— the spent-nonce record also refuses a deliberate same-nonce replacement (a dApp fee-bump or cancel;nonceis an accepted request field inapprovalTx.jsandpopulateTransaction()honours it), which a correctly-priced node would have taken. The copy then loops: "Please send it again from the site" produces the identical refusal until the worker restarts. Bounded by worker lifetime and a consequence of the mechanism exactly as planned on the issue, so raised rather than filed as a defect — but it is undocumented.README.md:1180andTODO.md:63call-32002an EIP-1193 code. EIP-1193 defines 4001/4100/4200/4900/4901;-32002is EIP-1474 "resource unavailable", which the code comment atsrc/background/index.js:92gets right.Verified and passing:
make checkgreen in a fresh clone (29 suites, 715 tests,script/verify-build18 cases, prettier clean); CI green on4fafa21; mergeable ontonext, no conflict; single commit, title ends(closes #271),TODO.mdin it, both rebases' entries intact; copy is full sentences perRULES.md, and-32002/-32603/4001all reach the page verbatim throughinpage.js; no attribution trailers. Slot lifecycle walked end to end — no double free (the handle identity guard at:118and the id match at:126both hold across the settle/finally microtask gap), no new leak, and the orphaned-window path closed.Discrimination re-derived independently by mutation, because
73db8eeis no longer on the remote:src/reverted tonextgives 9 failed / 706 passed; removing thewindowClosedsettle gives the window-closed test expected{code: 4001}, receivednull; taking the slot above the authorization checks gives the connected site{code: -32002}wherenullis expected; dropping the chain from the nonce key fails the cross-chain test alone.Disclosures: neither e2e suite was run — #287 and #290 record load-sensitive flake, and the unit coverage is at the right level for this change. Discrimination required mutating
src/in a throwaway clone; nothing was committed or pushed.PASS. The sole blocking finding was commit authorship; the owner has ruled that
sneakidentity is correct (#186), so it is withdrawn. The three non-blocking notes stand as recorded and are not held against this unit. Squash-merging.