Skip to content

Pass get_nfts the address its checks approved - #121

Merged
portdeveloper merged 1 commit into
portdeveloper:mainfrom
BeeHiveTeam:fix/nfts-owner-coercion
Oct 5, 2026
Merged

portdeveloper merged 1 commit into
portdeveloper:mainfrom
BeeHiveTeam:fix/nfts-owner-coercion

Conversation

@BeeHiveTeam

@BeeHiveTeam BeeHiveTeam commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #119.

ca31f86 closed #111 on both read paths. It held on one of them and not the other.

get_token_balance wraps resolveToken in try/catch, so anything ethers dislikes becomes a refusal whatever shape it arrived in. get_nfts checked one value and used another:

const owner = a.address ?? a.owner;
const trimmed = String(owner).trim();   // checked
await wallet.getNfts(owner);            // used

Measured on 8e14e1e through runAction, with no wallet and no network, and a plain address as the control:

address from the model before after
"0x9768…57E" (control) reaches the indexer unchanged
" 0x9768…57E " padded TypeError: invalid address (value=" 0x9768…") reaches the indexer, trimmed
["0x9768…57E"] TypeError (value=[ "0x…" ]) reaches the indexer
{ toString: () => "0x9768…57E" } TypeError reaches the indexer
"", 0, false Wallet not initialized Refused: "…" is not a valid address.
{ toString: null } TypeError: Cannot convert object to primitive value Refused: address was given but could not be read as text.
omitted, or {address: null} active account unchanged
{owner: null} Wallet not initialized active account

As #111 put it, handleAction returned "error: …" rather than "Refused: …", so isRefusal() read false for what is a rejection and a scripted run exited 0 on it.

The change, against the four points in the issue

Normalize once, pass what was validated. The coerced string is assigned back, so the wallet receives the value the checks approved. That is what the next case already does for fromAddress.

Keep the omitted-owner default — including through the alias. This is the last row of the table and it is the one I got wrong first. {address: null} already arrives as undefined, because ?? skips a null left side. {owner: null} does not: the alias is the right side, so the value stays null, the wallet's ownerAddress = address default never fires — it fires only for undefined — and an open wallet is reported as uninitialised. ?? undefined makes omitted and null-via-alias mean the same thing, which is what they are.

An explicitly supplied unusable value refuses before the wallet is called. Three groups:

  • not an address at all — refused as before;
  • falsy but present. "", 0 and false skipped the checks entirely and still reached the wallet. The same case records that trap for an empty fromAddress, one line above the rule it states;
  • unreadable. String({ toString: null }) throws, and {"toString": null} is JSON a model can emit. Given and unreadable is still given and unusable.

The issue proposed leaving that last one out, as a gap shared with get_token_balance. Your approval reads wider, so it is in. It remains open on the token path: get_token_balance still throws on { toString: null }, from isNativeToken rather than from here. That deserves its own issue and I have not touched it.

Two details of the refusal, each of which could have gone the other way. The message names the step that failed rather than reporting an empty address: falling back to "" would also refuse, but it would say the address was empty when it was not. And the value is not echoed, because reading it as text is exactly what failed.

Cover the distinction through runAction. Thirteen tests do — and one of them needed a seam, which is the next section.

Padding is trimmed, not refused. resolveRecipient trims and carries the trimmed value, and a read has nothing to sign, so there is no approved string to keep byte-for-byte. The opposite choice belongs to transfer_nft, where the string does get signed — and the comment there claimed it was following the recipient's rule, which it is not. Corrected while nearby, since citing it as precedent is the mistake this change would otherwise repeat.

Why getNfts is injectable here

Whether the owner default fires is not observable from outside the call. With no wallet, getNfts(undefined) resolves its default to a null module address and getNfts(null) keeps the null — both answer Wallet not initialized. A test on that message therefore holds whether the default fires or not, which is how the {owner: null} row above survived my first version of this change: the fix was a one-line edit and the suite stayed green either way.

opts.getNfts is the same seam opts.sendToken and opts.transferNft already are, with the same default — the real wallet call — and no production caller passes it. It lets the test read the argument, which is the only thing that separates a working default from a broken one.

Coverage

565 tests, up from 552. Thirteen new, in five blocks.

The usable shapes are asserted against a control: with no indexer key configured, a value that passed the checks and reached the wallet fails on the missing key and nowhere earlier. A converter error there means the value that travelled was not the value that was checked — the defect itself, stated as an assertion.

The four no-usable-owner shapes assert that getNfts is called with undefined, not that the error message looks a certain way. The falsy-but-present shapes assert a Refused: line and the absence of Wallet not initialized. The unreadable value has its own test, which requires could not be read as text and forbids is not a valid address.

