Skip to content

feat(trade): add --dry-run and TTY confirmation (--yes) to trade and bridge execute - #667

Merged
gulshngill merged 9 commits into
mainfrom
feat/trade-execute-confirm-dry-run
Sep 21, 2026
Merged

gulshngill merged 9 commits into
mainfrom
feat/trade-execute-confirm-dry-run

Conversation

@gulshngill

@gulshngill gulshngill commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

nansen trade execute and nansen bridge execute move funds irreversibly, but until now there was no way to preview what they would send and no acknowledgement step before broadcasting. This PR adds both, to each command.

--dry-run runs the same sign-free validation a real execute runs, prints the trade or transfer that would be sent, and stops before anything is signed:

  Trade plan — Base
    Quote:          1708900000000-abc123 via lifi (quote 1 of 2)
    Sell:           1000000 base units of 0x833589fc…
    Buy:            ~440000000000000 base units of 0xeeee…
    Max spend:      1000000 base units
    Wallet:         0xAbC0…0001
    Trading fee:    $0.01
    Network fee:    $0.02
    Approval:       required → 0x1231deb6… (scoped to 1000000 base units)
    Outcome check:  on, before broadcast

  DRY RUN — nothing was broadcast. No transaction was signed and the quote was not consumed.

The gate sits before any signing material is loaded, so a dry run needs no wallet password, cannot sign anything, and leaves the quote reusable. Bridge dry-runs validate and use the public signer address persisted in the cached quote, so they can also render and preflight the plan when no local EVM wallet is configured. On EVM, the preview reads the current token allowance when applicable and runs the sign-free transaction checks. On a multi-step bridge transfer it validates every step and stops before the first broadcast.

Confirmation. When stdin is an interactive terminal, both commands now print that plan and ask Broadcast this transaction? [y/N] before broadcasting. Anything but y/yes aborts with nothing signed. --yes (-y), or NANSEN_YES=1, skips the question.

stdin --yes / NANSEN_YES=1 Behaviour
Terminal no Prints the plan, asks Broadcast this transaction? [y/N]; declining aborts
Terminal yes Broadcasts, no prompt
Not a terminal (agents, CI, pipes) no Unchanged — broadcasts without prompting
Not a terminal yes Unchanged — broadcasts without prompting (--yes is a no-op)

Non-interactive behaviour is deliberately identical to today's, so existing automation keeps working untouched and --yes is always safe to pass. The plan and prompt are only emitted in dry-run mode or immediately before an interactive prompt. For an interactive confirmation both are emitted to stderr, so redirecting stdout does not hide the plan from the person answering on stdin; dry-run output remains on stdout for automation.

Exit codes (trade execute, bridge execute):

Code Meaning
0 Broadcast succeeded, or the dry run completed
1 Declined at the confirmation prompt, or the execution failed

Changes

  • src/execute-guard.js (new): flag/env resolution, plan rendering, the TTY-gated prompt, and the single guardExecution() entry point both commands call.
  • src/trading.js: buildTradeExecutionPlan() plus the gate in trade execute, placed after quote selection and before wallet credentials, signing, and the first approval transaction.
  • src/bridge.js: bridge plan builder plus the gate in bridge execute, before signing credentials and the first step. Dry-run validates the persisted quote signer before falling back to local wallet resolution. The fee/nonce override line is rendered once and reused by the plan and the EVM leg.
  • src/cli.js: --yes added to the valueless-flag list (--dry-run was already there); trade and bridge help updated; command interactivity explicitly follows stdin while output formatting continues to follow stdout.
  • src/schema.json: dry-run and yes documented on both execute commands, with examples.
  • Docs: README.md and skills/nansen-trading/SKILL.md gain the flags, the TTY/non-TTY table and the exit codes; NANSEN_YES added to the skill's environment table.
  • Tests: src/__tests__/execute-guard.test.js and src/__tests__/bridge-execute-guard.test.js contain 44 focused tests. Every signing/broadcast seam throws when it must not run (and dry-run fixtures include missing wallet configuration and passwords), so a dry run or declined confirmation that reached signing would fail the suite rather than pass quietly. All network and RPC calls are mocked.
  • Changeset: minor.

Validation

Latest focused validation after merging current main:

$ npm test -- --run <bridge/trade/guard/wallet-export/schema suites>
 Test Files  19 passed (19)
      Tests  572 passed (572)

$ npm run lint
> eslint .
(no output — clean)

