Conversation
Add NANSEN_TRADING_EXECUTION_ROUTE=standard to broadcast a swap through the execution endpoint that tracks the on-chain outcome server-side. Defaults to legacy, so behaviour is unchanged unless the route is opted in. On the standard route: - translate the request body (VM-type chain + chain id, explicit source, wallet address, cross-chain fields) and normalise the response back to the legacy result shape so the call sites and output block are unchanged - classify a 409 duplicate submission as a fatal broadcast failure: mark the quote spent and abort rather than broadcast the next candidate on top of a possibly-live swap - treat a node-rejected soft-fail as spent (warn, do not re-broadcast) - send a stable per-invocation attempt id on every POST, including retries - keep gasless on the legacy route (no gasless envelope on the new route) - omit the Broadcaster output line, which the new route does not report Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The standard execution route holds a per-quote single-flight lock and
deliberately keeps it after any attempt that produced a transaction hash.
Every candidate quote in one response shares a single backend quote id, so
the existing "try the next quote" fallback could only ever collect a 409 —
after signing a second transaction, which on the Privy and WalletConnect
signers means a real user prompt. Stop at a failed swap instead, on both
the 200-with-hash shape and the on-chain receipt revert, and report the
outcome that actually happened rather than a duplicate-submission error.
Also:
- Surface the aggregator's own revert reason. The route returns a typed
code with a STATIC message and puts the real cause in revertReason, so
reading message alone dropped the only informative field.
- Validate NANSEN_TRADING_EXECUTION_ROUTE instead of defaulting. A typo
('Standard', a stray space) silently running the legacy route is the
worst failure mode during a staged rollout: you believe you exercised
the new route and you did not. Resolved once, before any signing.
- Say up front that the receipt poll after a rejected broadcast can take
three minutes and may time out. broadcastSucceeded is set by both the
EVM and Solana executors, so on EVM the "may still propagate, keep
going" warning was otherwise buried behind a silent wait and a scarier
looking error.
Request and response shapes verified against the trading service schemas
rather than inferred. Tests are mutation-verified: each guard was broken
in turn to confirm the covering test fails.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All three swap e2e groups gated on `wallet list` printing an `EVM:` / `Solana:` summary, but that command reports data and so prints the standard JSON envelope on stdout in a terminal and a pipe alike (#584, extended to `wallet show` and redacted `wallet export` in #727). The preconditions could therefore never pass, and because each group is sequential, the whole file was unrunnable. Parse the envelope through one `walletAddresses()` helper instead of grepping prose, and take the Solana address from the parsed wallet rather than a base58 regex over human output. Pre-existing on main; unrelated to the execution route, but it blocks any e2e run of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nansen-pr-reviewer summaryThe change adds an opt-in standard execution route with fail-closed broadcast handling, quote locking, WalletConnect safeguards, and chain-name normalization. The new behavior is extensively covered by mocked unit tests, and the previously reported case-sensitive chain lookup issue is fixed. No correctness or security issue was identified in the reviewed diff. Deterministic check: success — No issues found Risk: 4/5 (High) — raised by: complexity: 1192 added lines Token usage: 105,692 input, 1,957 output, 21,806 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
A WalletConnect wallet that broadcasts the swap itself hands back a tx hash instead of signed bytes, so the swap never reaches the standard execution endpoint. That bypass is unavoidable — there is nothing to POST — but it left WalletConnect as the one signer where selecting the standard route changed nothing at all: a landed revert still fell through to the next candidate, which is a second real swap, approved in the wallet, against a quote already recorded as spent. Hoist the route decision above the signer branches so the WalletConnect path can see it, say out loud when the selected route was bypassed, and apply the same "a landed revert ends the command" rule there. Also tighten the two new privy.io URL checks in the standard-route Solana test to a full-prefix host match, which is what the substring form was flagged for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![nansen-pr-reviewer[bot]](https://avatars.githubusercontent.com/in/2987756?s=60&v=4){kind=small}
… aggregator id as the lock key Two gaps left by the previous round, both on paths where the standard route's "stop at a failed swap" contract did not actually hold. A WalletConnect send failure is ambiguous by construction: wcExec drops the child's exit code and kill signal, so a user rejection (nothing was broadcast) and a send-transaction timeout (the wallet may have accepted AND broadcast before the CLI gave up) arrive at the catch as the same bare Error. The wallet broadcasts for us on this path, so no backend single-flight lock stands between the next candidate and a second real swap on top of a possibly-live one -- and it is a second approval prompt either way. The landed-revert case already ends the command here; the ambiguous case, which is strictly less certain, fell through to the next candidate. Apply the same rule. The quote was already unusable ( swapHandedOff keeps the claim), so only the in-process fallthrough needed closing. quoteId is the key the backend holds its per-quote single-flight lock on, and the fallback feeding it was a per-quote id minted by the AGGREGATOR in its own namespace -- LiFi returns '<uuid>:<index>', which is not the backend id in the same response. Sending that would present a value the lock cannot match: the request would look idempotency- protected and would not be. Keep the fallback on the legacy route, where it only costs BI correlation, and send the backend id or nothing on the standard route. Every live /quote response carries one, so the absent case is shape drift rather than routine: say the lock does not apply instead of failing the swap, since the local claim and the executed marker already stop a repeat run. Tests are mutation-verified: each guard was broken in turn to confirm the covering test fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The standard route keys its per-quote single-flight lock on quoteId, so a submission without one claims duplicate protection and has none. The previous commit warned and broadcast anyway, reasoning that the local claim already stops a repeat run. That was the wrong trade: the local claim is a file under the quotes dir, so it is per-machine and does not replace a server-side lock, and warn-and-proceed contradicts the principle this route is built on -- executionRoute() refuses an unrecognised value for exactly the same reason. Degrade loudly or not at all. Every live /quote response carries response.metadata.quoteId (confirmed across Base, Solana and cross-chain quotes), so reaching this is shape drift or a hand-edited quote file, never a routine swap. The cost of stopping is a re-quote; the cost of proceeding is an unprotected broadcast. Checked next to the route resolution rather than at the broadcast call, so it refuses before anything is signed instead of being swallowed by the candidate loop and resurfacing as ALL_QUOTES_FAILED. That also makes the standard route's branch at the broadcast site unreachable, so the per-quote fallback is written plainly as the legacy-only path it now is rather than kept as a guard no test can distinguish. Test fixtures for this route now carry a backend quote id, matching what a live response always returns; the drift case is built explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…laim when nothing was broadcast Two more shapes the standard route handled wrongly, both reachable only because the test harness autofilled a transaction hash onto every success response and so never built them. A success body with no txHash was reported as a completed swap. There is nothing to print, nothing to poll and nothing the user can check on an explorer. EVM papered over it by deriving its own hash from the signed bytes; Solana has no equivalent step on this path, so it printed a successful trade with an undefined hash and an explorer link to match. The request WAS accepted, so this is ambiguous rather than failed: reject it in the response-translation layer, next to the 409 rule, as a fail-closed BROADCAST_FAILED that marks the quote spent and aborts. Separately, an explicit "failed, no hash" response left the quote claimed. swapHandedOff is set before the POST, so when every candidate came back that way the finalizer kept <quote>.executing.json and the next execute was refused with "claimed by another execution" -- for a quote that was never broadcast and is still good. The candidate fallback already trusts that answer enough to go sign another transaction against this quote, so it should trust it enough to hand the claim back. It does now, guarded by a sticky flag so a clean answer from one candidate cannot speak for an earlier one that threw mid-POST without ever saying what it did. The harness now takes noHash to build these shapes, and all three new guards are mutation-verified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ection executeTransaction has already classified every ambiguous outcome by the time it reaches the generic !res.ok branch: a network error, a truncated body and ANY 5xx are BROADCAST_FAILED, and a standard-route 409 is carved out. What is left is a parseable 4xx error body, which that branch's sibling already calls what it is -- "a definitive edge/backend rejection, so it stays nonfatal and leaves the quote reusable". It did not leave the quote reusable. swapHandedOff is set before the POST, so the finalizer kept the claimed quote file and the next execute was refused with "claimed by another execution" for a quote the backend had explicitly rejected and never broadcast. The classification was right and the claim silently disagreed with it. Tag both definitive branches with broadcastRuledOut and hand the claim back when one is raised, still subject to the sticky ambiguity flag so a later candidate's clean rejection cannot speak for an earlier attempt that threw without saying what it did. That sticky flag now has a real test. The previous one asserted it using a 400 as the ambiguous candidate, which this commit makes definitive; the surviving ambiguous-and-continuing path is a WalletConnect send that threw on the legacy route, so the test drives that instead and lets the next candidate fall through to the shared POST. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he standard body The quote command stores the chain exactly as the user typed it, and only resolveChain() lowercases on the way through. Every other lookup is a direct index into a lowercase-keyed map, so a capitalised spelling broke in two places. In the standard-route body, an uppercase destination silently dropped toChainId and produced a misclassified cross-chain request -- no error, just a wrong request. That is the one that matters: it is corruption rather than a failure. An uppercase source chain does not reach that code at all. It fails earlier and pre-existing, at the RPC registry, so trade quote --chain Base would quote fine and then never execute: "No RPC URL configured for chain: Base". That is wider than this route, so the chain is normalised once where the quote is loaded, which fixes both routes and quotes already on disk. Found by writing the regression test for the first case and watching it fail on the second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@nansen-pr-reviewer[bot] re-review |
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.
What
Adds
NANSEN_TRADING_EXECUTION_ROUTE=standard, which broadcasts a swap through the execution endpoint that tracks the on-chain outcome server-side. The default is unchanged — without the env var, behaviour is byte-identical to today.On the standard route the command:
source, wallet address, cross-chain fields) and normalises the response back to the legacy result shape, so the call sites and the output block are untouchedrevertReasonalongside the backend's typed code — the route returns a static message, so readingmessagealone dropped the only informative field409 DUPLICATE_EXECUTIONas fatal rather than retryable: the route holds a per-quote single-flight lock, so a 409 means the first attempt is in flight or already produced a hashBroadcaster:(this route does not report one)NANSEN_TRADING_EXECUTION_ROUTEis validated, not defaulted: a typo likeStandardor a stray space is rejected. Silently falling back to legacy is the worst failure mode during a staged rollout — you believe you exercised the new route and you did not. It resolves once, before any signing.Gasless stays on the legacy route deliberately: the standard route has no gasless envelope, so a signed Relay authorization sent there would lose its framing and submit as a plain swap.
Testing
Unit tests cover the body translation, response normalisation, failure-reason precedence, the 409 path, route validation, and the terminal-on-failure behaviour. They are mutation-verified — each guard was broken in turn to confirm the covering test fails.
Verified end to end against the live endpoint with real funds, with the route forced on. All three swap groups passed, and each leg was confirmed on-chain rather than from the assertions alone:
Endpoint selection was confirmed directly rather than inferred, by running
trade executebehind a request-logging proxy: with the env var set the CLI posts to the standard execution path with the expected body; with it unset it posts to the legacy path. Request and response shapes were checked against the trading service schemas rather than guessed.Also in this PR
The last commit is an unrelated, pre-existing fix. All three e2e groups gated on
wallet listprinting anEVM:/Solana:summary, but that command moved to the JSON envelope in #584 (extended in #727), so the preconditions could never pass and the sequential groups never ran. It now parses the envelope. Happy to split this out if you'd rather — it is test-only, but without it the route cannot be exercised e2e at all.🤖 Generated with Claude Code