Repository navigation
fix(payments): validate and settle exact payment amounts (APE-853) - #2473
multipletwigs wants to merge 16 commits into
Conversation
 [Critical impact] The changes reach payment admission and settlement across several payment flows. Resolve lookup-table recipients before merging so valid received payments can settle. FindingsSummaryAdds exact amount checks before storing payment work and compares raw integer balances during payment-request settlement.
Reviews (7) · Last reviewed commit: "Merge branch 'bashtwigs/ape-355-399-toke..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…stored The payment amount parse maps AmountError to a 400 at the converter, and transfers, payment requests and recurring create/update/resume check the amount against the mint's decimals before writing their row. Recurring activation, update and collection convert through the same static-amount read, so resolveMintDecimals (getTokenSupply) goes.
…t hold Plan create runs the token and static-amount checks prepare already runs, so a too-precise amount, native SOL, or an interest-bearing or scaled mint is a 400 before the plan is stored rather than a plan that can never be prepared.
a9f19d4 to
f344fce
Compare
…e token admission Token eligibility is start admission: an update asks it only when the request names a token and the update is not a recovered stale one, and a resume only when it is not settling a submit already sent. The amount precision check stays on every update. Pausing an issued token mid-flight no longer strands a schedule in updating.
…d every fresh update A resume asks the token terms where a recovered one is already admitted: after a landed recovery returns, before any new signature, so a recovered resume that must sign again is refused on a paused token. Every fresh update asks token admission again; only a recovered stale update skips it, as it skips money admission.
multipletwigs
changed the title
G1de0n
left a comment
G1de0n
left a comment
There was a problem hiding this comment.
The bigint settlement and create-time coverage look good, and all the greptile threads are addressed. Left one inline on the mint read in settlement. A few more:
- The fresh-ATA case still doesn't settle: settlement returns early when
preis undefined (:186), and the/paybuilder always addscreateDestinationAtaInstruction, so the first payment of a mint into a wallet without that ATA lands but the request staysawaiting_payment. Since this PR rewrote the comparison, treating a missingpreas 0 whenposthas the matching mint/decimals (plus a fresh-ATA test) would fit here. - Legacy over-precise rows return silently at :140 and stay payable on
/pay. For SOL,payment-request-sponsorship.ts:251still callsparseTokenAmountdirectly, so theAmountErrorcomes out as a 500. Routing it throughloadPaymentMint(SOL_MINT).toBaseUnitsand logging the skip would help. resolveDestinationTokenAccountincollection.ts:311now usesloadPaymentMint, so recovering an older collection on a refused-extension mint 400s on every retry. That path only needs the token program.- Transfer-fee mints still can't settle exact (recipient nets less than amount). Fine as out of scope, but refusing them at create would be clearer than a request that never settles.
| return row; | ||
| } | ||
|
|
||
| const mint = await loadPaymentMint(assertValidAddress(row.token, "token")); |
There was a problem hiding this comment.
This mint read can now take down the whole list. loadPaymentMint throws badRequest for a mint that's gone, not a token account, or one of the refused extensions, and reconcilePaymentRequest rethrows any AppError even with bestEffort (:84). listPaymentRequests reconciles the page with Promise.all (handlers.ts:75), so one bad row fails the tenant's whole list with 400, plus the single GET and the /pay page for that row.
Rows like that exist: the create path on the base stored the token without reading the mint, and a closeable mint can go away later. It's also cheap to trigger, since /pay/:token exposes the reference and any tx listing it gets past findReference and verifyTransactionLanded. Before this PR the same row fell through validateTransfer and came back unchanged.
Catching the AppError from the mint read and returning the row (like the AmountError case right below), plus making the list reconcile per row (Promise.allSettled or a per-row catch), would close it.
G1de0n
left a comment
G1de0n
left a comment
There was a problem hiding this comment.
Nice, the latest push covers the mint-read isolation, the logging and fresh token accounts. Two small cleanups left:
assertPaymentAmountFitsMintandparseStaticPaymentAmounthave the same body (token-amount.ts:131-145), andreadStaticAmountMintDecimalshas no production caller left. Keeping justparseStaticPaymentAmountwould do.ResolvedRecurringPaymentUpdate.tokenis typedstring(update.ts:156) even though it's already anAddress, so it gets re-parsed at :994 and :1475. Typing it asAddressdrops both.
|
|
||
| /** Validates the referenced transfer and its exact recipient balance increase in one read. */ | ||
| export async function validatePaymentRequestTransfer( | ||
| rpc: SolanaRpc, |
There was a problem hiding this comment.
validatePaymentRequestTransfer takes an RPC client from its caller. The “RPC and fee payment” section requires on-chain helpers to read the open scope through createClusterRpc() instead of passing its RPC client between functions. Resolve the client inside this helper and remove the caller argument. This repository requirement must be satisfied before merging.
File Used: docs/contributing/code-standards.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Windscar
left a comment
Windscar
left a comment
There was a problem hiding this comment.
Looks good. The amount and settlement changes hold up.
Reviewed: transfer and batch validation, payment-request settlement, recurring update and recovery paths, and the issues raised in earlier review threads.
…wigs/payment-amount-precision-at-request # Conflicts: # apps/sdp-api/src/services/payments/recurring-payments/update.ts
| const accountIndex = compiled.staticAccounts.indexOf(recipientAccount); | ||
| if (accountIndex === -1) throw new ValidateTransferError("Missing recipient account"); |
There was a problem hiding this comment.
validatePaymentRequestTransfer rejects a valid version-0 payment when its recipient or recipient ATA comes from an address lookup table. It decompiles without resolving those addresses, then searches only compiled.staticAccounts for the recipient. The recipient is absent from that list, so the request stays unpaid even after receiving the full amount.
Resolve the loaded addresses and use the complete account list for both instruction checks and balance indexes.
Knowledge Base Used: Payments and earn integrations
This branch was successfully deployed
3 participants
Payment amounts with excess mint precision or a value outside u64 are rejected with 400 before a transfer, payment request, recurring schedule or subscription plan is stored. Payment-request settlement also compares raw integer balances, closing APE-853. Stacked on #2453.
For example, a legacy six-decimal USDC request for
1.0000001previously became paid after receiving1.000000: Solana Pay rounded the expected number to six decimals. It now stays awaiting payment. An exact amount above 2^53 raw is accepted, and a payment one raw unit short stays unpaid.RPC budget: SOL and catalogue-mint settlement stay at 4 calls. Custom-mint settlement adds one required mint read (4 → 5); repeated rows of that mint in the same scope share it. Early create validation adds no build read: a custom SPL send still has one mint and one source read, while catalogue mint facts require none.
Verification: regression reproduced against the real Solana Pay validator before the fix; exact, one-unit-short, legacy excessive-precision and reference checks; payment-request create precision/u64 tests; scoped lookup sharing/retry tests; payment, batch, recurring, subscription and pay-link suites; 277 tests across six affected suites plus 29 settlement tests including fresh-ATA, underpayment, reference, program, mint and decimals checks; API typecheck, Biome, module-boundary and value-movement checks.
Transfer-fee handling is out of scope by request, preserving USDG/PYUSD support. Finality remains in #2480; expiry/atomic settlement (APE-571) is separate. No migration or new dependency.