Conversation
mducroux
left a comment
mducroux
left a comment
There was a problem hiding this comment.
Thanks for adding this flow @ielashi! I run into an issue when running just run icrc21 call --canister-id xxydu-fqaaa-aaaam-ad2ka-cai --method swap --arg $(didc encode '("ICP", "ckBTC", 1000000000 : nat64)') twice and rejecting the transaction on the Ledger the first time:
Error: A ledger error happened during signature:
Code: 27012
Message: "Data is invalid : "
| .addCommand( | ||
| new Command("call") | ||
| .description( | ||
| 'Request a consent message for a canister call\n\nExample: ic-hardware-wallet icrc21 call --canister-id xxydu-fqaaa-aaaam-ad2ka-cai --method swap --arg $(didc encode \'("ICP", "ckBTC", 1000000000 : nat64)\')' |
There was a problem hiding this comment.
ic-hardware-wallet -> should be just run or something else?
| .addOption( | ||
| new Option("--network <network>", "The IC network to talk to.") | ||
| .default("https://ic0.app") | ||
| .default("https://icp0.io") |
There was a problem hiding this comment.
understanding: why did this change?
| tryParseHexString | ||
| ) | ||
| .action(async ({ canisterId, method, arg }) => | ||
| run(async () => { |
There was a problem hiding this comment.
nit: to be consistent with the other commands, consider moving this into its own function
| } | ||
| } | ||
|
|
||
| export function tryParseHexString(value: string): string { |
There was a problem hiding this comment.
nit: I think the name is slightly misleading as we are not parsing but validating. What about: assertHexString?
| * Uses an anonymous agent to fetch consent messages from canisters, then delegates | ||
| * the actual call signing to the provided identity via BLS signatures. | ||
| */ | ||
| export class Icrc21Agent implements Agent { |
There was a problem hiding this comment.
Do you need Icrc21Agent to implement Agent? Implementing Agent with three throwing stubs seems misleading.
| ); | ||
|
|
||
| // Reset the ICRC-21 flag after signing | ||
| this._icrc21Flag = false; |
There was a problem hiding this comment.
Consider putting this in a finally statement to make sure it is executed even if signing with BLS fails
| let signature: Signature; | ||
|
|
||
| if (this._icrc21Flag) { | ||
| // Use BLS signing for ICRC-21 transactions |
There was a problem hiding this comment.
Is that accurate? I read it as ask the ledger wallet to produce a BLS signature but it is not the case right? In which case the name signBls is probably misleading.
| } | ||
|
|
||
| const consentRequest = consentMessageSubmitResponse.requestDetails; | ||
|
|
There was a problem hiding this comment.
nit: consider adding:
if (!consentRequest) { throw new Error("Missing request details from consent message call"); }
| const result = await this.signingAgent.call(canisterId, { | ||
| methodName: fields.methodName, | ||
| arg: fields.arg, | ||
| effectiveCanisterId: canisterId, |
There was a problem hiding this comment.
understanding: why not: fields.effectiveCanisterId?
| // Flag the identity so transformRequest will use BLS signing | ||
| this.identity.flagUpcomingIcrc21(consentRequestHex, certificateHex); | ||
|
|
||
| // Submit the actual canister call |
There was a problem hiding this comment.
suggestion: // Send the canister call via the signing agent which triggers ICRC-21 approval on the Ledger
2 participants
Icrc21Agentfor signing using the ICRC-21 standardLedgerIdentity.