From e44374b2b6f5bae5baa7c5b9a8a66c04feddb337 Mon Sep 17 00:00:00 2001 From: Vladimir Rogojin Date: Mon, 4 May 2026 14:16:56 +0200 Subject: [PATCH 1/3] feat(trader): add 'sphere trader withdraw' subcommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the gap noted in trader-service PR-17 recovery review: trader-service's ACP WITHDRAW_TOKEN handler exists, but no operator-facing CLI exposed it. Operators couldn't pull funds back out of their trader. Adds: - 'sphere trader withdraw --asset --amount --to-address ' invokes WITHDRAW_TOKEN over ACP. Three required flags mirror the trader-side validation (asset non-empty, amount > 0 positive integer string, to_address valid). - buildWithdrawParams pure function (exported for unit testing). - 8 unit tests pinning the wire-shape contract: snake_case wire fields, bigint-string amount preservation (no Number coercion / precision loss), strict positive-integer amount validation (rejects 0, negative, decimal, '1e6', leading-zero, non-numeric), all three address forms accepted (@nametag, DIRECT://hex, raw hex). - Subcommand-tree test now asserts 7 controller-scoped subcommands (was 6 — withdraw is the 7th). Verified: 114/114 tests pass (was 106, +8 new wire-shape tests). typecheck + lint clean. The same withdraw subcommand is added to trader-service's bundled trader-ctl in vrogojin/trader-service PR #17, so direct-docker e2e tests can also exercise the full deposit→trade→withdraw flow. --- src/trader/trader-commands.test.ts | 74 +++++++++++++++++++++++++++++- src/trader/trader-commands.ts | 67 +++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) diff --git a/src/trader/trader-commands.test.ts b/src/trader/trader-commands.test.ts index 465b842..5a1f832 100644 --- a/src/trader/trader-commands.test.ts +++ b/src/trader/trader-commands.test.ts @@ -11,6 +11,7 @@ import { resolveTenantAddress, createTraderCommand, buildCreateIntentParams, + buildWithdrawParams, } from './trader-commands.js'; describe('parseTimeout (trader)', () => { @@ -79,7 +80,7 @@ describe('resolveTenantAddress', () => { }); describe('createTraderCommand', () => { - it('exposes the 6 controller-scoped trader subcommands', () => { + it('exposes the 7 controller-scoped trader subcommands', () => { const trader = createTraderCommand(); const names = trader.commands.map((c) => c.name()).sort(); // No `status` here: STATUS is system-scoped per Unicity @@ -92,6 +93,7 @@ describe('createTraderCommand', () => { 'list-intents', 'portfolio', 'set-strategy', + 'withdraw', ]); }); @@ -287,3 +289,73 @@ describe('buildCreateIntentParams (wire shape)', () => { expect(result.params['volume_max']).toBe(huge); }); }); + +describe('buildWithdrawParams (wire shape)', () => { + const baseOpts = { + asset: 'UCT', + amount: '1000', + toAddress: '@alice', + }; + + it('emits asset / amount / to_address (snake_case wire fields)', () => { + const result = buildWithdrawParams(baseOpts); + if ('error' in result) throw new Error(result.error); + expect(result.params['asset']).toBe('UCT'); + expect(result.params['amount']).toBe('1000'); + expect(result.params['to_address']).toBe('@alice'); + expect(result.params).not.toHaveProperty('toAddress'); + }); + + it('preserves bigint-string amount verbatim (no coercion to Number)', () => { + const huge = '99999999999999999999999999'; + const result = buildWithdrawParams({ ...baseOpts, amount: huge }); + if ('error' in result) throw new Error(result.error); + expect(result.params['amount']).toBe(huge); + }); + + it('rejects empty asset', () => { + const result = buildWithdrawParams({ ...baseOpts, asset: '' }); + expect('error' in result).toBe(true); + if (!('error' in result)) return; + expect(result.error).toMatch(/asset is required/); + }); + + it('rejects whitespace-only asset', () => { + const result = buildWithdrawParams({ ...baseOpts, asset: ' ' }); + expect('error' in result).toBe(true); + }); + + it('rejects empty amount', () => { + const result = buildWithdrawParams({ ...baseOpts, amount: '' }); + expect('error' in result).toBe(true); + if (!('error' in result)) return; + expect(result.error).toMatch(/amount is required/); + }); + + it('rejects zero, negative, decimal, scientific notation, leading-zero amounts', () => { + for (const bad of ['0', '-100', '1.5', '1e6', '01000', 'abc']) { + const r = buildWithdrawParams({ ...baseOpts, amount: bad }); + expect('error' in r, `expected error for amount="${bad}"`).toBe(true); + } + }); + + it('rejects empty to-address', () => { + const result = buildWithdrawParams({ ...baseOpts, toAddress: '' }); + expect('error' in result).toBe(true); + if (!('error' in result)) return; + expect(result.error).toMatch(/to-address is required/); + }); + + it('accepts all three address forms (@nametag, DIRECT://hex, raw hex)', () => { + for (const addr of [ + '@alice', + 'DIRECT://0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef', + '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef', + ]) { + const r = buildWithdrawParams({ ...baseOpts, toAddress: addr }); + expect('error' in r, `expected accept for to-address="${addr}"`).toBe(false); + if ('error' in r) continue; + expect(r.params['to_address']).toBe(addr); + } + }); +}); diff --git a/src/trader/trader-commands.ts b/src/trader/trader-commands.ts index 237c917..d4b159e 100644 --- a/src/trader/trader-commands.ts +++ b/src/trader/trader-commands.ts @@ -65,6 +65,12 @@ interface SetStrategyOpts { trustedEscrows?: string; } +interface WithdrawOpts { + asset: string; + amount: string; + toAddress: string; +} + // ============================================================================= // Helpers // ============================================================================= @@ -334,6 +340,57 @@ async function handlePortfolio(cmd: Command): Promise { // rely on `sphere trader portfolio`/`list-intents` succeeding as an // implicit liveness signal. +/** + * Build the wire payload for WITHDRAW_TOKEN. Pure function for unit-testing. + * + * Validation here mirrors the trader-side handler in + * trader-service/src/trader/trader-command-handler.ts:633: asset must be + * non-empty, amount must be a positive bigint string, to_address must + * be a valid address. The trader re-validates everything; we catch + * the trivial cases at the CLI layer for a faster local error path. + */ +export function buildWithdrawParams( + opts: WithdrawOpts, +): { readonly params: Record } | { readonly error: string } { + if (!opts.asset || opts.asset.trim() === '') { + return { error: '--asset is required (e.g. UCT, USDU, or hex coin id)' }; + } + if (!opts.amount) { + return { error: '--amount is required (positive integer in smallest units)' }; + } + // Strict positive-integer parse — reject `1e6`-style scientific + // notation, leading zeros, signs, decimals. The trader uses + // safeParseBigint which is similarly strict. + if (!/^[1-9]\d*$/.test(opts.amount)) { + return { + error: `--amount must be a positive integer in smallest units (got "${opts.amount}")`, + }; + } + if (!opts.toAddress || opts.toAddress.trim() === '') { + return { error: '--to-address is required' }; + } + return { + params: { + asset: opts.asset, + amount: opts.amount, + to_address: opts.toAddress, + }, + }; +} + +async function handleWithdraw(cmd: Command, opts: WithdrawOpts): Promise { + await runWithTransport(cmd, async ({ transport, json }) => { + const built = buildWithdrawParams(opts); + if ('error' in built) { + writeStderr(built.error); + process.exitCode = 1; + return; + } + const response = await transport.sendCommand('WITHDRAW_TOKEN', built.params); + emitResult(json, response); + }); +} + async function handleSetStrategy(cmd: Command, opts: SetStrategyOpts): Promise { await runWithTransport(cmd, async ({ transport, json }) => { const params: Record = {}; @@ -439,6 +496,16 @@ export function createTraderCommand(): Command { await handleSetStrategy(this, opts); }); + trader + .command('withdraw') + .description('Withdraw a token from the trader to an external address (ACP WITHDRAW_TOKEN)') + .requiredOption('--asset ', 'Asset symbol (e.g. UCT, USDU) or hex coin id') + .requiredOption('--amount ', 'Amount to withdraw in smallest units (string-encoded bigint, must be > 0)') + .requiredOption('--to-address
', 'Destination address: @nametag, DIRECT://hex, or 64-char hex pubkey') + .action(async function (this: Command, opts: WithdrawOpts) { + await handleWithdraw(this, opts); + }); + // Attach the shared-options help text to every subcommand. for (const sub of trader.commands) { sub.addHelpText('after', `\n${inheritedHelp}`); From 3bcd22a9e7549b697f7a66f9b96c702ac6770fd1 Mon Sep 17 00:00:00 2001 From: Vladimir Rogojin Date: Mon, 4 May 2026 14:32:34 +0200 Subject: [PATCH 2/3] fix(trader): trim --asset and --to-address before forwarding to wire Steelman finding (companion to trader-service fix): a leading or trailing space in --asset or --to-address would pass the empty-after-trim CLI guard but reach the trader's address regex verbatim, producing a confusing INVALID_PARAM remote error rather than a local CLI message. trim() at the wire boundary normalizes valid inputs without changing the validation contract (empty-after- trim was and still is rejected at the CLI layer above). The trader-side regex fix (accepts @nametag, DIRECT://hex, and raw hex pubkey rather than only alphanumeric) lands in trader-service's recover/merge-real-settlement branch. Together the two fixes make all three address forms advertised in --help actually work end-to-end. Verified: 114/114 tests pass. --- src/trader/trader-commands.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/trader/trader-commands.ts b/src/trader/trader-commands.ts index d4b159e..7f46abc 100644 --- a/src/trader/trader-commands.ts +++ b/src/trader/trader-commands.ts @@ -369,11 +369,16 @@ export function buildWithdrawParams( if (!opts.toAddress || opts.toAddress.trim() === '') { return { error: '--to-address is required' }; } + // Trim before forwarding so a leading/trailing space in the operator's + // input doesn't reach the wire (where the trader's address regex + // would reject it as INVALID_PARAM with a remote-side error rather + // than a local CLI message). The empty-after-trim cases were already + // caught above; this pass just normalizes content of valid inputs. return { params: { - asset: opts.asset, + asset: opts.asset.trim(), amount: opts.amount, - to_address: opts.toAddress, + to_address: opts.toAddress.trim(), }, }; } From 5c06cca9481f36190b2b75cfef04bfc127831c1a Mon Sep 17 00:00:00 2001 From: Vladimir Rogojin Date: Mon, 4 May 2026 14:42:52 +0200 Subject: [PATCH 3/3] fix(trader): drop misleading 'or 64-char hex pubkey' from --to-address help sphere-sdk's parseAddress does NOT accept bare hex without a DIRECT://, PROXY://, or @ prefix. The CLI advertised three forms but only two would actually work. trader-service's strict address validator (now layered on top of parseAddress + isValidNametag, plus a hex regex for DIRECT/PROXY) makes the contract explicit. Verified: 114/114 tests pass. --- src/trader/trader-commands.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trader/trader-commands.ts b/src/trader/trader-commands.ts index 7f46abc..a25139c 100644 --- a/src/trader/trader-commands.ts +++ b/src/trader/trader-commands.ts @@ -506,7 +506,7 @@ export function createTraderCommand(): Command { .description('Withdraw a token from the trader to an external address (ACP WITHDRAW_TOKEN)') .requiredOption('--asset ', 'Asset symbol (e.g. UCT, USDU) or hex coin id') .requiredOption('--amount ', 'Amount to withdraw in smallest units (string-encoded bigint, must be > 0)') - .requiredOption('--to-address
', 'Destination address: @nametag, DIRECT://hex, or 64-char hex pubkey') + .requiredOption('--to-address
', 'Destination address: @nametag (lowercase alphanumeric + _/-, 1-30 chars), DIRECT://<64-80 hex>, or PROXY://<64-80 hex>') .action(async function (this: Command, opts: WithdrawOpts) { await handleWithdraw(this, opts); });