sneak/AutistMask
1
0
Fork 0
Code Issues 42 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

test: ConfirmTx has no automated coverage at all — the screen that decides what gets signed #238

New Issue
Closed
opened 2026-08-11 15:36:18 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-11 15:36:18 +02:00
Collaborator
Copy Link

src/popup/views/confirmTx.js has no unit tests, and make test-e2e never reaches the confirmation screen. So the screen where the user decides whether to sign a transaction has zero automated coverage of its own behaviour.

Established during the third review of #197, which had to verify the change by reading the code and by driving a browser by hand. The reviewer's exact observation: a mutant that made the spend gate read the DISPLAY estimate instead of the reserve — reintroducing #154 in full — would NOT be caught by the test suite. It is correct today by reading, not by test.

The pure arithmetic underneath is well covered: src/shared/txValidation.js has 29 tests and seven mutants that all die. The gap is the wiring — which value is passed to the gate, when validation re-runs, what the fee block renders, and whether the Send button is enabled.

Same class as #229 (Settings has no browser coverage), and worse in consequence: Settings breaking is visible and annoying, ConfirmTx being wrong is invisible and expensive.

Implementation requirements

  • Extend the e2e harness to reach ConfirmTx. This needs a funded-balance fixture, which is why it has not been done — the RPC is stubbed by route interception, so a balance and a gas estimate can be stubbed too.
  • Cover, at minimum: the Send button disabled while the fee estimate is pending; enabled for a comfortably-funded send; disabled with the correct distinct message when the estimate fails; disabled for a send that exceeds balance-plus-reserve; and the fee block rendering both the estimate and the reserve.
  • Assert the GATE uses the reserve, not the estimate. A test that would still pass if those two were swapped does not close this gap.
  • Cover the ERC-20 path as well as native ETH — the two use different balances and a different error.
  • No layout shift when the estimate lands or fails; the reviewer measured a constant 874px view height by hand, which is exactly the assertion that should be automated.

Definition of done

  • make test-e2e drives ConfirmTx for both the ETH and ERC-20 paths.
  • Swapping the reserve for the estimate in the gate makes the suite fail — demonstrated, with captured output.
  • The pending, funded, over-balance and estimate-failed states are each asserted.
  • View height is asserted constant across those state transitions.
  • TODO.md updated in the same commit.
  • make check passes.
`src/popup/views/confirmTx.js` has no unit tests, and `make test-e2e` never reaches the confirmation screen. So the screen where the user decides whether to sign a transaction has **zero** automated coverage of its own behaviour. Established during the third review of https://git.eeqj.de/sneak/AutistMask/pulls/197, which had to verify the change by reading the code and by driving a browser by hand. The reviewer's exact observation: a mutant that made the spend gate read the DISPLAY estimate instead of the reserve — reintroducing https://git.eeqj.de/sneak/AutistMask/issues/154 in full — would NOT be caught by the test suite. It is correct today by reading, not by test. The pure arithmetic underneath is well covered: `src/shared/txValidation.js` has 29 tests and seven mutants that all die. The gap is the wiring — which value is passed to the gate, when validation re-runs, what the fee block renders, and whether the Send button is enabled. Same class as https://git.eeqj.de/sneak/AutistMask/issues/229 (Settings has no browser coverage), and worse in consequence: Settings breaking is visible and annoying, ConfirmTx being wrong is invisible and expensive. ## Implementation requirements - Extend the e2e harness to reach ConfirmTx. This needs a funded-balance fixture, which is why it has not been done — the RPC is stubbed by route interception, so a balance and a gas estimate can be stubbed too. - Cover, at minimum: the Send button disabled while the fee estimate is pending; enabled for a comfortably-funded send; disabled with the correct distinct message when the estimate fails; disabled for a send that exceeds balance-plus-reserve; and the fee block rendering both the estimate and the reserve. - Assert the GATE uses the reserve, not the estimate. A test that would still pass if those two were swapped does not close this gap. - Cover the ERC-20 path as well as native ETH — the two use different balances and a different error. - No layout shift when the estimate lands or fails; the reviewer measured a constant 874px view height by hand, which is exactly the assertion that should be automated. ## Definition of done - [ ] `make test-e2e` drives ConfirmTx for both the ETH and ERC-20 paths. - [ ] Swapping the reserve for the estimate in the gate makes the suite fail — demonstrated, with captured output. - [ ] The pending, funded, over-balance and estimate-failed states are each asserted. - [ ] View height is asserted constant across those state transitions. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.
clawbot added this to the 1.0.0 milestone 2026-08-11 15:36:18 +02:00
clawbot commented 2026-08-12 10:54:16 +02:00
Author
Collaborator
Copy Link

