fix: every plain ERC-20 transfer produces two rows in transaction history #177

New Issue

clawbot · 2026-08-09T07:08:12+02:00

clawbot commented

2026-08-09 07:08:12 +02:00

Problem

Found while writing the first test coverage for the transaction filters
(#160 / PR #175), and deliberately not fixed there.

A plain ERC-20 transfer shows up twice in the transaction history: once as
the real token transfer row, and once as a spurious zero-value native row.

Mechanism:

src/shared/transactions.js:35 sets method === "transfer" for the native
side of an ERC-20 transfer.
The merge loop at src/shared/transactions.js:163-190 deduplicates a native
row against its token row by testing existing.direction === "contract".
Because method is "transfer" rather than a contract call, that test
fails, so the native row is never absorbed and survives alongside the token
row.
The survivor then escapes the dust filter too: transactions.js:253-260
skips dust filtering when isContractCall is true, so a zero-value row that
would otherwise be filtered as dust is displayed.

This is user-visible on the Home, AddressDetail and AddressToken transaction
lists, and it affects the single most common operation in the wallet after
sending ETH. It also undermines the anti-poisoning work: a history padded with
spurious zero-value rows is harder to scan, and scannability is the whole
reason the README allows truncation in list views.

Implementation requirements

Fix the dedup condition so the native side of an ERC-20 transfer is absorbed
into its token row. Prefer matching on the transaction hash pairing rather
than widening the direction/method string comparison — string-matching on
method is what produced this bug, and adding another special case invites
the next one.
Check the neighbouring cases before choosing the condition, so the fix is not
narrowly tailored to transfer: ERC-20 approve, a swap through the
Universal Router, a contract creation, and a self-send. Each should produce
the row count a user would expect. Say in the PR which you checked and what
each produces before and after.
Do not simply suppress all zero-value native rows — a genuine zero-value
native transaction is a real thing and should still be visible.
Re-check the dust-filter interaction at transactions.js:253-260 once the
dedup is fixed. If the spurious row is gone, the isContractCall exemption
may no longer be doing what it was written for; do not change it without
saying why.
Keep filterTransactions behaviour otherwise identical — the four
anti-poisoning filters are now covered by tests and must stay green.

Test coverage already waiting

tests/transactions.test.js (from PR #175) contains a test named with a
current behaviour: prefix pinning the duplicate-row output. Invert it rather
than deleting it, so it becomes the regression guard. The existing merge/dedup
tests must continue to pass unmodified.

Definition of done

A plain ERC-20 transfer produces exactly one row in the merged history.
approve, a Universal Router swap, a contract creation, and a self-send
each produce the expected row count; the PR states what each was before
and after.
A genuine zero-value native transaction is still displayed.
The current behaviour: test is inverted into a regression guard.
All existing tests in tests/transactions.test.js still pass unmodified.
src/shared/transactions.js is the only source file changed, or the PR
explains why not.
TODO.md updated in the same commit.
make check passes.

## Problem Found while writing the first test coverage for the transaction filters (#160 / PR #175), and deliberately not fixed there. A plain ERC-20 transfer shows up **twice** in the transaction history: once as the real token transfer row, and once as a spurious zero-value native row. Mechanism: - `src/shared/transactions.js:35` sets `method === "transfer"` for the native side of an ERC-20 transfer. - The merge loop at `src/shared/transactions.js:163-190` deduplicates a native row against its token row by testing `existing.direction === "contract"`. Because `method` is `"transfer"` rather than a contract call, that test fails, so the native row is never absorbed and survives alongside the token row. - The survivor then escapes the dust filter too: `transactions.js:253-260` skips dust filtering when `isContractCall` is true, so a zero-value row that would otherwise be filtered as dust is displayed. This is user-visible on the Home, AddressDetail and AddressToken transaction lists, and it affects the single most common operation in the wallet after sending ETH. It also undermines the anti-poisoning work: a history padded with spurious zero-value rows is harder to scan, and scannability is the whole reason the README allows truncation in list views. ## Implementation requirements - Fix the dedup condition so the native side of an ERC-20 transfer is absorbed into its token row. Prefer matching on the transaction hash pairing rather than widening the `direction`/`method` string comparison — string-matching on `method` is what produced this bug, and adding another special case invites the next one. - Check the neighbouring cases before choosing the condition, so the fix is not narrowly tailored to `transfer`: ERC-20 `approve`, a swap through the Universal Router, a contract creation, and a self-send. Each should produce the row count a user would expect. Say in the PR which you checked and what each produces before and after. - Do not simply suppress all zero-value native rows — a genuine zero-value native transaction is a real thing and should still be visible. - Re-check the dust-filter interaction at `transactions.js:253-260` once the dedup is fixed. If the spurious row is gone, the `isContractCall` exemption may no longer be doing what it was written for; do not change it without saying why. - Keep `filterTransactions` behaviour otherwise identical — the four anti-poisoning filters are now covered by tests and must stay green. ## Test coverage already waiting `tests/transactions.test.js` (from PR #175) contains a test named with a `current behaviour:` prefix pinning the duplicate-row output. Invert it rather than deleting it, so it becomes the regression guard. The existing merge/dedup tests must continue to pass unmodified. ## Definition of done - [ ] A plain ERC-20 transfer produces exactly one row in the merged history. - [ ] `approve`, a Universal Router swap, a contract creation, and a self-send each produce the expected row count; the PR states what each was before and after. - [ ] A genuine zero-value native transaction is still displayed. - [ ] The `current behaviour:` test is inverted into a regression guard. - [ ] All existing tests in `tests/transactions.test.js` still pass unmodified. - [ ] `src/shared/transactions.js` is the only source file changed, or the PR explains why not. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.

clawbot added this to the 1.0.0 milestone 2026-08-09 07:08:12 +02:00

clawbot referenced this issue

2026-08-09 07:17:29 +02:00

test: cover the address-poisoning filters in transactions.js (closes #160) #175

clawbot commented

2026-08-09 07:17:49 +02:00

Additional requirement, from the mutation testing done during the review of
PR #175.

The reviewer applied 15 mutations to src/shared/transactions.js and 13 were
killed by the new test suite. One of the two survivors is in exactly the code
this issue changes:

src/shared/transactions.js:173 — narrowing isReceived || needsAmount to
isReceived survives undetected. No fixture covers a swap whose token legs
are all sent, so the needsAmount fallback that supplies the display amount
in that case is currently unpinned. Someone could delete that condition and
every test would still pass.

That fallback lives in the same merge loop (transactions.js:163-190) whose
dedup condition this issue fixes, so please close the gap while you are in
there:

Add a fixture for a swap whose token legs are all sent, and assert the
merged entry gets its display amount from the needsAmount path.
Verify the mutation is now caught: narrow the condition to isReceived
alone, confirm at least one test fails, then revert. State the result in the
PR, the same way PR #175 did.

This matters for the fix itself, not just for coverage. This issue asks you to
change how the merge loop decides to absorb a native row into its token row,
and an unpinned amount-selection branch a few lines away is precisely the thing
a dedup change can break silently. Better to have it under test before you
touch the loop than after.

Additional requirement, from the mutation testing done during the review of PR #175. The reviewer applied 15 mutations to `src/shared/transactions.js` and 13 were killed by the new test suite. One of the two survivors is in exactly the code this issue changes: **`src/shared/transactions.js:173` — narrowing `isReceived || needsAmount` to `isReceived` survives undetected.** No fixture covers a swap whose token legs are all `sent`, so the `needsAmount` fallback that supplies the display amount in that case is currently unpinned. Someone could delete that condition and every test would still pass. That fallback lives in the same merge loop (`transactions.js:163-190`) whose dedup condition this issue fixes, so please close the gap while you are in there: - Add a fixture for a swap whose token legs are all `sent`, and assert the merged entry gets its display amount from the `needsAmount` path. - Verify the mutation is now caught: narrow the condition to `isReceived` alone, confirm at least one test fails, then revert. State the result in the PR, the same way PR #175 did. This matters for the fix itself, not just for coverage. This issue asks you to change how the merge loop decides to absorb a native row into its token row, and an unpinned amount-selection branch a few lines away is precisely the thing a dedup change can break silently. Better to have it under test before you touch the loop than after.

clawbot referenced this issue from a commit

2026-08-11 14:20:11 +02:00

fix: one transaction history row per value movement (closes #177)

clawbot referenced a pull request that will close this issue

2026-08-11 14:20:46 +02:00

fix: one transaction history row per value movement (closes #177) #196

clawbot referenced this issue

2026-08-11 14:27:45 +02:00

fix: one transaction history row per value movement (closes #177) #196

clawbot referenced this issue from a commit

2026-08-11 14:32:24 +02:00

fix: one transaction history row per value movement (closes #177)