feat: submit a signed message with icp message send - #717
Draft
lwshang wants to merge 3 commits into
Draft
Conversation
The other half of the air-gapped workflow: `icp canister call --sign-only` writes a file on the machine that holds the key, and this submits it from the machine that has the network. A separate command rather than a mode of `call`, because it shares none of `call`'s inputs — canister, method, args, candid, proxy, cycles and query all come from the file — and because `call` resolves an identity and may unlock a key, which is exactly what the submitting machine must not do. It builds an anonymous agent and never looks for a default identity. It reports where now falls in the submission window before anything else, and before anything touches the network, so "not yet valid" reads as the state the signer asked for rather than as a puzzle; a message outside its window is refused with the window named, and with a note that being only just outside it usually means the signing machine's clock has drifted. Everything displayed is decoded from the signed envelope, never read from the file's metadata. `--dry-run` prints the same summary and stops without an agent, a root key, or an interface fetch, so inspecting a file is entirely offline. Submission goes through `update_signed`, falling back to `wait_signed` with the signer's pre-signed status check when the call does not answer synchronously — that fallback is what lets this machine await an outcome with no key, and is why no `poll` or `status` command is needed. Both failure paths carry the recovery advice: re-run on the same file, do not re-sign, since only an identical request id is de-duplicated by the IC. `canister call`'s response rendering moves to `operations/call_output.rs` so both commands decode and print a reply the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Completes the air-gapped call workflow by adding submission of previously signed messages without resolving an identity.
Changes:
- Adds
icp message send, including validation, confirmation, dry-run, submission, and polling. - Shares response rendering with
canister call. - Adds integration tests and documentation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Documents the completed workflow. |
docs/reference/cli.md |
Adds command reference material. |
crates/icp-cli/src/main.rs |
Dispatches message commands. |
crates/icp-cli/src/commands/mod.rs |
Registers the command group. |
crates/icp-cli/src/commands/message/mod.rs |
Defines message subcommands. |
crates/icp-cli/src/commands/message/send.rs |
Implements signed-message submission. |
crates/icp-cli/src/commands/canister/call.rs |
Uses shared rendering and updates signing guidance. |
crates/icp-cli/src/operations/mod.rs |
Registers shared call output operations. |
crates/icp-cli/src/operations/call_output.rs |
Provides shared response decoding and rendering. |
crates/icp-cli/tests/message_send_tests.rs |
Tests submission, retries, validation, and dry-run behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`icp message send` no longer takes `--network` / `--root-key`. Overriding is a rare case, and a courier who genuinely has to redirect a message can edit `network` in the file — those fields are unauthenticated whether a flag writes them or an editor does, so that is inside the trust model rather than a way around it, and the envelope carries no URL, so where a message goes cannot change what executes. Removing them also closes a footgun. `--network` inherits `env = "ICP_NETWORK"`, which is right for every command that chooses where to act but wrong for one whose artifact already decided: a stray shell variable silently redirected a message signed for a local network to mainnet, and with `--yes` in a scripted courier nothing would have caught it. The new test pins that, and that an edited `network` is honoured. The convenience lost is small in a way the design already guarantees: the only realistic reason a recorded URL goes stale is the network moving, and a five-minute submission window means such a message has almost certainly expired anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found that "display only" was doing more work than §4.4 claimed.
The embedded Candid interface is unauthenticated, and Candid's record
subtyping lets a narrower one decode the very same bytes while dropping
fields — so a doctored `.did` renders `(record { to = "alice" })` for an
argument that also says `amount = 1_000`, with the envelope, summary and
request id all still validating. The operator approves what they were
shown, which is not what was signed.
`send` now cross-checks against the untyped decode. That reads the
argument's own type table, which is part of the signed bytes, so it cannot
be tampered with and cannot omit anything; when the readable rendering
surfaces fewer values, both are shown and the readable one is flagged.
The honest case is unchanged and unadorned.
Four smaller findings from the same review:
- The confirmation prompt can sit open past the window, which then spent a
round trip on a message known to be stale. Re-checked after the prompt.
- The clock-drift hint was unreachable when expired: measured against
`valid_from`, the expired side is always more than a whole window away.
It now measures whichever edge was missed.
- A reply that arrived but could not be rendered left the riskiest case —
an executed transfer — without the do-not-re-sign warning that the
changelog promises for every post-submission failure.
- Recovery advice told a `-` caller to re-run a command whose stdin is
already consumed; it now asks for the same bytes instead. The pasteable
path in `--sign-only`'s output goes through `shell_quote`.
A fifth was already answered by dropping `--network`: nothing on the
`--dry-run` path can resolve a root key any more, so it cannot fetch one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Stacked on #716 — review that first; this PR's diff is against its branch, not
main.The other half of the air-gapped workflow. #716 writes a signed call on the machine that holds the key; this submits it from the machine that has the network.
A separate command rather than a mode of
call, because it shares none ofcall's inputs — canister, method, args, candid, proxy, cycles and query all come from the file — and becausecallresolves an identity and may unlock a key, which is precisely what this machine must not do. It builds an anonymous agent and never looks for a default identity.What to look at
Window first, before anything touches the network. A message outside its window is refused before an agent exists, with the window named and a note that being only just outside it usually means the signing machine's clock drifted rather than that you were early. Everything shown is decoded from the signed envelope; the file's metadata is never read for a decision.
--dry-runis genuinely offline — no agent, no root key, nocandid:servicefetch. That is what makes it safe to point at a file you have not decided to send, and it is what the test asserts by dry-running against an unreachable network.wait_signedis the mechanism that makes the whole design work. OnCallResponse::Pollthe reply is awaited using the status check the signer pre-signed, so this machine polls with no key of its own. That is why there is nopollorstatuscommand, and why #716 had to force the call and its status check into one window.Where it submits comes from the file alone. There is deliberately no
--networkoverride: overriding is rare, and a courier who genuinely has to redirect edits the file'snetworkfield — unauthenticated whether a flag writes it or an editor does, so inside the trust model rather than around it. A flag here would also inheritenv = "ICP_NETWORK", which silently redirected a message signed for a local network to mainnet; with--yesin a script, nothing would have caught it.Recovery advice on both failure paths: re-run on the same file, do not re-sign — only an identical request id is de-duplicated by the IC, and a fresh signature means a fresh expiry, hence a different id. For a transfer that is a double spend.
canister call's response rendering moves tooperations/call_output.rs, so both commands decode and print a reply identically.Tests
Round trip for update and query against a live local network — signed inside the project, submitted from outside it, which is what proves no project is needed. Plus
--dry-runsends nothing, tampered summary refused, not-yet-valid refused, expired refused with the closing time.Two the reviewer should know the shape of:
--sign-onlywill not produce an already-closed window. It uses a query, which needs no status check to keep in step.greetwith no mutable state, so there is nothing to count. The IC's de-duplication is what guarantees the rest.🤖 Generated with Claude Code