Richer send confirmation: resolved recipient, balance impact, simulation - #43
Conversation
Before a send executes, show a resolved confirmation block instead of the one-line preview: - recipient resolved to its EIP-55 checksummed form; a mixed-case address whose checksum does not verify is a typo and is rejected *before* the confirmation prompt (format.toChecksumAddress via ethers getAddress) - amount and token, plus the wallet balance before -> after the send - gas expectation labelled by mode (gasless via paymaster / dry-run simulated / you-pay-gas), with the simulated fee where a quote is available - an insufficient balance is surfaced as a WARNING line in the block (the send would revert) rather than blocking dry-run previews - the existing y/N confirm and the mainnet acknowledgement are unchanged; runAction now sends to the checksummed address so it matches the preview New in tools.mjs: previewSend() (async pre-flight, throws on bad checksum) and renderSendPreview(). cli.handleAction builds the preview for send_mon and rejects before the prompt on failure. Tested on Linux (lavapipe Vulkan): dry-run and gasless both render the block; a typo'd-checksum send is refused before the prompt (exit 1, no confirm shown); interactive pty send shows the block then the y/N prompt; npm run smoke SMOKE_OK.
The confirmation block already flagged an insufficient balance; a quoteSend that throws is itself an obvious revert the simulator caught, so show it as a WARNING line before the prompt instead of swallowing it. Keeps the block informative when the estimate path fails, and matches the issue's 'obvious reverts are caught before confirmation'.
|
Rebased onto main after #47 landed; conflicts were import lines only, resolved. Re-verified on the rebased build: npm test 72/72 (including the new send-token and fetch-model suites), npm run smoke SMOKE_OK, and the acceptance checks (dry-run preview block, typo'd checksum refused pre-prompt with exit 1, non-yes cancels). Note: the new send_token action currently keeps the one-line preview; extending the resolved preview block to token sends looks like natural follow-up work once this lands. |
b958c1f to
01bde08
Compare
portdeveloper
left a comment
There was a problem hiding this comment.
I read the full diff at 939d9da plus 01bde08 and checked the rebase against current main (the symbols the diff leans on all exist there: println, hadFailure, wallet.quoteSend, wallet.getBalance, and isWrite now covering send_token). Standing alone the change is good. previewSend in src/tools.mjs throws on a bad mixed-case checksum before the prompt, a failed quoteSend and an insufficient balance both surface as WARNING lines in the block, and scripted mode exits non-zero on a refused preview, which matches the acceptance notes on #24.
I'm requesting changes on one structural point: how this composes with #42, which is approved and will land. Two things break.
previewSend resolves the recipient itself, straight from the raw action.to via toChecksumAddress. Once #42 lands, action.to can be an address-book name like "alice", and getAddress("alice") throws, so every alias send would be refused before the prompt. #42's handleAction resolves once via resolveSend and hands the result down; previewSend needs to take that already-resolved address as input rather than resolving on its own.
handleAction also throws away the address the preview computed: runAction's send_mon case re-derives it with its own toChecksumAddress(a.to) call. Today that is harmless because checksumming an in-memory string is deterministic, but the send path never actually uses the previewed value, and #42's runAction takes the resolved recipient as an argument precisely so nothing re-resolves between the y/N prompt and the signature. Pass preview.to into runAction so there is a single resolution point. With that shape the eventual #42 merge is close to mechanical: resolve once in handleAction, then feed that same address through the preview and the send.
Two smaller notes. The comment in src/cli.mjs handleAction says an over-balance send is "rejected before the confirmation prompt", but the code (correctly, per #24) only prints a WARNING and still prompts, so update the comment. And previewSend's insufficient check compares value > before without the fee, so a native-gas send that only reverts because of gas shows no warning; fine as a follow-up if you note it.
The rebase itself looks clean (import-line conflicts only, as you described), and keeping the one-line preview for send_token as follow-up work is fine with me.
… preview and send Per the review on portdeveloper#43: previewSend now takes the already-resolved checksummed recipient instead of resolving raw action.to itself, so when portdeveloper#42's address book lands, name resolution slots in ahead of the checksum step and alias sends stop dying on getAddress. handleAction resolves once, the preview and runAction both receive that address, and nothing re-resolves between the y/N prompt and the signature (direct runAction callers keep a fallback resolve). Also: corrected the stale over-balance comment (it is a WARNING plus prompt, not a pre-prompt rejection) and noted the fee-exclusive insufficient check as a known follow-up.
|
Done in 1a9274c, point by point:
Re-verified on the revised build: npm test 72/72, smoke SMOKE_OK, and the acceptance checks (preview block renders, typo'd checksum refused pre-prompt with exit 1, non-yes cancels). |
portdeveloper
left a comment
There was a problem hiding this comment.
Re-reviewed the follow-up commit against current main and the requested changes are addressed: the recipient is checksummed once before preview, the exact previewed address is threaded into execution, the stale comment is corrected, and the fee-only insufficiency edge case is explicitly scoped as follow-up. CI is green; locally verified 72/72 tests, production build, and a disposable-wallet dry-run covering the rendered balance warning, dry-run execution, and invalid mixed-case checksum refusal. Approved.
What this changes
Closes #24. Sends now render a resolved confirmation block before the y/N prompt: checksummed recipient, amount + wallet balance after the send, gas expectation (gasless / dry-run / native), and a simulated outcome so obvious reverts surface before confirmation.
format.mjs:toChecksumAddress(EIP-55 via ethersgetAddress) — a typo'd mixed-case checksum throws, so it is refused before the prompt.tools.mjs:previewSend(checksum + balance read + fee quote + post-send balance, best-effort simulation) andrenderSendPreview. A failed simulation is rendered as a revert warning instead of being swallowed; insufficient balance is a warning line, so dry-run previews still render.runActionsends to the checksummed address.cli.mjs:send_monbuilds the preview first and refuses pre-prompt on failure (scripted mode exits non-zero); anything but yes still aborts.Example (dry-run):
Acceptance: both dry-run (no
PIMLICO_API_KEY) and gasless mode show the block (the block is built inhandleActionbefore the gas-mode branch, andpreviewSendlabels each mode);npm run smokeprintsSMOKE_OK; a typo'd checksum (0x92936497b6ad2BA84...— first letter lowercased) is rejected before the confirmation prompt with exit 1 in scripted mode.Rebased on current
main(includes #25 ERC-20 balance reads);npm test33/33 after the rebase.How I tested it
npm run buildsucceedsnpm run smokeprintsSMOKE_OKModel / platform tested on: QWEN3_8B_INST_Q4_K_M · Ubuntu 22.04 headless VPS (lavapipe/llvmpipe Vulkan), Node 20
Scope check
Conventions
npm(not pnpm/yarn) and did not add a globalsodium-nativeoverride..env, seeds, keys, or model weights.