diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f5f284d..036ee30 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -117,10 +117,59 @@ jobs: - name: Run collar smoke tests run: ${{ steps.detect-pm.outputs.run }} test:collar + cli: + name: CLI + runs-on: ubuntu-latest + defaults: + run: + working-directory: cli + steps: + - name: Checkout + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 + + - name: Setup Node.js + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 + with: + node-version: '22' + cache: npm + cache-dependency-path: | + package-lock.json + cli/package-lock.json + + # The CLI depends on the SDK via `file:..`, and the SDK's package entry + # points at ./dist, which is gitignored. A fresh checkout therefore has + # no SDK build for the CLI to typecheck or run against — build the root + # package first or every step below fails on unresolved types. + - name: Install root SDK dependencies + run: npm ci + working-directory: . + + - name: Build root SDK + run: npm run build + working-directory: . + + - name: Install CLI dependencies + run: npm ci + + - name: Run TypeScript check + run: npm run typecheck + + # Offline only. Covers the wallet-security regressions (config write + # atomicity, approval-target defaults) and the documented exit-code + # contract — all of which are silent failures if they regress. + - name: Run CLI tests + run: npm test + + - name: Build + run: npm run build + + - name: Check build output exists + run: test -f dist/index.js + all-checks: name: All Checks runs-on: ubuntu-latest - needs: [lint, build, collar-tests] + needs: [lint, build, collar-tests, cli] steps: - name: All checks passed run: echo "All checks passed!" diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 9d5345c..22bee51 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -7,6 +7,113 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.0] — 2026-08-25 + +Wallet-security release. Three changes are **breaking** — they turn silent, +irreversible defaults into explicit choices. See *Migration* below. + +### Security + +- **Config writes are now atomic.** `saveConfig` writes to a temp file in the + same directory and `rename(2)`s it into place, matching the pattern already + used by the RFQ key storage. Previously an in-place `writeFileSync` could be + interrupted by a crash, `SIGINT`, or `ENOSPC` and leave a truncated + `config.json`. Because `wallet create` discards the BIP-39 mnemonic and + declares the config file the user's only copy of the key, a torn write there + permanently destroyed funds. A reader now always sees either the complete old + file or the complete new one. + - Side effect, intentional: a symlink at the config path is now **replaced** + rather than written through. This closes the write-side gap left by + `loadConfig`'s `O_NOFOLLOW` read. Users who symlink their config from a + dotfiles repo will find the link severed after the next write — the config + becomes a regular file and the dotfile copy goes stale. + - Not covered: there is no `fsync` before the rename. Process-level crashes + are fully handled; a kernel panic or power loss during the write is not. + +- **`wallet approve` no longer defaults to an unlimited allowance.** + `--amount` is now required. It previously defaulted to `max`, so + `thetanuts wallet approve --token USDC --for optionBook --yes` granted + `MaxUint256` to the spender with no interactive gate — the warning went to + stderr and `--yes` auto-passed the confirm. `--amount max` still works and + still warns. + +- **`rfq request --ensure-allowance` approves the exact escrow on BUY.** + Omitting `--approve-amount` on a BUY now approves exactly `reservePrice`, + the amount the OptionFactory escrows at request time, matching `book fill` + and `position close`. SHORT deliberately still defaults to `MaxUint256`: the + settle-time collateral draw is a structure-dependent max-loss figure that is + not carried on the request (`params.collateralAmount` is hardcoded to 0 by + every SDK builder, and the send path rejects any nonzero value), so there is + no exact amount to approve. Under-approving a SHORT reverts at settlement + *after* a maker has committed, which is worse than a broad allowance. + `--approve-amount ` remains available to cap it. + +- **`--yes` is no longer consent to destroy an existing private key.** + `wallet create` and `wallet import` now exit 2 when the config already holds + a key and `--yes` is passed, instead of overwriting it. The old key has no + other copy on disk and the replacement wallet's mnemonic is not persisted, so + a habitual `--yes` in a script silently burned funds with only a stderr line + to show for it. `wallet create --force` overwrites deliberately; + `wallet import` is interactive by nature and has no `--force`. This matches + the existing rule that `--reveal-key` refuses `--yes`. + +### Fixed + +- **Usage errors exit `2` again.** Commander exits `1` for every parse failure, + but the documented contract reserves `1` for generic runtime errors (network, + RPC, contract revert) and `2` for usage errors. Making `wallet approve + --amount` required therefore produced a `1` where the README promised a `2`, + leaving automation unable to tell a mistyped flag from a reverted transaction. + Commander's usage-error codes are now mapped to `2` centrally across the whole + command tree; `--help` and `--version` continue to exit `0`, and any code the + map does not recognise keeps Commander's own exit code rather than being + silently coerced. + - This also covers a group command invoked with no subcommand — `thetanuts + wallet`, `thetanuts book`, or bare `thetanuts` — which Commander reports as + `commander.help` with exit `1`. That is an incomplete invocation, not a + runtime failure. Successful `--help` uses a separate code and still + exits `0`. + +### Added + +- `cli/tests/approvalDefaults.test.ts` — regression coverage for the + `--ensure-allowance` target defaults and for `saveConfig` atomicity, + permissions, and symlink replacement. No existing test touched these paths. +- `cli/tests/exitCodes.test.ts` — subprocess coverage for the documented exit + codes. Exit status is a process-level contract, so an in-process assertion + cannot observe it. +- **CI now runs the CLI.** The workflow previously installed, built, and tested + only the root SDK, so every CLI test could fail without blocking a merge. A + `cli` job runs typecheck, tests, and build, and `all-checks` depends on it. + The job builds the root SDK first: the CLI resolves the SDK through `file:..` + and the SDK's entry points at a gitignored `dist/`, so a fresh checkout has + nothing for the CLI to compile against. + +### Migration + +- `thetanuts wallet approve --token X --for Y` → add an explicit + `--amount `, or `--amount max` to keep the previous behavior. +- `thetanuts wallet create --yes` / `wallet import --yes` over an existing key + → use `wallet create --force`, or drop `--yes` and confirm interactively. +- Scripts that relied on the leftover unlimited allowance from a BUY + `rfq request --ensure-allowance` for a **subsequent** transaction must now + pass `--approve-amount max` explicitly. + +### Known gaps + +- The private key is still stored **unencrypted** in `config.json`, protected + only by `0600` permissions. That stops other users on the machine; it does + not stop another process running as you (for example a malicious postinstall + script in an unrelated project). An encrypted keystore or OS-keychain option + is not yet implemented. +- `config set privateKey` and `config unset privateKey` still overwrite or + delete the stored key with no confirmation. +- Excess positional arguments are still accepted and ignored rather than + rejected — Commander's `allowExcessArguments` defaults to true and is not + disabled, so `wallet approve extraarg --token USDC --amount 1 --for + optionBook` runs the approve flow instead of erroring. Pre-existing; the + exit-code mapping above does not change it. + ## [0.4.0] — 2026-08-21 ### Added diff --git a/cli/README.md b/cli/README.md index 1bb301f..2573a1e 100644 --- a/cli/README.md +++ b/cli/README.md @@ -794,7 +794,7 @@ thetanuts wallet transfer --token USDC --to 0xRecipient --amount 5.50 | `--token ` | Token symbol (USDC for trading) | | `--spender ` | Explicit spender address | | `--for ` | Alternative: `optionBook` or `optionFactory` | -| `--amount ` | `max` approves MaxUint256 (WARNING printed). Otherwise a decimal. | +| `--amount ` | **Required.** A decimal amount, or `max` for MaxUint256 (WARNING printed). No default — as of 0.5.0 omitting it is an error rather than a silent unlimited approval. | | `--yes` | Skip confirmation prompt | | `--dry-run` | Emit calldata, do not broadcast | @@ -1162,7 +1162,9 @@ fi - **Fill identity.** A successful book fill also renders the created option address, ticker, buyer/seller, and an on-chain `position info` command; `position list` may lag briefly while the indexer catches up. - **`--yes` skips prompts.** Use it in CI / automation only. - **Approvals are never bundled silently with fills** — they require their own confirmation. -- **`max` approvals require explicit opt-in.** `--approve-amount max` (or `wallet approve --amount max`) prints a stderr WARNING and cannot be combined with key-disclosure flags. +- **`max` approvals require explicit opt-in.** `--approve-amount max` (or `wallet approve --amount max`) prints a stderr WARNING and cannot be combined with key-disclosure flags. `wallet approve --amount` has no default, and `rfq request --ensure-allowance` approves exactly the escrowed `reservePrice` on a BUY. A SHORT RFQ still defaults to `max`: the settle-time collateral draw is not knowable at request time, and under-approving it reverts at settlement after a maker has committed. +- **`--yes` is not consent to destroy a key.** `wallet create` and `wallet import` refuse `--yes` when the config already holds a private key (exit 2) — the old key is unrecoverable and the new wallet's mnemonic is not persisted. Use `wallet create --force` to overwrite deliberately, or confirm interactively. Same rule as `--reveal-key`, which also refuses `--yes`. +- **The private key is stored unencrypted**, protected only by `0600` file permissions. That stops other users on the machine; it does not stop another process running as you. Config writes are atomic (temp file + rename), so an interrupted write cannot truncate the file that holds it. - **`keys export`/`import` refuse stdin/stdout** to prevent private-key material from landing in shell history or pipe targets. - **HTTPS-only RPC.** The CLI rejects non-HTTPS RPC URLs unless they point at localhost. diff --git a/cli/package.json b/cli/package.json index 1cd0ddf..c0b0f5c 100644 --- a/cli/package.json +++ b/cli/package.json @@ -1,6 +1,6 @@ { "name": "@thetanuts-finance/cli", - "version": "0.4.0", + "version": "0.5.0", "description": "Thetanuts Finance V4 command-line interface", "type": "module", "main": "dist/index.js", diff --git a/cli/src/commands/rfq.ts b/cli/src/commands/rfq.ts index 62c9dc6..ebd16e7 100644 --- a/cli/src/commands/rfq.ts +++ b/cli/src/commands/rfq.ts @@ -1523,8 +1523,38 @@ function checkOfferDeadlineFuture(req: RFQRequest): void { * path stays a soft stderr advisory and proceeds with the request. * * With `--ensure-allowance`, confirm prompt for the approval, then - * `ensureAllowance(MaxUint256)` (or the user's `--approve-amount`). + * `ensureAllowance()`. BUY defaults to the exact `reservePrice` the contract + * escrows at request time; SHORT defaults to MaxUint256 because the settle-time + * collateral draw is not knowable from the request. `--approve-amount` + * overrides either with a decimal amount or an explicit `max`. */ +/** + * Decide what allowance amount `--ensure-allowance` should target. + * + * Pure and exported so the direction-dependent default is unit-testable — + * a regression here is silent (an under-approval just reverts later, at + * settlement), so it must not depend on running a live RFQ to observe. + * + * @param approveAmount raw `--approve-amount` flag ('max' | decimal | undefined) + * @param isBuy `params.isRequestingLongPosition` + * @param reservePrice `req.reservePrice` — what the factory escrows for a BUY + * @param parseCustom converts a decimal flag value to token units + */ +export function resolveApproveTarget( + approveAmount: string | undefined, + isBuy: boolean, + reservePrice: bigint, + parseCustom: (value: string) => bigint +): { target: bigint; isMax: boolean } { + if (approveAmount === 'max') return { target: MaxUint256, isMax: true }; + if (approveAmount !== undefined) return { target: parseCustom(approveAmount), isMax: false }; + // Defaults. BUY escrows exactly reservePrice at request time, so approve + // exactly that. SHORT's settle-time draw is not derivable from the request + // (see maybeEnsureCollateralAllowance), so it stays unlimited rather than + // under-approving and reverting after a maker has already committed. + return isBuy ? { target: reservePrice, isMax: false } : { target: MaxUint256, isMax: true }; +} + async function maybeEnsureCollateralAllowance( result: GetClientResult, req: RFQRequest, @@ -1540,20 +1570,36 @@ async function maybeEnsureCollateralAllowance( const minRequired = isBuy ? req.reservePrice : 0n; const directionLabel = isBuy ? 'BUY' : 'SHORT'; - // Compute target allowance amount (what we approve TO, if --ensure-allowance is set) - let target: bigint; - let isMax = false; - if (flags.approveAmount === undefined || flags.approveAmount === 'max') { - target = MaxUint256; - isMax = true; - } else { - const decimals = Number(await client.erc20.getDecimals(req.params.collateral)); - target = client.utils.toBigInt(flags.approveAmount, decimals); - } + // Compute target allowance amount (what we approve TO, if --ensure-allowance is set). + // + // Omitting --approve-amount used to mean MaxUint256 on BOTH directions, so a + // scripted `rfq request --ensure-allowance --yes` granted the factory an + // unlimited allowance non-interactively. BUY now defaults to the exact + // reservePrice the contract escrows at request time, matching `book fill` + // and `position close`. + // + // SHORT deliberately still defaults to MaxUint256. The collateral pulled at + // settle is a structure-dependent max-loss figure that is NOT carried on the + // request — `params.collateralAmount` is hardcoded to 0 by every SDK builder + // and the send path rejects any nonzero value ('collateral is pulled by the + // contract'), so there is no exact bigint to approve here. Under-approving a + // SHORT reverts at settlement AFTER a maker has committed, which is a worse + // outcome than a broad allowance; the MaxUint256 warning below still fires, + // and `--approve-amount ` remains available for users who want to cap it. + const decimals = + flags.approveAmount !== undefined && flags.approveAmount !== 'max' + ? Number(await client.erc20.getDecimals(req.params.collateral)) + : 0; + const { target, isMax } = resolveApproveTarget( + flags.approveAmount, + isBuy, + req.reservePrice, + (value) => client.utils.toBigInt(value, decimals) + ); if (target < minRequired) { throw new Error( `--approve-amount (${target.toString()}) is less than the contract's required ` + - `escrow at request time (${minRequired.toString()}). Increase --approve-amount or omit it (defaults to MaxUint256).` + `escrow at request time (${minRequired.toString()}). Increase --approve-amount, omit it to approve exactly what is required, or pass --approve-amount max.` ); } @@ -1578,7 +1624,7 @@ async function maybeEnsureCollateralAllowance( `BUY RFQ: current allowance on ${req.params.collateral} → OptionFactory (${spender}) ` + `is ${current.toString()}, but the contract will escrow ${minRequired.toString()} ` + `(your reserve-price total) at request time. Either:\n` + - ` • Pass --ensure-allowance to approve in-flow (uses MaxUint256 by default), or\n` + + ` • Pass --ensure-allowance to approve in-flow (approves exactly this reserve-price amount by default), or\n` + ` • Pre-approve manually: thetanuts wallet approve --token --for optionFactory --amount 5` ); (err as Error & { exitCode?: number }).exitCode = 4; @@ -1632,7 +1678,7 @@ function registerRequest(grp: Command): void { ) .option( '--approve-amount ', - 'amount to ensure-allowance to (default: max = MaxUint256; or a decimal token amount). Only used with --ensure-allowance.' + 'allowance amount when --ensure-allowance fires. Default: exact reservePrice for BUY, "max" for SHORT (settle-time collateral is not known at request time). Pass "max" for unlimited (MaxUint256), or a decimal token amount.' ) .option( '--pay-with ', diff --git a/cli/src/commands/wallet.ts b/cli/src/commands/wallet.ts index 463d34d..6515e00 100644 --- a/cli/src/commands/wallet.ts +++ b/cli/src/commands/wallet.ts @@ -320,7 +320,7 @@ export function register(program: Command): void { 'The private key is NEVER printed unless --reveal-key is passed (with confirmation). ' + 'For a one-time backup, use --reveal-key or copy the config file to a secure location.' ) - .option('--force', 'overwrite an existing private key without prompting', false) + .option('--force', 'overwrite an existing private key (required when one exists; --yes is not accepted as consent)', false) .option( '--reveal-key', 'after saving, display the private key and BIP-39 mnemonic on stdout. Will appear in scrollback — pipe to a secure destination.', @@ -343,10 +343,26 @@ export function register(program: Command): void { const path = (opts.config as string | undefined) ?? defaultConfigPath(); const existing = loadConfig(path); if (existing?.privateKey && !local.force) { + // Deliberately NOT passing `yes: opts.yes`. Overwriting destroys the + // only copy of an existing key (the mnemonic was never persisted), + // and the replacement wallet's own mnemonic is discarded too — so a + // habitual `--yes` in a script silently burned the user's funds with + // nothing but a stderr line to show for it. Key destruction now + // requires the dedicated `--force` flag, the same way `--reveal-key` + // refuses to accept `--yes` as consent for disclosure. + if (opts.yes) { + process.stderr.write( + `Config at ${path} already has a private key (${maskPrivateKey(existing.privateKey)}).\n` + + '`--yes` does not authorize overwriting an existing key: the old key is unrecoverable ' + + 'and the new wallet\'s mnemonic is not persisted. Re-run with `--force` to overwrite ' + + 'deliberately, or point --config at a different path.\n' + ); + process.exit(2); + } const ok = await confirm( `Config at ${path} already has a private key (${maskPrivateKey(existing.privateKey)}). Overwrite with a NEW random wallet? ` + `The existing key cannot be recovered after overwrite.`, - { yes: Boolean(opts.yes), dryRun: Boolean(opts.dryRun) } + { yes: false, dryRun: Boolean(opts.dryRun) } ); if (!ok) process.exit(3); } else if (existing?.privateKey && local.force) { @@ -491,9 +507,23 @@ export function register(program: Command): void { const path = (opts.config as string | undefined) ?? defaultConfigPath(); const existing = loadConfig(path); if (existing?.privateKey) { + // Same rule as `wallet create`: `--yes` is not consent to destroy an + // existing key. The old key has no other copy on disk, so a scripted + // import that blanket-answers prompts must not be able to silently + // discard it. There is no `--force` on import (the flow is inherently + // interactive — it prompts for the key), so this is a hard refusal. + if (opts.yes) { + process.stderr.write( + `Config at ${path} already has a private key (${maskPrivateKey(existing.privateKey)}).\n` + + '`--yes` does not authorize overwriting an existing key — it is unrecoverable once replaced. ' + + 'Re-run without `--yes` to confirm interactively, or point --config at a different path.\n' + ); + process.exit(2); + } const ok = await confirm( - `Config at ${path} already has a private key (${maskPrivateKey(existing.privateKey)}). Overwrite?`, - { yes: Boolean(opts.yes), dryRun: Boolean(opts.dryRun) } + `Config at ${path} already has a private key (${maskPrivateKey(existing.privateKey)}). Overwrite? ` + + 'The existing key cannot be recovered after overwrite.', + { yes: false, dryRun: Boolean(opts.dryRun) } ); if (!ok) process.exit(3); } @@ -554,7 +584,14 @@ export function register(program: Command): void { .requiredOption('--token ', 'token symbol or address') .option('--spender ', 'spender address') .option('--for ', 'preset spender: optionBook | optionFactory') - .option('--amount ', 'amount or "max"', 'max') + // No default. This previously defaulted to "max", so a bare + // `wallet approve --token USDC --for optionBook --yes` silently granted an + // unlimited (MaxUint256) allowance with no interactive gate. Every other + // approval path in the CLI (`book fill`, `position close`, `rfq request + // --ensure-allowance`) defaults to the exact amount required; this command + // has no trade context to derive an exact amount from, so it now forces the + // caller to state one. `--amount max` still works, and still warns. + .requiredOption('--amount ', 'decimal amount to approve, or "max" for unlimited (MaxUint256)') .action(async (_localOpts: unknown, cmd: Command) => { const opts = getGlobalOpts(cmd); const local = cmd.opts<{ diff --git a/cli/src/config.ts b/cli/src/config.ts index efd7340..5b7f94e 100644 --- a/cli/src/config.ts +++ b/cli/src/config.ts @@ -1,6 +1,7 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; +import { randomBytes } from 'node:crypto'; /** * Persisted CLI configuration. Stored at `~/.config/thetanuts/config.json` by @@ -80,15 +81,38 @@ export function saveConfig(cfg: Config, configPath?: string): void { if (!fs.existsSync(dir)) { fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); } - fs.writeFileSync(p, JSON.stringify(cfg, null, 2) + '\n', { mode: 0o600 }); - // `mode: 0o600` on writeFileSync is honored only on file creation. Force - // tight perms unconditionally so we tighten any pre-existing loose mode. - // Same applies to the parent directory + + // Write atomically: temp file in the same directory, then rename(2). + // + // `wallet create` discards the BIP-39 mnemonic and tells the user this file + // is now the ONLY copy of their key. A crash, SIGINT, or ENOSPC partway + // through an in-place writeFileSync would leave a truncated config and + // permanently destroy that key. rename(2) within a filesystem is atomic, so + // a reader sees either the old file or the new one — never a partial write. + // + // Writing to a fresh temp path also means we never follow a symlink planted + // at `p`: rename replaces the link itself rather than writing through it, + // which closes the write-side gap left by the O_NOFOLLOW read in loadConfig. + const tempPath = `${p}.${randomBytes(8).toString('hex')}.tmp`; try { - fs.chmodSync(p, 0o600); - } catch { - // ignore (Windows / unsupported FS) + fs.writeFileSync(tempPath, JSON.stringify(cfg, null, 2) + '\n', { mode: 0o600 }); + // `mode` on writeFileSync is honored only at creation and is masked by + // umask. Force it before the rename so the key is never briefly readable. + try { + fs.chmodSync(tempPath, 0o600); + } catch { + // ignore (Windows / unsupported FS) + } + fs.renameSync(tempPath, p); + } catch (err) { + try { + fs.unlinkSync(tempPath); + } catch { + // ignore cleanup errors — the original file is still intact + } + throw err; } + try { fs.chmodSync(dir, 0o700); } catch { diff --git a/cli/src/index.ts b/cli/src/index.ts index 422395a..fe8c75c 100644 --- a/cli/src/index.ts +++ b/cli/src/index.ts @@ -33,6 +33,45 @@ program.configureHelp({ showGlobalOptions: true }); addGlobalOptions(program); registerCommands(program); +// Commander exits 1 for every parse failure, but the CLI documents exit 2 as +// "usage error (bad flags, missing required arg)" and reserves 1 for generic +// runtime failures — network, RPC, contract revert. Without this mapping, +// automation cannot tell a typo'd flag apart from a reverted transaction. +// +// The set is Commander's own error codes for input the user got wrong. Anything +// unrecognised keeps Commander's exit code rather than being coerced, so a new +// error code in a future Commander release fails visibly instead of silently +// reporting itself as a usage error. +const USAGE_ERROR_CODES = new Set([ + 'commander.missingArgument', + 'commander.missingMandatoryOptionValue', + 'commander.optionMissingArgument', + 'commander.unknownOption', + 'commander.unknownCommand', + 'commander.invalidArgument', + 'commander.excessArguments', + 'commander.conflictingOption', + // A group command invoked with no subcommand (`thetanuts wallet`, or bare + // `thetanuts`) prints help via help({ error: true }), which throws + // `commander.help` with exitCode 1. That is an incomplete command — a usage + // error by the README's own definition — not a runtime failure. Safe to map + // here because successful `--help` uses the distinct `commander.helpDisplayed` + // code with exitCode 0, which the guard below returns on before reaching this. + 'commander.help', +]); + +// exitOverride is per-Command and is NOT inherited by subcommands, so walk the +// whole tree — `thetanuts wallet approve` is a grandchild of the root command. +function mapUsageExitCodes(cmd: Command): void { + cmd.exitOverride((err) => { + // --help and --version are successful terminations, not failures. + if (err.exitCode === 0) process.exit(0); + process.exit(USAGE_ERROR_CODES.has(err.code) ? 2 : err.exitCode); + }); + for (const sub of cmd.commands) mapUsageExitCodes(sub as Command); +} +mapUsageExitCodes(program); + program.parseAsync(process.argv).catch((err) => { // Try to use the structured error renderer if we can detect --json-errors, // otherwise print just the message — never the stack diff --git a/cli/tests/approvalDefaults.test.ts b/cli/tests/approvalDefaults.test.ts new file mode 100644 index 0000000..374972d --- /dev/null +++ b/cli/tests/approvalDefaults.test.ts @@ -0,0 +1,77 @@ +import { strict as assert } from 'node:assert'; +import { MaxUint256 } from 'ethers'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { resolveApproveTarget } from '../src/commands/rfq.js'; +import { saveConfig, loadConfig, type Config } from '../src/config.js'; + +const parse = (v: string): bigint => BigInt(Math.round(Number(v) * 1e6)); + +// ---- rfq --ensure-allowance target defaults ------------------------------ + +// BUY with no --approve-amount: exactly the reservePrice the factory escrows. +{ + const { target, isMax } = resolveApproveTarget(undefined, true, 12_500_000n, parse); + assert.equal(target, 12_500_000n, 'BUY default must be the exact reservePrice'); + assert.equal(isMax, false); +} + +// SHORT with no --approve-amount: MaxUint256. +// +// Regression guard. A previous fix defaulted this to `params.collateralAmount`, +// which every SDK builder hardcodes to 0 (and the send path rejects if nonzero). +// target=0 made `current >= target` always true, so --ensure-allowance became a +// SILENT no-op and settlement reverted after a maker had committed. A zero or +// small target here is the bug, not a stricter policy. +{ + const { target, isMax } = resolveApproveTarget(undefined, false, 0n, parse); + assert.notEqual(target, 0n, 'SHORT default of 0 makes --ensure-allowance a silent no-op'); + assert.equal(target, MaxUint256, 'SHORT default must stay unlimited'); + assert.equal(isMax, true, 'SHORT default must flag isMax so the warning fires'); +} + +// Explicit --approve-amount max on either direction. +for (const isBuy of [true, false]) { + const { target, isMax } = resolveApproveTarget('max', isBuy, 5n, parse); + assert.equal(target, MaxUint256); + assert.equal(isMax, true); +} + +// Explicit numeric amount wins over both defaults and never reports isMax. +for (const isBuy of [true, false]) { + const { target, isMax } = resolveApproveTarget('2.5', isBuy, 999n, parse); + assert.equal(target, 2_500_000n); + assert.equal(isMax, false); +} + +// ---- saveConfig atomicity / permissions ---------------------------------- + +const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'tnu-cfg-')); +const cfgPath = path.join(dir, 'nested', 'config.json'); +const key = '0x' + '11'.repeat(32); +const cfg: Config = { version: 1, chainId: 8453, rpcUrl: 'https://mainnet.base.org', privateKey: key }; + +saveConfig(cfg, cfgPath); +assert.equal(loadConfig(cfgPath)?.privateKey, key, 'roundtrip must preserve the key'); +assert.equal(fs.statSync(cfgPath).mode & 0o777, 0o600, 'config must be 0600'); +assert.equal(fs.statSync(path.dirname(cfgPath)).mode & 0o777, 0o700, 'config dir must be 0700'); +assert.equal( + fs.readdirSync(path.dirname(cfgPath)).filter((f) => f.includes('.tmp')).length, + 0, + 'no temp files may survive a successful save' +); + +// A symlink at the config path must be replaced, not written through — this is +// the write-side counterpart to loadConfig's O_NOFOLLOW read. +const outside = path.join(dir, 'outside.txt'); +fs.writeFileSync(outside, 'ORIGINAL'); +fs.rmSync(cfgPath); +fs.symlinkSync(outside, cfgPath); +saveConfig({ ...cfg, rpcUrl: 'https://example.invalid' }, cfgPath); +assert.equal(fs.readFileSync(outside, 'utf8'), 'ORIGINAL', 'symlink target must not be written through'); +assert.equal(fs.lstatSync(cfgPath).isSymbolicLink(), false, 'symlink must be replaced by a regular file'); + +fs.rmSync(dir, { recursive: true, force: true }); + +console.log('approval defaults + config atomicity tests passed'); diff --git a/cli/tests/exitCodes.test.ts b/cli/tests/exitCodes.test.ts new file mode 100644 index 0000000..c65a202 --- /dev/null +++ b/cli/tests/exitCodes.test.ts @@ -0,0 +1,87 @@ +import { strict as assert } from 'node:assert'; +import { spawnSync } from 'node:child_process'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// Subprocess tests: exit codes are a process-level contract, and the mapping +// lives in the parse path — an in-process call cannot observe it. +// +// README "Exit codes" documents: +// 1 = generic error (network, RPC, contract revert) +// 2 = usage error (bad flags, missing required arg) +// Commander defaults every parse failure to 1, which made a typo'd flag +// indistinguishable from a reverted transaction for anything scripting the CLI. + +const here = path.dirname(fileURLToPath(import.meta.url)); +const cliRoot = path.join(here, '..'); +const entry = path.join(cliRoot, 'src', 'index.ts'); + +/** + * Every case below fails during argument parsing, before any action handler + * runs, so none of them read config or touch the network — no key or RPC + * isolation is needed here. If a case that reaches an action handler is ever + * added, isolate it with `--config `: the CLI has no config + * env var, and setting THETANUTS_PRIVATE_KEY='' does NOT suppress key lookup + * (client.ts uses a truthy check, so '' falls through to the config file). + */ +function run(args: string[]): { code: number; stderr: string; stdout: string } { + const r = spawnSync('npx', ['tsx', entry, ...args], { + encoding: 'utf8', + // Pin cwd so `npx` resolves the local tsx from cli/node_modules. Without + // it, running the suite from another directory sends npx to the registry. + cwd: cliRoot, + env: process.env, + }); + return { code: r.status ?? -1, stderr: r.stderr ?? '', stdout: r.stdout ?? '' }; +} + +// --- usage errors must exit 2 -------------------------------------------- + +// The regression this file exists for: `wallet approve --amount` became a +// required option in 0.5.0, and Commander reported that as exit 1. +{ + const r = run(['wallet', 'approve', '--token', 'USDC', '--for', 'optionBook']); + assert.equal(r.code, 2, `missing --amount must exit 2 (usage), got ${r.code}`); + assert.match(r.stderr, /required option .*--amount/, 'must name the missing option'); +} + +for (const args of [ + ['wallet', 'approve'], // missing several required options + ['book', 'fill', '--definitely-not-a-flag'], // unknown option + ['not-a-command'], // unknown command + ['wallet', 'not-a-subcommand'], // unknown subcommand + // A group command with no subcommand is an incomplete invocation, not a + // runtime failure. Commander reports these as `commander.help` with + // exitCode 1 — the one usage-class code that is easy to miss, because it + // shares a name with the successful `--help` path (`commander.helpDisplayed`, + // exitCode 0), which must keep exiting 0. Both directions are asserted here. + ['wallet'], + ['book'], + ['rfq'], + [], // bare `thetanuts` +]) { + const r = run(args); + assert.equal(r.code, 2, `\`${args.join(' ')}\` must exit 2 (usage), got ${r.code}`); +} + +// --- help and version are successful terminations, not failures ---------- + +for (const args of [ + ['--help'], + ['--version'], + ['wallet', '--help'], + ['wallet', 'approve', '--help'], + ['rfq', 'request', '--help'], +]) { + const r = run(args); + assert.equal(r.code, 0, `\`${args.join(' ')}\` must exit 0, got ${r.code}`); +} + +// --version must still print the version, i.e. the exit mapping did not +// swallow Commander's output on the way past. +{ + const r = run(['--version']); + assert.match(r.stdout.trim(), /^\d+\.\d+\.\d+/, 'expected a semver on stdout'); +} + +console.log('exit code contract tests passed');