Mutations

Eleven mutations, each reverted on its own with the full suite run each time, plus a no-op control:

reverted tests red
control — the presence check rewritten by De Morgan, same meaning 0 (565 pass)
?? undefined dropped, so null-via-alias reaches the wallet 2
the seam ignored, so the real wallet call is always used 5
the validated value not passed to the wallet 6
presence check replaced by truthiness 3
null treated as a present value 2
the trim dropped 2
the unreadable-value refusal removed, so the coercion throws again 1
the unreadable value falling back to "" instead of refusing 1
the isAddress check removed 1
the checksum refusal removed 1

Three of those earned a change rather than confirming one.

?? undefined is in the list because reverting it reddens exactly two tests — the alias cases — and leaves the other two green. Before the seam existed, reverting it reddened nothing.

null as a present value survived an earlier run, and that was not a coverage gap in the ordinary sense: both behaviours are defensible and nothing had decided between them. It is red now because a test states the decision.

The "" fallback survived until the refusal message was pinned. It still produced a refusal, so a test that only checked isRefusal() passed — while the operator was told the address was empty when it was unreadable.

The control is also worth a line. It was a rename of the local in four places, and a partial application made it report 19 red — a broken mutant dressed as a finding. It is now a De Morgan rewrite, which cannot be applied halfway.

Gates

npm run build clean; suite run three times at 565/565. Two files, +160 −13: the get_nfts case, the getNfts seam, one inaccurate sentence in the transfer_nft comment re-wrapped to its neighbours' width, and the tests.

Environment

  • branch: main @ 8e14e1e
  • OS: Linux x64, node v22
  • Gas mode: n/a — the repro and the new tests run through runAction with no wallet and no network

`ca31f86` closed portdeveloper#111 on both read paths. It held on `get_token_balance`, which wraps
`resolveToken` in try/catch, so anything ethers dislikes becomes a refusal whatever
shape it arrived in. It did not hold on `get_nfts`, which checked one value and used
another:

    const owner = a.address ?? a.owner;
    const trimmed = String(owner).trim();   // checked
    await wallet.getNfts(owner);            // used

`String(["0x…"])` and `String({ toString })` both read as a valid address, so the
checks passed and the original travelled on as an array or an object; a padded string
travelled with its padding. All three reached ethers and threw, where
`get_token_balance` answers the same three exactly as it answers a plain address.
`handleAction` then prints `error: …` rather than `Refused: …`, so `isRefusal()` reads
false for what is a rejection and a scripted run exits 0 on it.

The value is normalized once and the normalized value is what the wallet receives,
which is what the next case in this file already does for `fromAddress`.

Present is separated from truthy. `""`, `0` and `false` skipped the checks and still
reached the wallet, where `ownerAddress = address` only defaults for `undefined`, so
`!ownerAddress` answered "Wallet not initialized" about a wallet that was open. The
same case records that trap for an empty `fromAddress`.

`null` is the same trap through the alias, and it needed its own line. `{address: null}`
already arrives as `undefined`, because `??` skips a null left side. `{owner: null}` does
not: the alias is the right side, so the value stays `null`, the wallet's owner default
never fires, and an open wallet is reported as uninitialised. `?? undefined` makes
omitted and null-via-alias mean the same thing, which is what they are.

The coercion is itself a step that can fail. `String({ toString: null })` throws, and
`{"toString": null}` is JSON a model can emit; a value that was given and cannot be read
as text is still a given unusable value, so it refuses rather than letting the
converter's error stand in for the refusal. Falling back to an empty string would also
refuse, but it would report an empty address, which is not what was given.

Padding is trimmed rather than refused. `resolveRecipient` trims and carries the trimmed
value, and a read has nothing to sign. The opposite choice belongs to `transfer_nft`,
where the string does get signed — and the comment there claimed it was following the
recipient's rule, which it is not; corrected while nearby.

565 tests, up from 552. `getNfts` is injectable here for one reason: whether the owner
default fires is not observable from the outside. With no wallet, `getNfts(undefined)`
resolves its default to a null module address and `getNfts(null)` keeps the null — both
answer "Wallet not initialized", so a test on that message holds whether the default
fires or not. The seam lets the test read the argument, which is the only thing that
separates the two.
@BeeHiveTeam
BeeHiveTeam force-pushed the fix/nfts-owner-coercion branch from 3114243 to 2bf3b34 Compare October 5, 2026 04:24

@portdeveloper portdeveloper left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for covering the owner alias and the value passed to the wallet

@portdeveloper
portdeveloper merged commit ce97b94 into portdeveloper:main Oct 5, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants