Refuse an unreadable NFT token id instead of throwing or encoding zero - #105
Merged
Merged
Conversation
The token id went to a bare BigInt twice: in the ownership check and in the
calldata encoder. Neither asked whether the value was a token id, so the field
had both halves of the failure. An empty or blank id, an empty array, false —
all reach BigInt as 0, which is a real token, so the result was a complete,
signable transfer of token #0 that nobody asked for. Anything else BigInt or
ethers disliked came back as a raw converter error rather than a refusal.
The two conversions were also independent, which is why the ownership check did
not catch the fabrication: ownerOf(BigInt("")) asks about token #0 too, so the
two agreed on a token nobody named.
requireTokenId now resolves both. It takes a string, a number or a bigint,
refuses an empty or blank string, and requires the result to fit the uint256 the
ABI declares. Hex still works and "0" is still a token id — which is exactly what
"" had to stop being.
It throws rather than returning null because both refusals already in this file
throw, and an existing test requires a throw for a bad address; two mechanisms in
one function would be worse than either. The offending value is deliberately not
echoed: transferNft already prints a raw tokenId in its wrong-owner refusal, and
a second unescaped echo is not something to add here.
transferNft resolves the id before the session check. A malformed argument is the
caller's to fix whether or not a wallet is open, and it is what makes that refusal
reachable from a test at all — every other statement in the function needs an
initialised account. Without the reorder the guard was unobservable, which a
mutation confirmed by surviving.
BeeHiveTeam
force-pushed
the
fix/token-id-guard
branch
2 times, most recently
from
September 22, 2026 17:53
97bfae4 to
9cacb12
Compare
ColinkaMir
added a commit
to ColinkaMir/nad-agent
that referenced
this pull request
Sep 24, 2026
The precision is gone before any line of this repo runs. JSON.parse turns 9007199254740993 into 9007199254740992, so parseAction never sees the id the model wrote, String() then makes it an ordinary decimal, and requireTokenId from portdeveloper#105 accepts it because it is a legal uint256. The transfer is complete, signable, and for a different token. Nothing downstream can recover the digit, so this refuses rather than repairs, at the two places a number can enter. At the action boundary the check sits before the String() coercion, because after it the value is indistinguishable from an id somebody meant. In requireTokenId only the number branch changes: strings and bigints keep the whole uint256 range, so 9007199254740993 remains a usable id when it is written as a string, which is what the refusal tells the operator to do. runAction gained a transferNft seam beside the sendToken one it already had. Without it the ordering is unobservable: a test can see a refusal but not whether the wallet was reached first, and ordering is the entire fix here. Tests: the model output from the issue, asserting both the refusal and that the stubbed wallet recorded no call; the safe-integer boundary accepted as a number and the next value refused; the same id accepted as a string; the exported helpers refusing the number; an exact large string decoded back out of the calldata unchanged; and uint256 max still legal. Five of the eight fail on the code before this commit.
ColinkaMir
added a commit
to ColinkaMir/nad-agent
that referenced
this pull request
Sep 24, 2026
The precision is gone before any line of this repo runs. JSON.parse turns 9007199254740993 into 9007199254740992, so parseAction never sees the id the model wrote, String() then makes it an ordinary decimal, and requireTokenId from portdeveloper#105 accepts it because it is a legal uint256. The transfer is complete, signable, and for a different token. Nothing downstream can recover the digit, so this refuses rather than repairs, at the two places a number can enter. At the action boundary the check sits before the String() coercion, because after it the value is indistinguishable from an id somebody meant. In requireTokenId only the number branch changes: strings and bigints keep the whole uint256 range, so 9007199254740993 remains a usable id when it is written as a string, which is what the refusal tells the operator to do. runAction gained a transferNft seam beside the sendToken one it already had. Without it the ordering is unobservable: a test can see a refusal but not whether the wallet was reached first, and ordering is the entire fix here. Tests: the model output from the issue and the same number arriving as a native tool argument, both asserting the refusal and that the stubbed wallet recorded no call; the safe-integer boundary accepted as a number and the next value refused; the same id accepted as a string; the exported helpers refusing the number; an exact large string decoded back out of the calldata unchanged; and uint256 max still legal. Six of the nine fail with the sources checked out at the previous commit.
portdeveloper
pushed a commit
that referenced
this pull request
Sep 25, 2026
The precision is gone before any line of this repo runs. JSON.parse turns 9007199254740993 into 9007199254740992, so parseAction never sees the id the model wrote, String() then makes it an ordinary decimal, and requireTokenId from #105 accepts it because it is a legal uint256. The transfer is complete, signable, and for a different token. Nothing downstream can recover the digit, so this refuses rather than repairs, at the two places a number can enter. At the action boundary the check sits before the String() coercion, because after it the value is indistinguishable from an id somebody meant. In requireTokenId only the number branch changes: strings and bigints keep the whole uint256 range, so 9007199254740993 remains a usable id when it is written as a string, which is what the refusal tells the operator to do. runAction gained a transferNft seam beside the sendToken one it already had. Without it the ordering is unobservable: a test can see a refusal but not whether the wallet was reached first, and ordering is the entire fix here. Tests: the model output from the issue and the same number arriving as a native tool argument, both asserting the refusal and that the stubbed wallet recorded no call; the safe-integer boundary accepted as a number and the next value refused; the same id accepted as a string; the exported helpers refusing the number; an exact large string decoded back out of the calldata unchanged; and uint256 max still legal. Six of the nine fail with the sources checked out at the previous commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #103.
The token id reached
BigInttwice with nothing in front of it — once in the ownership check, once in the calldata encoder — so the field carried both halves of the failure, with the CLI guard atsrc/tools.mjs:996standing between the model and one of them.Through the CLI, an id that is non-empty goes to the wallet as
String(a.tokenId)and is unexamined from there:"abc","1.5","-1"and a 79-digit number come back as a rawSyntaxErroror ethersvalue out-of-bounds, where every neighbouring field on this path answers with a readableRefused:line. That is the inconsistency #79 fixed forfromAddress.Called directly, the exported helpers are looser.
src/tools.mjs:996refuses an id that is empty, blank,undefined,nullor[]before the wallet sees it;buildNftTransferCalldataandtransferNfthave no check of their own, andBigInt("")is0n, which is a real token. SobuildNftTransferCalldata(from, to, "")returns a complete, signablesafeTransferFromof token #0. The two conversions were independent, which is why the ownership check did not contradict it —ownerOf(BigInt(""))asks about token #0 too, so both sides agreed on a token nobody named.The change
One guarded conversion,
requireTokenId, used by both call sites:uint256the ABI declares —0through2^256-1"0x2a"→ 42), and"0"is still a token id, which is exactly what""had to stop beingIt throws rather than returning
null: both refusals already in this function throw, and an existing test requires a throw for a bad address. Two refusal mechanisms in one function would be worse than either.The offending value is deliberately not echoed in the message.
transferNftalready prints a rawtokenIdin its wrong-owner refusal, and a second unescaped echo is not something to add here.transferNftresolves the id before the session check. A malformed argument is the caller's to fix whether or not a wallet is open, and it is what makes the refusal reachable from a test at all — every other statement in that function needs an initialised account. Without the reorder the guard was unobservable, which a mutation confirmed by surviving.Coverage
518 tests, up from 494. The new table lists both directions in one place on purpose — a shape that encodes and a shape that is refused are statements about the same guard, and listing them apart is how one half drifts.
Encoded:
"730","0","0x2a"," 42 ",42,42n, maxuint256.Refused:
""," ","abc","1.5","1e3","-1",-1, max+1,null,undefined,true,false,[],["5"],{},{ toString: () => "7" }.The refusals assert the message, not only the throw:
err.message.startsWith("Refused:"). The throw alone was already there before this PR — it just came from ethers and said something else.A second test drives the same ids through
transferNftwith no wallet open, so the ownership check is pinned separately from the encoder rather than assumed to follow it.The four pre-existing
buildNftTransferCalldatatests are untouched, including the one that requires a 78-digit max-uint256id to keep encoding.Mutations
Each guard reverted on its own, full suite each time, plus a no-op control so a red result means the mutant and not the harness:
BigInt)1n << 256nBigInt(tokenId))Refused:prefix dropped from the messageThe ownership-check row is the one that began as a survivor, before the id was resolved ahead of the session check.
Scope
No change to the request, the ABI, the calldata for ids that were already valid, or any message other than the new refusal.
For direct callers, shapes that used to convert by coercion are now refused —
["5"],{ toString: () => "7" }. Through the CLI they are unaffected:String(a.tokenId)turns them into"5"and"7"before the wallet is called.Rebased onto
7e0ad55, so the counts above are against currentmainincluding the native tool-calling work.npm run buildclean; suite run ten consecutive times with no failures.