Repository navigation
feat(payments): handle Token-2022 transfer fees and scaled UI amounts on single transfers - #2491
multipletwigs wants to merge 17 commits into
Conversation
|
React Doctor found 3 new issues in 3 files · 3 warnings · score 93 / 100 (Great) · 0 fixed · vs 3 warnings
Reviewed by React Doctor for commit |
 [Medium impact] The changes affect token transfers and payment transaction views. Fix the payment-request test setup and the misleading received-amount display before merging. FindingsSummaryAdds Token-2022 fee-aware transfers and scaled amount conversion.
Reviews (3) · Last reviewed commit: "feat(web): align the unverified-mint war..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const transferFee = | ||
| signed.fee === null ? undefined : formatDecimalAmount(signed.fee, signed.decimals); | ||
| const movedAmount = formatDecimalAmount(signed.amount, signed.decimals); | ||
| const scaled = compareDecimalAmounts(movedAmount, row.amount) !== 0; | ||
| return { | ||
| transferFee, | ||
| receivedAmount: | ||
| transferFee === undefined ? undefined : subtractDecimalAmounts(row.amount, transferFee), |
There was a problem hiding this comment.
receivedAmount subtracts an unscaled fee from a scaled amount. Custom mints can enable both extensions. With a multiplier of 2 and a 1% fee, sending 10 moves 5 underlying tokens and withholds 0.05. The recipient gets 9.9 displayed tokens, but this returns 9.95.
Convert the fee and received amount to the same UI scale before returning them, and add a test with both extensions.
| const transferFee = | ||
| signed.fee === null ? undefined : formatDecimalAmount(signed.fee, signed.decimals); | ||
| const movedAmount = formatDecimalAmount(signed.amount, signed.decimals); | ||
| const scaled = compareDecimalAmounts(movedAmount, row.amount) !== 0; | ||
| return { | ||
| transferFee, | ||
| receivedAmount: | ||
| transferFee === undefined ? undefined : subtractDecimalAmounts(row.amount, transferFee), |
There was a problem hiding this comment.
receivedAmount subtracts an underlying fee from a scaled UI amount. Sending 10 with multiplier 2 and a 1% fee moves 5 underlying tokens and withholds 0.05. The recipient sees 9.9, but this returns 9.95 and labels the fee as 0.05.
Convert the fee and received balance into the same units as amount, and add a test combining both extensions.
| <RecordRow label={t("DashboardPayments.transactionDetail.recipientReceived")}> | ||
| {receivedAfterFee.received} |
There was a problem hiding this comment.
The new “Recipient received” row also appears for failed and still-processing transfers. signedTokenTransferFields reads the signed instructions regardless of status, and signed bytes remain stored when a send fails. A rejected transfer can therefore show a positive amount as received even though no tokens moved.
Show this row only after successful confirmation, or clearly label it as an expected amount.
| const symbolsByMint = await lookupTokenSymbols( | ||
| result.rows.flatMap((row) => (row.token === null ? [] : [row.token])) | ||
| ); |
There was a problem hiding this comment.
/v1/transactions now waits for optional token symbols before returning its database rows. The DAS request has no timeout, and the cache shares unfinished requests. If Helius stalls, this list and later reads for the same tokens remain waiting.
Add a bounded timeout so the list can return without symbols.
Windscar
left a comment
Windscar
left a comment
There was a problem hiding this comment.
Good work. The transfer construction looks sound, but the API reports the wrong recipient amount when a mint has both scaled UI amounts and transfer fees.
Reviewed: Token-2022 amount and fee calculation, signed transaction decoding, off-ramp funding, sponsorship fee reporting, and dashboard presentation.
Required for approval:
- Express
transferFeeandreceivedAmountin the same scaled UI units for mints with both extensions; see inline comment.
| return { | ||
| transferFee, | ||
| receivedAmount: | ||
| transferFee === undefined ? undefined : subtractDecimalAmounts(row.amount, transferFee), |
There was a problem hiding this comment.
row.amount is the requested scaled UI amount, while transferFee comes from the signed instruction in underlying units. With a 10× multiplier, a request for 100 UI units moves 10 underlying units; if 1 is withheld, the recipient gets 90 UI units, but this returns receivedAmount: "99" and transferFee: "1". Please calculate both displayed values in the same units, using the transferred amount after the fee. A test with both extensions would catch this.
There was a problem hiding this comment.
Addressed on the current head: transferFee and receivedAmount now both use underlying units, with matching API documentation and dashboard labels. I'm superseding this finding with a review of the new head.
Windscar
left a comment
Windscar
left a comment
There was a problem hiding this comment.
Good work. The scaled fee calculation from my earlier review is addressed: the fee and received amount now use underlying units, and the contract and UI labels say so.
Reviewed: Token-2022 conversion and signed amounts, failed submission state, payment and batch scoping, transaction symbols, and dashboard presentation.
Required for approval:
- Show
receivedAmountas received only after successful settlement; see the mapper comment. - Guard scaled transfers whose f64 conversion still displays less than the requested amount; see the conversion comment.
- Warn when a transaction view displays a self-declared mint symbol, including the classic list and the detail's primary amount; see the UI comments.
| receivedAmount: | ||
| signed.fee === null | ||
| ? undefined | ||
| : formatDecimalAmount(signed.amount - signed.fee, signed.decimals), |
There was a problem hiding this comment.
This is derived from saved signed bytes regardless of row.status. Those bytes are persisted before broadcast and retained on a definite submission or chain failure, so a failed or still-processing transfer can return a positive receivedAmount; the dashboard then says ‘Recipient received’ even though nothing settled. Gate the received field on the canonical successful status predicate, or label it as an expected amount until confirmation.
| multiplier: number | ||
| ): bigint { | ||
| const shownUiUnits = BigInt(Math.trunc(Number(truncatedBaseUnits) * multiplier)); | ||
| return shownUiUnits < requestedUiUnits ? truncatedBaseUnits + 1n : truncatedBaseUnits; |
There was a problem hiding this comment.
Adding one base unit does not always make the displayed amount reach the request because Number(raw + 1n) can equal Number(raw). With 6 decimals, multiplier 0.001, and request 90000000.000002, the pinned conversion helper gives 90000000000001984n; both that value and the incremented value display as 90000000.000001. This signs a transfer that delivers less than requested. Please keep incrementing until the display check passes, or reject amounts where the f64 conversion cannot guarantee it.
| : transaction.token === null | ||
| ? transaction.amount | ||
| : `${transaction.amount} ${resolveTokenByMint(transaction.token, issuedTokensByMint).tokenName}`} | ||
| : `${transaction.amount} ${resolveTokenByMint(transaction.token, issuedTokensByMint, transaction.tokenSymbol).tokenName}`} |
There was a problem hiding this comment.
This now shows the mint's self-declared DAS symbol for an unknown mint without the unverified-mint warning added to the redesign list. A mint claiming a familiar symbol can therefore look verified in this transactions view. Please apply the same warning here.
| <RecordRow | ||
| label={t("DashboardPayments.transactionDetail.scaledAmount")} | ||
| hint={t("DashboardPayments.transactionDetail.scaledAmountHint", scaled)} | ||
| valueAccessory={unverifiedToken ? <UnverifiedMintWarning /> : undefined} |
There was a problem hiding this comment.
The warning is attached only to the underlying-amount row, which is absent for ordinary unscaled mints. The primary amount and page title still show the self-declared symbol with no warning. Please put the warning beside the primary amount whenever unverifiedToken is true.
… on single transfers Transfers convert scaled UI amounts with the multiplier in effect at the Clock, state the expected fee with TransferCheckedWithFee on fee mints, and report transferFee, receivedAmount and the priced network fee. Unified transaction rows carry the mint's DAS symbol.
Transfer responses carry unscaledAmount and uiAmountMultiplier, decoded from the signed TransferChecked instruction, and the send screen and transaction detail show a Scaled amount row with an info hint.
…ansfer rows A scaled mint's truncated conversion could show the recipient one UI unit short; one more base unit covers it. A transaction with several Token-2022 transfers (a batch chunk) no longer reports one recipient's terms as the row's.
…a derived multiplier amount divided by whole base units only approximates the mint's multiplier (500 / 499.975002 = 1.000049998 on a 1.00005 mint), so the response drops uiAmountMultiplier and the row reads the underlying amount and what it is shown as.
…nd flag unverified mints Payment requests and batches refuse scaled-UI and fee mints until they build and settle against the signed terms. Fee fields come from the signed base units, so they are underlying amounts on scaled mints. DAS symbol lookups get a 3s deadline and skip native and well-known mints; out-of-range scaled amounts are a 400. The transaction detail flags names that only the mint's own metadata vouches for.
a1b1270 to
bbc322d
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -361,14 +365,26 @@ function DetailsStep({ wizard, contact, privateSend, onPayByBank }: StepProps) { | |||
|
|
|||
| function ReviewSummary({ wizard, counterpartyName }: StepProps) { | |||
There was a problem hiding this comment.
React Doctor · react-doctor/no-high-complexity-react-function (warning)
ReviewSummary has cyclomatic complexity 17, cognitive complexity 17, and maximum nesting depth 2, so its React logic is hard to understand and change. Extract independent branches into components or hooks.
Fix → Extract independent render branches and state logic into focused components or hooks until the control flow is easy to follow.
| * @param tokenSymbol - The row's metadata symbol, when the API resolved one. | ||
| * @returns True when the name shown is only the mint's self-declared symbol. | ||
| */ | ||
| export function isUnverifiedMintName( |
There was a problem hiding this comment.
React Doctor · react-doctor/only-export-components (warning)
This file exports non-components, so Fast Refresh can't safely preserve component state.
Fix → Move non-component exports out of component files so Fast Refresh can preserve component state instead of full-reloading.
| const wallet = resolveWallet(scope.wallets, body.walletId); | ||
| const token = normalizePaymentToken(body.token); | ||
| if (token !== SOL_MINT) { | ||
| await assertMintMovesUnscaled(assertValidAddress(token, "token"), "Payment requests"); |
There was a problem hiding this comment.
The USDC create test in payments.requests.idempotency.test.ts expects 201, but its setup only mocks the old RPC wrappers. The new guard calls fetchEncodedAccount(createClusterRpc(), mint) directly, while global test setup clears every managed RPC endpoint. The request therefore fails before creating the row, breaking the suite in CI.
Install the cluster RPC stub and answer the mint read in this suite, as the updated transfer suites do.
keeganthomp
left a comment
keeganthomp
left a comment
There was a problem hiding this comment.
Blocking: PYUSD/USDG payment requests and batches now 400 (token-accounts.ts:311), and three paths re-read RPC state they already hold (handlers.ts:713, resolve.ts:75, token-accounts.ts:514); nothing in this stack writes #2493's ui_amount_multiplier, so is it still needed?
| // biome-ignore lint/security/noSecrets: Token-2022 extension name, not a secret. | ||
| isExtension("ScaledUiAmountConfig", extension) | ||
| ), | ||
| transferFeeConfig: extensions.find((extension) => isExtension("TransferFeeConfig", extension)), |
There was a problem hiding this comment.
PYUSD and USDG both carry a TransferFeeConfig at 0 bps (mainnet and devnet), so assertMintMovesUnscaled now 400s their payment requests and batches, and every single PYUSD/USDG transfer reports transferFee: "0". Count only a config that can charge, which puts them back on main's TransferChecked:
| transferFeeConfig: extensions.find((extension) => isExtension("TransferFeeConfig", extension)), | |
| // PYUSD and USDG carry a 0 bps config; only one that can charge changes the amount. | |
| transferFeeConfig: extensions.find( | |
| (extension): extension is TransferFeeConfig => | |
| isExtension("TransferFeeConfig", extension) && | |
| [extension.olderTransferFee, extension.newerTransferFee].some( | |
| (fee) => fee.transferFeeBasisPoints > 0 && fee.maximumFee > 0n | |
| ) | |
| ), |
| const partiallySigned = await partiallySignTransactionMessageWithSigners(message); | ||
| const txEncoder = getTransactionEncoder(); | ||
| const txBytes = new Uint8Array(txEncoder.encode(partiallySigned)); | ||
| const networkFee = await sponsoredNetworkFee(txBytes); |
There was a problem hiding this comment.
On managed deployments BudgetedFeePayment.prepareAdmission already prices these exact bytes with sponsoredNetworkFee, so every single transfer now pays a second, sequential getFeeForMessage. Have prepareOwnedSubmission return the fee it already priced (self-hosted would price there) instead of calling it again here.
| await assertMintMovesUnscaled(mintAddress, "Batch transfers"); | ||
| const tokenProgram = await resolveMintTokenProgram(rpc, mintAddress); |
There was a problem hiding this comment.
assertMintMovesUnscaled fetches the mint, then resolveMintTokenProgram fetches it again on the next line: two getAccountInfo calls for one account per batch. Once the assert returns the TransferMint it read, this becomes (and the resolveMintTokenProgram import goes):
| await assertMintMovesUnscaled(mintAddress, "Batch transfers"); | |
| const tokenProgram = await resolveMintTokenProgram(rpc, mintAddress); | |
| const { tokenProgram } = await assertMintMovesUnscaled(mintAddress, "Batch transfers"); |
| source?: SplTransferSource; | ||
| }) { | ||
| const rpc = createClusterRpc(); | ||
| const transferMint = await readTransferMint(input.mint); |
There was a problem hiding this comment.
A ramp deposit passes source because assertRampDepositFunded already read the mint (and Clock), but this re-reads both, where main reused source.tokenProgram. Carry the TransferMint on SplTransferSource and read only when it's absent.
| unscaledAmount: z.string().optional().openapi({ | ||
| description: | ||
| "The base units the transfer moved, at the mint's decimals, before its Token-2022 scaled UI amount multiplier. Present only when the mint scaled `amount`.", | ||
| example: "5", |
There was a problem hiding this comment.
nit: the combined example can't happen: with unscaledAmount 5, underlying receivedAmount can't be 99.75.
This branch was successfully deployed
3 participants
Single transfers now handle Token-2022 transfer-fee and scaled-UI mints, and transfer responses report the fee.
TransferCheckedWithFeewith the epoch's expected fee, so an issuer fee change fails the send (FeeMismatch) instead of settling short.unscaledAmount, shown as an Underlying amount row.transferFee/receivedAmountare decoded from the savedsigned_transaction: no new columns, present only when a fee is stated.feeis now the network fee priced withsponsoredNetworkFeebefore the send, not the budget ledger (its settled lamports include ATA rent)./v1/transactionsrows gain optionaltokenSymbolfrom Helius DAS; it is issuer-controlled, so well-known mints keep priority in the dashboard.amount; not handled here.API after (local):
POST /v1/payments/transfers10 SDPFEE →"amount": "10", "transferFee": "0.1";GET /v1/transactions→"token": "So111…", "tokenSymbol": "SOL".Verification: typecheck passes in every package except sdp-web's stale local
.next/types;check:value-movementpasses; local app: a 10 SDPFEE devnet send confirmed withtransferFee: "0.1". Tests: 18 new cases across token-accounts, mappers, DAS and transactions suites, plus updated transfer route suites; vitest not run locally (CI runs it); Pi browser verify and screenshots not run.