Done in #258, base next.

The e2e harness gained a funded-balance fixture (ethBalanceWei,
seedTokenBalance), a latest block carrying baseFeePerGas — without it
ethers has no maxFeePerGas and the reserve and the estimate collapse into one
number, which would make the gate tests vacuous — decimals() for the stub
token on eth_call, a refusable eth_estimateGas (failGasEstimate), and a
holdable one (holdGasEstimate) so the pending state is observed rather than
raced. Nine tests over both the ETH and the ERC-20 path: pending, funded,
over-balance, estimate-failed, both fee numbers rendered, and view height
constant across every transition (1022.5625px ETH, 1070.5625px ERC-20, printed
on every run).

The gate assertion is demonstrated, not inspected. Changing feeWei = gasCostWei to feeWei = estimateWei in estimateGas() — the mutant from the
third review of #197 — turns
the suite red:

# gate probe: balance=1.0 ETH amount=0.997879 estimate=0.002121 ETH reserve=0.004221 ETH sendDisabled=false amountFeeError=false
not ok 17 - ConfirmTx gates on the fee RESERVE, not the displayed estimate (#238)
  Send is ENABLED for a transfer the fee RESERVE does not cover — the spend gate is reading the displayed estimate, which is issue #154
# erc-20 gate probe: ethBalance=0.002121 estimate=0.002121 ETH reserve=0.004221 ETH sendDisabled=false gasError=false
not ok 22 - ConfirmTx gates the ERC-20 fee on the RESERVE, with the ERC-20 message (#238)
  Send is ENABLED for an ERC-20 transfer whose fee RESERVE exceeds the ETH balance — the spend gate is reading the displayed estimate (#154)
# 21/23 tests passed
# FAILED

The mutation was reverted; the branch touches nothing under src/.

Verified: make check green (19 suites, 416 tests, prettier clean),
make test-e2e green at 23/23, both re-run after rebasing onto next at
bd4bdca.

Done in [#258](https://git.eeqj.de/sneak/AutistMask/pulls/258), base `next`. The e2e harness gained a funded-balance fixture (`ethBalanceWei`, `seedTokenBalance`), a `latest` block carrying `baseFeePerGas` — without it ethers has no `maxFeePerGas` and the reserve and the estimate collapse into one number, which would make the gate tests vacuous — `decimals()` for the stub token on `eth_call`, a refusable `eth_estimateGas` (`failGasEstimate`), and a holdable one (`holdGasEstimate`) so the pending state is observed rather than raced. Nine tests over both the ETH and the ERC-20 path: pending, funded, over-balance, estimate-failed, both fee numbers rendered, and view height constant across every transition (1022.5625px ETH, 1070.5625px ERC-20, printed on every run). The gate assertion is demonstrated, not inspected. Changing `feeWei = gasCostWei` to `feeWei = estimateWei` in `estimateGas()` — the mutant from the third review of [#197](https://git.eeqj.de/sneak/AutistMask/pulls/197) — turns the suite red: ``` # gate probe: balance=1.0 ETH amount=0.997879 estimate=0.002121 ETH reserve=0.004221 ETH sendDisabled=false amountFeeError=false not ok 17 - ConfirmTx gates on the fee RESERVE, not the displayed estimate (#238) Send is ENABLED for a transfer the fee RESERVE does not cover — the spend gate is reading the displayed estimate, which is issue #154 # erc-20 gate probe: ethBalance=0.002121 estimate=0.002121 ETH reserve=0.004221 ETH sendDisabled=false gasError=false not ok 22 - ConfirmTx gates the ERC-20 fee on the RESERVE, with the ERC-20 message (#238) Send is ENABLED for an ERC-20 transfer whose fee RESERVE exceeds the ETH balance — the spend gate is reading the displayed estimate (#154) # 21/23 tests passed # FAILED ``` The mutation was reverted; the branch touches nothing under `src/`. Verified: `make check` green (19 suites, 416 tests, prettier clean), `make test-e2e` green at 23/23, both re-run after rebasing onto `next` at `bd4bdca`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#238
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?