Checklist

  • Relevant tests pass
  • src/schema.json updated if new commands or flags were added
  • README.md updated if new top-level commands or categories were added
  • Changeset added (.changeset/) — only required for user-facing changes (new commands, flags, bug fixes, breaking changes)

🤖 Generated with Claude Code

…cute

`trade execute` and `bridge execute` broadcast irreversible on-chain
transactions with no way to preview them and no acknowledgement step.

--dry-run runs the usual validation, prints the trade or transfer that
would be sent, and stops before signing. It is placed before any wallet
material is loaded, so a dry run needs no password, cannot sign anything,
and leaves the quote reusable. On EVM it also reads the current allowance
and runs the pre-broadcast revert simulation when no approval is pending.

When stdin is an interactive terminal, both commands now print that plan
and ask for confirmation before broadcasting; declining exits 1 with
nothing signed. --yes / -y, or NANSEN_YES=1, skips the question.

Behaviour is deliberately unchanged when stdin is not a terminal (agents,
CI, pipes): the command proceeds without prompting, and --yes is accepted
there as a no-op, so existing automation keeps working untouched.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@nansen-pr-reviewer

nansen-pr-reviewer Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

pr-reviewer Summary for #0da13b6

✅ No issues found

The code review completed successfully with no findings.

Review effort: 4/5 (Complex)

Summary

This is a well-executed PR. The --dry-run / --yes gate, the TTY-split (stdin for interactivity, stdout for rendering), and the consent-boundary guarantees are all correctly implemented. The key safety invariants are solid:

  • The gate is placed before wallet credential resolution and before any signing or approval transaction in both trade execute and bridge execute, so a dry run cannot sign anything even if no wallet is configured.
  • Fallback-candidate consent is handled correctly: only candidates that passed sign-free preflight and appeared in the printed plan are eligible for the real broadcast loop (consentedCandidateIndexes). A transient preflight failure cannot cause a candidate to silently recover and broadcast unseen.
  • signer is never null when resolveSigningCredentials(signer) is reached in bridge.js — dry-run always returns false from guardExecution, hitting the return undefined before that line.
  • The CONFIRMATION_UNAVAILABLE path (interactive TTY, no promptFn injected) fails closed with a clear error rather than hanging or proceeding.
  • EOF and Ctrl+C in promptForConfirmation both resolve to '' (treated as "no"), so a closed stdin cannot leave an irreversible command in a hung state.
  • The stdin/stdout TTY split in runCLI correctly preserves the existing isTTY-as-stdout-signal for wallet export, which uses it to decide whether to warn about printing private keys — the isInputTTY seam is layered on top without changing that module's semantics.
  • Compliance screening happens before any dry-run success path and before the interactive confirmation prompt, so a sanctioned wallet cannot obtain a "looks good" plan preview.
  • Token amounts throughout use BigInt consistently; no floating-point leakage.
  • Comments throughout explain why (gate placement, consent scope, preflightPlan lazy evaluation), not just what — no slop.
  • The 43-test suite explicitly proves that signing and broadcast seams throw if reached during dry-run or declined confirmation — failures are loud, not silent.
  • Changeset is correctly marked minor for a new feature addition.

No actionable findings.


Token usage: 8,463 input, 7,572 output, 1,411,977 cache read, 71,983 cache write | Usage Guide

New pushes are reviewed automatically with a 10-minute cooldown between reviews. To request a review at any time, comment @nansen-pr-reviewer re-review.

@gulshngill

Copy link
Copy Markdown
Contributor Author

Merge position: 7 of 9. Merge after #664 and #666.

Shares src/trading.js and skills/nansen-trading/SKILL.md with #664, and src/cli.js, src/schema.json and README.md with #662, #666 and #668. After rebasing onto #666, check the command-surface drift-guard test it adds still passes with the new flags registered here.

Comment thread src/__tests__/execute-guard.test.js Resolved
@gulshngill

Copy link
Copy Markdown
Contributor Author

@nansen-pr-reviewer re-review

@gulshngill

Copy link
Copy Markdown
Contributor Author

@nansen-pr-reviewer re-review

@gulshngill
gulshngill merged commit 8dad3b9 into main Sep 21, 2026
9 checks passed
@gulshngill
gulshngill deleted the feat/trade-execute-confirm-dry-run branch September 21, 2026 15:56
@github-actions github-actions Bot mentioned this pull request Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant