Repository navigation
fix(x402): rank Privy payment options and skip methods it cannot sign - #822
Conversation
createPrivyPaymentSignatures signed the EVM options in the order the server listed them, then Solana, without rankRequirements, so a Privy wallet could pay a more expensive option than the local wallet (nansen-ai#700) or WalletConnect (nansen-ai#745) would. It also signed every EVM option as an EIP-3009 TransferWithAuthorization, including permit2-exact options such as BNB Chain USDT, which the facilitator then rejects. Go through rankRequirements' order and skip options whose assetTransferMethod is not EIP-3009, with a reason, as the WalletConnect path does.
![nansen-pr-reviewer[bot]](https://avatars.githubusercontent.com/in/2987756?s=60&v=4){kind=small}
nansen-pr-reviewer summaryThe change consistently ranks Privy payment options by policy-approved USD cost, skips transfer methods Privy cannot sign, scopes wallet lookup failures to their chain, and includes focused tests plus a changeset. I found no clear correctness or security defect in the reviewed diff. Deterministic check: success — No issues found Risk: 3/5 (Elevated) ✅ Auto-approved — Deterministic gates passed: severity thresholds met, recommendation "approve", risk level 3/5 within threshold 3. Token usage: 36,609 input, 2,103 output, 35,350 cache read | Usage Guide Cooldown: for the next 10 minutes (counting from when this review finished), new pushes to this PR will not trigger another review — the next push after the window expires will. Need a fresh review sooner? Comment |
With options ranked across chains, a failed wallet lookup for the cheapest option's chain (e.g. a Privy API error) rejected the generator before options on the other chain were tried. Log the failure, skip that chain's options and keep going, with a test where the Solana lookup fails and the EVM option is still signed.
|
Good catch, fixed in the latest commit. A failed wallet lookup now logs the error and skips only that chain's options, so the remaining ranked options on the other chain are still tried. Added a test where the Solana lookup fails (Privy 503) and the EVM option is still signed; it fails without the change. |
There was a problem hiding this comment.
✅ Auto-approved by Nansen review
Deterministic gates passed: severity thresholds met, recommendation "approve", risk level 2/5 within threshold 3.
- The deterministic severity check passed: No issues found
- Risk level 2/5 (Low)
- The model recommended: approve
Approval was decided by deterministic gates, not by the model. The model's recommendation can block auto-approval but never cause it.
|
@erkancamli Thanks, this works as described. I ran the CLI against a mocked 402 for the ranking, permit2-exact, Solana-first and lookup-failure cases, and all of them behave correctly now. The new tests fail on main and pass here. One regression from the second commit: when a wallet lookup fails and no other option gets signed, the JSON error used to be x402 Privy payment failed: (for example Invalid app ID or app secret). Now it is just Payment required and the reason appears only on stderr. Could you keep the lookup error and if the loop finishes without signing anything, rethrow it? The fallback to the other chain would still work, and users who get the Privy setup wrong would see the reason in the error again. A test with an EVM-only 402 where the EVM lookup fails, asserting the generator rejects with that message, would cover it. |
Skipping a chain whose wallet lookup failed hid the reason when no other option could be signed: the CLI ended with "Payment required" and the Privy error only reached stderr. Keep the first lookup error and rethrow it when the loop finishes without signing anything, so the JSON error reads "x402 Privy payment failed: <reason>" again. The fallback to an option on the other chain is unchanged.
|
@kome12 thanks for testing it. Done in the latest commit: the first wallet lookup error is kept, and if the loop finishes without signing anything it is rethrown, so the JSON error reads Added the test you described: an EVM-only 402 whose EVM lookup fails with a 401 |
There was a problem hiding this comment.
✅ Auto-approved by Nansen review
Deterministic gates passed: severity thresholds met, recommendation "approve", risk level 3/5 within threshold 3.
- The deterministic severity check passed: No issues found
- Risk level 3/5 (Elevated)
- The model recommended: approve
Approval was decided by deterministic gates, not by the model. The model's recommendation can block auto-approval but never cause it.
x402 auto-payment with a Privy default wallet does not follow the rules the local wallet (#700) and WalletConnect (#745) paths already follow.
createPrivyPaymentSignatures(src/privy.js):rankRequirements. With a 402 listing[Base USDC 0.90, Base USDC 0.01](the case from fix(x402): pay the cheapest viable option a server offers #700), a Privy wallet signs and pays 900000 base units, while a local wallet pays 10000.TransferWithAuthorization, whatever itsextra.assetTransferMethodis. For apermit2-exactoption such as BNB Chain USDT (feat(x402): BSC multi-asset payments + Permit2 signing #457), the payload claims Permit2 but carries an EIP-3009 authorization for a token that has notransferWithAuthorization, so it cannot be settled and the request ends in "Payment required". The local wallet signs the same option with Permit2.Fix
rankRequirements(requirements), so EVM and Solana options are tried cheapest first. The per-option signing code moves unchanged into two helpers, one for EVM and one for Solana; each wallet is still looked up once.assetTransferMethodis not EIP-3009 are skipped with the reason, the same rule as the WalletConnect path, and the next option is tried.Tests
src/__tests__/privy.test.js(ranking across EVM options, Solana before a pricier EVM option, skippingpermit2-exact, skipping an unknown method). All 4 fail onmainand pass with this change.nansen research smart-money netflowend to end with a temp HOME and a mocked API, RPC and Privy client. Onmainthe Privy wallet signsvalue=900000for the ranking case and an EIP-3009 payload for thepermit2-exactcase. With this change it pays 10000, and it skips thepermit2-exactoption with the reason.npm run lintpasses.npm test:The 5 failures are the
src/__tests__/doctor.test.jscases for unreadable wallets and config files. My sandbox runs as root, wherechmod 000does not block reads; they fail the same way on unmodifiedmainthere.