Repository navigation
Conversation
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adds assertValidTokenTransferDestination(rpc, { mint, destination, recipient }, config?) to the JS client as the token counterpart of assertValidTransferSolDestination (solana-program/system#96). It fetches the recipient and destination in parallel, derives the recipient's ATA, and throws a typed InvalidTokenTransferDestinationError with one of eight reasons. Three of the checks (unexpected-owner / unfunded-recipient / off-curve) are the SOL helper's rules applied to the recipient wallet; the remaining five validate the token account itself. Two token-specific knobs are added: allowNonAssociatedTokenAccount (accept any existing token account owned by the recipient) and allowMissingAssociatedTokenAccount (default true, mirrors transferToATA's idempotent create).
The implementation is correct, the ordering is deterministic and matches the docblock, and the LiteSVM tests cover every reason plus the two non-obvious orderings (owner-before-off-curve, and allowMissingAssociatedTokenAccount: false overriding allowUnfundedRecipient: true). The API shape, error class, and InstructionAccountInput handling are all consistent with the merged system helper, so there's nothing new to decide on that front.
Things to watch out for (all non-blocking)
- Frozen destinations pass the check but the transfer will fail. The Token program rejects a transfer when either side is frozen, so an ATA in
AccountState.Frozengets through here and thentransferToATAfails on-chain withAccountFrozen. Given this helper's whole point is pre-flighting the transfer, a'frozen'reason seems in scope. See inline. - Guard-option leak test doesn't cover the new options. The
does not forward guard options to getAccountInfotest passes the three options inherited from the SOL helper but notallowNonAssociatedTokenAccount/allowMissingAssociatedTokenAccount, so it wouldn't catch those being dropped from the destructure. See inline. mint?on the error is the destination's actual mint, sitting alongside an input namedmint(the expected one).tokenOwneris disambiguated butmintisn't.tokenMintwould be symmetric withtokenOwner, but it's a naming nit — fine to leave if you prefer parity with the SOL helper's flat fields.- Token program is hardcoded.
findAssociatedTokenPdausesTOKEN_PROGRAM_ADDRESS, anddecodeToken+getTokenSize()are SPL-Token-specific anyway, so this is the right call for this package. Just noting that a caller usinggetTransferToATAInstructionPlanwithconfig.tokenProgramset to Token-2022 would getata-mismatchhere, which is a slightly misleading message. A one-line doc note ("Token program only; see the token-2022 client for its equivalent") would pre-empt that question when the sibling lands.
For subsequent reviewers
- No
.changeset/in this repo. This adds new public exports (assertValidTokenTransferDestination,InvalidTokenTransferDestinationError,InvalidTokenTransferDestinationReason,InvalidTokenTransferDestinationDetails,AssertValidTokenTransferDestinationInput,AssertValidTokenTransferDestinationConfig), so the next JS release should be a minor bump. @solana-program/systemwas already a runtime dependency (^0.16.0) and@solana/kit/program-client-coreis already imported bytransferToATA.ts, so no new dependency surface.- The
destinationAccount.data.length === getTokenSize()guard beforedecodeTokenis what keeps mints (82 bytes) and multisigs (355 bytes) from throwing inside the decoder; thenot-token-accounttest iterates over wallet / mint / multisig / uninitialized-165-byte accounts to pin that down. - The
unexpected-ownercheck runs beforeata-mismatch, so passing an ATA as the recipient (an easy mistake) reports the recipient as Token-owned rather than a confusing "destination is not the ATA of ". That's tested.
LGTM.
|
great feedback @mcintyre94!
|
|
#194 covers the audit failures, just rebase to resolve the CI issue. |
Refs: DEV-1241
Refs: DEV-1241
Refs: DEV-1241
Refs: DEV-1241
Refs: DEV-1241
d23bba4 to
d0c4568
Compare
|
done
|
mcintyre94
left a comment
There was a problem hiding this comment.
This LGTM now, thanks for the changes!
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Re-fetched the diff since my first pass. All four points from my previous review are addressed, plus one larger shape change:
'frozen'reason added, checked last (after owner match), with a LiteSVM test that creates a mint with a freeze authority and freezes the ATA. ✅- Guard-option leak test now passes every config key (
allowMissingAssociatedTokenAccount,allowNonAssociatedTokenAccount,associatedTokenProgram,tokenProgram, …) and asserts onlycommitment+encodingreachgetAccountInfo. ✅ - Error fields moved into a
detailsdiscriminated union ({ reason: 'mint-mismatch', mint },{ reason: 'token-owner-mismatch', tokenOwner }, …). This resolves themintnaming ambiguity and is strictly better than optional flat fields — callers can narrow ondetails.reasonand get the right extra field typed. Note it diverges fromassertValidTransferSolDestination's flatreason/owner?shape; fine by me, but if the system helper ever gets a v2 it's worth aligning. tokenProgram/associatedTokenProgramconfig added rather than hardcoding Token.isTokenAccountDatahandles the Token-2022 layout: exactlygetTokenSize()bytes, or (outside the Token program) longer, notgetMultisigSize(), with theAccountTypebyte at offset 165 equal to2. I checked this against Token-2022's extension layout: mints with extensions are padded to 165 and carryAccountType = 1, multisigs never have extensions, and Token-2022 deliberately pads extended accounts so they never land on exactlyMultisig::LEN, so the length exclusion can't misclassify a real token account. The Token-2022 test (ATA carriesImmutableOwner, so it's > 165 bytes) exercises this path end-to-end.
Nothing new to flag. Two housekeeping notes before merge:
- The PR description is stale: it still describes flat
owner?/mint?/tokenOwner?fields and the reason table is missingfrozenand thetokenProgram/associatedTokenProgramoptions. Worth updating since it'll become the release-notes reference. - Still a minor bump on the next JS release (new public exports).
LGTM.
Summary
assertValidTokenTransferDestinationso exchanges can check a token withdrawal destination before buildingtransferToATA. Same shape asassertValidTransferSolDestination.mint/destination/recipientinputs asgetTransferToATAInstructionPlan, fetches the recipient and destination, and throws anInvalidTokenTransferDestinationError(typeddestination,recipient,reason, plusowner?/mint?/tokenOwner?).allowNonAssociatedTokenAccount: trueaccepts an existing token account formintowned byrecipient. Transfer to these withtransferChecked, sincetransferToATA` only works with the ATA.allowMissingAssociatedTokenAccount: falserequires the ATA to already exist, for teams that won't pay its rent or want proof the recipient has held the mint.reasonunexpected-ownerprogramOwnerunfunded-recipientallowUnfundedRecipient: trueoff-curveallowOffCurve: falseallowOffCurve: true)ata-mismatchdestinationisn't the recipient's ATA formintallowNonAssociatedTokenAccount: truemissing-token-accountallowMissingAssociatedTokenAccount: false)allowMissingAssociatedTokenAccount: true)not-token-accountmint-mismatchtoken-owner-mismatchExample
Closes DEV-1241