feat(ckbtc-minter): support ICRC-21 consent messages#10093
feat(ckbtc-minter): support ICRC-21 consent messages#10093Dfinity-Bjoern wants to merge 14 commits intomasterfrom
Conversation
Implement the ICRC-21 canister call consent message standard on the
ckBTC minter so that ICRC-21-aware wallets can show the user a human-
readable description of `retrieve_btc_with_approval` (and `retrieve_btc`)
calls before signing.
Both display variants are supported:
- GenericDisplay: a Markdown message for software wallets.
- FieldsDisplay: a list of (label, Value) fields tailored for hardware
wallets like the Ledger Nano S+. Long Text values (Bitcoin addresses,
hex subaccounts) are split into chunks of at most 18 characters with a
"(N/M)" pagination suffix, and labels are kept short ("BTC address",
"From subaccount", "Amount") so they fit on a single hardware-wallet
line.
Also adds an `icrc10_supported_standards` query that advertises
ICRC-10 and ICRC-21.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds ICRC-21 “canister call consent message” support to the ckBTC minter so ICRC-21-aware wallets can display a human-readable preview for retrieve_btc_with_approval and retrieve_btc before signing, and advertises support via an ICRC-10 supported-standards endpoint.
Changes:
- Introduces consent-message construction for
retrieve_btc_with_approvalandretrieve_btcwith bothGenericDisplay(Markdown) andFieldsDisplay(chunked text fields). - Exposes new canister endpoints
icrc21_canister_call_consent_message(update) andicrc10_supported_standards(query). - Extends the public Candid interface (
ckbtc_minter.did) with the ICRC-10/ICRC-21 types and methods.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rs/bitcoin/ckbtc/minter/src/updates/icrc21.rs | Implements ICRC-21 request handling, consent message formatting, chunking helper, and unit tests. |
| rs/bitcoin/ckbtc/minter/src/updates.rs | Exposes the new icrc21 updates module. |
| rs/bitcoin/ckbtc/minter/src/main.rs | Wires the new ICRC-21 (update) and ICRC-10 (query) endpoints into the canister. |
| rs/bitcoin/ckbtc/minter/ckbtc_minter.did | Declares ICRC-10 / ICRC-21 types and the two new service methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review feedback on PR #10093: 1. Token symbol was hard-coded to "ckBTC" / "BTC" but the minter is also deployed against testnet/regtest where the same code drives the ckTESTBTC / TESTBTC tokens (see dashboard.rs and update_balance.rs). Introduce a TokenSymbols struct that picks the right symbols based on the configured Network and thread it through both the GenericDisplay markdown and FieldsDisplay fields so consent messages stay accurate on test deployments. 2. Fix a stale doc comment that claimed push_chunked_text iterates via `char_indices` — it iterates `chars()` (the comment intent stands: we use Unicode-scalar iteration to keep the helper safe for any non-ASCII caller). Add unit tests for the testnet/regtest symbol path on both display variants. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review feedback on PR #10093: replace the local 500-byte copy of the ICRC-21 argument-size limit with the shared constant icrc_ledger_types::icrc21::lib::MAX_CONSENT_MESSAGE_ARG_SIZE_BYTES so the two values cannot drift in the future. The shared constant is a u16, cast to usize at the comparison and test sites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mbjorkqvist
left a comment
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks @Dfinity-Bjoern, just a few minor comments!
| /// Maximum number of characters per `Value::Text` field for the FieldsDisplay | ||
| /// variant. Hardware wallets like the Ledger Nano S+ have a small display and | ||
| /// cannot fit a full Bitcoin address (up to 62 characters for bech32m) or a | ||
| /// 64-character hex subaccount on a single screen. Long values are split into | ||
| /// multiple fields with a "(N/M)" pagination suffix in the label so each chunk | ||
| /// fits on a single screen with the label on top. | ||
| /// | ||
| /// Field labels are kept short (e.g. "BTC address", "From subaccount") so that | ||
| /// even with a "(N/M)" suffix the label still fits on one line at the typical | ||
| /// hardware-wallet font size. |
There was a problem hiding this comment.
The ICRC-21.did canister interface spec describes the FieldsDisplay and GenericDisplay devices as having line-wrapping/pagination capability, which suggests the canister should provide one logical field per value and trust the device to render it. Pre-chunking with (N/M) label suffixes works around the spec's design assumption rather than following it, so if possible, I'd recommend not performing this chunking in the minter.
There was a problem hiding this comment.
Agreed and fixed in 3bf0482 (refactor) + e96631c (rebase). Confirmed by reading the Ledger ICP app: msg.rs:render_item reads the raw Value::Text { content } and passes it to handle_ui_message(text, message, page), which slices the value into out.len()-1-sized device pages (item.chunks(m_len).nth(page)) and returns the total page count so the device-side UI handles scrolling natively. Pre-chunking on the canister side was actively harmful — one logical address turned into 3 separate fields the user had to step through with confusing (1/3) labels, and the device couldn't paginate cleanly. Dropped push_chunked_text / chunk_text / FIELDS_DISPLAY_TEXT_CHUNK_LEN and now emit each long value as a single Value::Text. Short labels (Amount, BTC address, From subaccount) are kept since titles in the Ledger app are truncated rather than paginated.
| message.push_str(&format!( | ||
| "\n\n**Bitcoin destination address:**\n`{}`", | ||
| args.address | ||
| )); |
There was a problem hiding this comment.
nit: There's a possible Markdown injection vector here, where the user could provide args such as retrieve_btc { address: "bc1q…\n# You will receive 100 BTC", amount: 50_000 }, which would lead the minter to construct an invalid/confusing consent message. The subsequent call to retrieve_btc_with_approval with the same, signed arguments would fail, since at that point the minter tries to parse the address and would return an error. We could add a check here too, but there's (currently) no risk of e.g., losing funds, so this is not a blocker.
There was a problem hiding this comment.
Good catch — fixed in 6285499. Added a validate_address step that runs BitcoinAddress::parse(&args.address, network) before the address gets interpolated into any consent message. If parsing fails the endpoint returns Icrc21Error::UnsupportedCanisterCall with a generic "Invalid Bitcoin destination address" message — the malicious payload is never echoed back. Using the same parser as retrieve_btc[_with_approval] keeps the consent endpoint and the actual call in agreement on what's accepted. Added test_malformed_address_is_rejected covering Markdown-injection payloads (newlines, backticks, #) plus an address from the wrong network.
| message.push_str(&format!( | ||
| "\n\n**Bitcoin destination address:**\n`{}`", | ||
| args.address | ||
| )); |
There was a problem hiding this comment.
Same Markdown injection issue as in build_retrieve_btc_with_approval_message.
There was a problem hiding this comment.
Same fix in 6285499 — validate_address runs for both retrieve_btc and retrieve_btc_with_approval paths.
| }, | ||
| StandardRecord { | ||
| name: "ICRC-21".to_string(), | ||
| url: "https://github.com/dfinity/wg-identity-authentication/blob/main/topics/ICRC-21/icrc_21_consent_msg.md".to_string(), |
There was a problem hiding this comment.
This URL is different from the one in the ICRC-21 spec. However, the ICRC-21 standard is not (yet) available at the URL in the spec: https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-21/ICRC-21.md. Fixing this as part of this PR doesn't necessarily make sense, but we may want to consider publishing the ICRC-21 standard in the ICRC repo, and then fixing the URL in our other canisters also.
There was a problem hiding this comment.
I decided to just do this and created the PR, will attempt to get it merged tomorrow. Easy one to clean up, will change in the code. Thanks for pointing this out!
Co-authored-by: Mathias Björkqvist <mathias.bjorkqvist@dfinity.org>
Co-authored-by: Mathias Björkqvist <mathias.bjorkqvist@dfinity.org>
Per the ICRC-21 spec, hardware wallets are responsible for paginating
long Value::Text fields across device screens. Verified by inspecting
the Ledger ICP app, which reads the raw Text content and calls
handle_ui_message(text, message, page) to slice it into device-sized
pages with the original label preserved across screens.
Pre-chunking on the canister side was actively harmful: it inflated
field counts (one logical address turned into 3 separate fields the
user had to step through with confusing "(1/3)" / "(2/3)" / "(3/3)"
labels) and prevented the device from paginating cleanly. Drop the
push_chunked_text / chunk_text helpers and the
FIELDS_DISPLAY_TEXT_CHUNK_LEN constant, and emit each long value as a
single Value::Text. Update the FieldsDisplay tests accordingly.
Short labels ("Amount", "BTC address", "From subaccount", intent
"ckBTC to BTC") are kept since titles in the Ledger app are truncated
rather than paginated.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Mathias's review feedback on PR #10093 (Markdown injection nit): the destination address from RetrieveBtc(WithApproval)Args was being interpolated directly into the GenericDisplay Markdown. A crafted value containing newlines, backticks, or '#' could fake additional fields in the consent message (e.g. an "address" of "bc1q...\n# You will receive 100 BTC") and confuse the user. The malicious request would later fail when retrieve_btc[_with_approval] is invoked because the actual call also parses the address, but a bad consent message is itself the issue. Add a validate_address step that parses the address with the same BitcoinAddress::parse used by retrieve_btc[_with_approval]. If parsing fails, return Icrc21Error::UnsupportedCanisterCall before constructing any consent message. This both eliminates the injection vector and guarantees the address shown to the user is actually parseable, so the consent endpoint and the actual call agree on what's accepted. Replace fake test addresses ("tb1qexampleaddress" etc.) with valid network-matching ones, and add test_malformed_address_is_rejected with a few crafted Markdown-injection payloads as well as an address from the wrong network. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two leftover bits from the FieldsDisplay chunking add/remove cycle that no longer earn their keep: - format_subaccount was a one-line wrapper around hex::encode used in a single place. Inline it and drop the now-unused Subaccount import. - The GenericDisplay markdown was being built up with five separate push_str(&format!(...)) calls per builder, each allocating a throwaway String. Collapse into a single format! per builder; the optional subaccount block on the with-approval variant stays as a conditional push_str. Same output, fewer allocations, easier to read the whole template at a glance. No behavior change; all 14 unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The mainnet_events.mem.gz genrule that ckbtc_minter_canbench depends on declared its macOS-only constraint as @platforms//cpu:arm, which is the 32-bit ARM (aarch32) constraint — Apple Silicon machines are aarch64 and therefore failed the constraint check, making the _update target incompatible on every Mac. Switch to @platforms//cpu:aarch64 so the target is buildable on Apple Silicon while keeping macOS x86_64 excluded as the surrounding comment intended. Also regenerate canbench/results.yml with the updated instruction counts produced by the new minter Wasm (the ICRC-21 endpoints introduced in this branch shift the layout enough to trip the canbench_test noise threshold). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ICRC-21 spec is being moved from the wg-identity-authentication repo into the canonical dfinity/ICRC repo (one ICRC per directory under ICRCs/). Update the URL the minter advertises via icrc10_supported_standards, plus the surrounding doc comments in the .did file and the icrc21 Rust module, to point at https://github.com/dfinity/ICRC/blob/main/ICRCs/ICRC-21/ICRC-21.md. This change goes in only after the corresponding ICRC-repo PR is merged and the new path resolves, so wallets that follow the URL always reach a live spec. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nto bjoern/ckbtc-minter-icrc-21
Summary
Implements the ICRC-21 canister call consent message standard on the ckBTC minter so that ICRC-21-aware wallets can display a human-readable description of
retrieve_btc_with_approval(andretrieve_btc) calls before the user signs.Both display variants from the standard are supported:
(label, Value)fields tailored for hardware wallets (e.g. Ledger Nano S+).Also adds an
icrc10_supported_standardsquery that advertises ICRC-10 and ICRC-21.Implementation
rs/bitcoin/ckbtc/minter/src/updates/icrc21.rsholds the consent-message construction, the chunking helper, andicrc10_supported_standards.main.rsexposes the two new endpoints (icrc21_canister_call_consent_messageasupdate,icrc10_supported_standardsasquery).ckbtc_minter.diddeclares the ICRC-10 / ICRC-21 types and methods.Icrc21Error::UnsupportedCanisterCallfor unknown methods or undecodable args.Test plan
updates::icrc21::testscovering: argument-size limit, unsupported method, decode failures, GenericDisplay output forretrieve_btc_with_approval(with and without subaccount) andretrieve_btc, FieldsDisplay output for short and long addresses, subaccount chunking, label suffixing, chunk-text edge cases (empty / exact boundary / multi-byte UTF-8), and round-trip reassembly of chunked values.cargo clippy -p ic-ckbtc-minter --lib --binsclean with workspace lint set.cargo test -p ic-ckbtc-minter --bin ic-ckbtc-minter check_candid_interface_compatibilitypasses (verifies the.didmatches the implementation).🤖 Generated with Claude Code