From 1f2e7d9dbd6c06e57fa55c6c8cc4c07e0ebee31b Mon Sep 17 00:00:00 2001 From: David de Kloet <122978264+dskloetd@users.noreply.github.com> Date: Wed, 20 Sep 2023 11:23:57 +0200 Subject: [PATCH] Add icrc1Transfer method to ICP ledger canister (#419) # Motivation We want to transfer ICP using the ICRC-1 API because then we don't have to compute account identifiers in the client. Passing the ICP ledger canister ID to the ICRC-1 API works but the ICP ledger canister has its own ICRC-1 interface so calling it directly on the ICP ledger API is cleaner. *Note*: I started with a commit that duplicates the non-ICRC-1 code. That way you can see how the ICRC-1 code differs from the existing code by reviewing the second commit. # Changes 1. Added `icrc1Transfer` canister method. 2. Added `toIcrc1TransferRawRequest` request converter. 3. Added `mapIcrc1TransferError` error converter. 4. Added `Icrc1TransferRequest` request type. # Tests Copied the existing tests for `transfer` and applied them to `icrc1Transfer` as far as they made sense. The new method has less custom code for hardware wallets so some of those test have not been duplicated. Tested manually in NNS dapp by using the new method when making ICRC-1 transfers and then testing an ICRC-1 transfer. --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- CHANGELOG.md | 1 + packages/nns/README.md | 22 +- .../ledger/ledger.request.converts.ts | 25 +- packages/nns/src/errors/ledger.errors.ts | 33 +- packages/nns/src/ledger.canister.spec.ts | 363 ++++++++++++++++++ packages/nns/src/ledger.canister.ts | 30 +- packages/nns/src/types/ledger_converters.ts | 18 + 7 files changed, 482 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb1f70d6..cb4cd47d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ - add support for `icrc2_transfer_from`, `icrc2_approve` and `icrc2_allowance` in `@dfinity/ledger` - update index did definitions in ledger which provides more information in the transactions +- add support for icrc1_transfer on the ICP ledger canister in `@dfinity/nss` ## Build diff --git a/packages/nns/README.md b/packages/nns/README.md index 3d576c24..6c9176c9 100644 --- a/packages/nns/README.md +++ b/packages/nns/README.md @@ -259,7 +259,7 @@ Parameters: ### :factory: LedgerCanister -[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L27) +[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L32) #### Methods @@ -267,6 +267,7 @@ Parameters: - [accountBalance](#gear-accountbalance) - [transactionFee](#gear-transactionfee) - [transfer](#gear-transfer) +- [icrc1Transfer](#gear-icrc1transfer) ##### :gear: create @@ -274,7 +275,7 @@ Parameters: | -------- | ----------------------------------------------------- | | `create` | `(options?: LedgerCanisterOptions) => LedgerCanister` | -[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L38) +[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L43) ##### :gear: accountBalance @@ -287,7 +288,7 @@ it is fetched using a query call. | ---------------- | ------------------------------------------------------------------------------------------------------------------------ | | `accountBalance` | `({ accountIdentifier, certified, }: { accountIdentifier: AccountIdentifier; certified?: boolean; }) => Promise` | -[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L70) +[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L75) ##### :gear: transactionFee @@ -297,7 +298,7 @@ Returns the transaction fee of the ledger canister | ---------------- | ----------------------- | | `transactionFee` | `() => Promise` | -[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L94) +[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L99) ##### :gear: transfer @@ -308,7 +309,18 @@ Returns the index of the block containing the tx if it was successful. | ---------- | ----------------------------------------------- | | `transfer` | `(request: TransferRequest) => Promise` | -[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L107) +[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L112) + +##### :gear: icrc1Transfer + +Transfer ICP from the caller to the destination `Account`. +Returns the index of the block containing the tx if it was successful. + +| Method | Type | +| --------------- | ---------------------------------------------------- | +| `icrc1Transfer` | `(request: Icrc1TransferRequest) => Promise` | + +[:link: Source](https://github.com/dfinity/ic-js/tree/main/packages/nns/src/ledger.canister.ts#L137) ### :factory: GovernanceCanister diff --git a/packages/nns/src/canisters/ledger/ledger.request.converts.ts b/packages/nns/src/canisters/ledger/ledger.request.converts.ts index 0167b45a..1bf9f488 100644 --- a/packages/nns/src/canisters/ledger/ledger.request.converts.ts +++ b/packages/nns/src/canisters/ledger/ledger.request.converts.ts @@ -1,11 +1,15 @@ import type { ICPTs, Subaccount } from "@dfinity/nns-proto"; -import { arrayOfNumberToUint8Array } from "@dfinity/utils"; +import { arrayOfNumberToUint8Array, toNullable } from "@dfinity/utils"; import type { Tokens, + TransferArg as Icrc1TransferRawRequest, TransferArgs as TransferRawRequest, } from "../../../candid/ledger"; import { TRANSACTION_FEE } from "../../constants/constants"; -import type { TransferRequest } from "../../types/ledger_converters"; +import type { + Icrc1TransferRequest, + TransferRequest, +} from "../../types/ledger_converters"; import { importNnsProto } from "../../utils/proto.utils"; export const subAccountNumbersToSubaccount = async ( @@ -47,3 +51,20 @@ export const toTransferRawRequest = ({ ? [] : [arrayOfNumberToUint8Array(fromSubAccount)], }); + +export const toIcrc1TransferRawRequest = ({ + fromSubAccount, + to, + amount, + fee, + memo, + createdAt, +}: Icrc1TransferRequest): Icrc1TransferRawRequest => ({ + to, + fee: toNullable(fee ?? TRANSACTION_FEE), + amount, + // Always explicitly set the memo for compatibility with ledger wallet - hardware wallet + memo: toNullable(memo ?? new Uint8Array()), + created_at_time: toNullable(createdAt), + from_subaccount: toNullable(fromSubAccount), +}); diff --git a/packages/nns/src/errors/ledger.errors.ts b/packages/nns/src/errors/ledger.errors.ts index f211a855..85387782 100644 --- a/packages/nns/src/errors/ledger.errors.ts +++ b/packages/nns/src/errors/ledger.errors.ts @@ -1,5 +1,8 @@ import { convertStringToE8s } from "@dfinity/utils"; -import type { TransferError as RawTransferError } from "../../candid/ledger"; +import type { + Icrc1TransferError as RawIcrc1TransferError, + TransferError as RawTransferError, +} from "../../candid/ledger"; import type { BlockHeight } from "../types/common"; export class TransferError extends Error {} @@ -13,7 +16,7 @@ export class InsufficientFundsError extends TransferError { } export class TxTooOldError extends TransferError { - constructor(public readonly allowed_window_secs: number) { + constructor(public readonly allowed_window_secs?: number | undefined) { super(); } } @@ -60,6 +63,32 @@ export const mapTransferError = ( ); }; +export const mapIcrc1TransferError = ( + rawTransferError: RawIcrc1TransferError, +): TransferError => { + if ("Duplicate" in rawTransferError) { + return new TxDuplicateError(rawTransferError.Duplicate.duplicate_of); + } + if ("InsufficientFunds" in rawTransferError) { + return new InsufficientFundsError( + rawTransferError.InsufficientFunds.balance, + ); + } + if ("CreatedInFuture" in rawTransferError) { + return new TxCreatedInFutureError(); + } + if ("TooOld" in rawTransferError) { + return new TxTooOldError(); + } + if ("BadFee" in rawTransferError) { + return new BadFeeError(rawTransferError.BadFee.expected_fee); + } + // Edge case + return new TransferError( + `Unknown error type ${JSON.stringify(rawTransferError)}`, + ); +}; + export const mapTransferProtoError = (responseBytes: Error): TransferError => { const { message } = responseBytes; diff --git a/packages/nns/src/ledger.canister.spec.ts b/packages/nns/src/ledger.canister.spec.ts index 8be05fe8..fc7304ba 100644 --- a/packages/nns/src/ledger.canister.spec.ts +++ b/packages/nns/src/ledger.canister.spec.ts @@ -1,5 +1,6 @@ import { ActorSubclass } from "@dfinity/agent"; import { Memo, Payment, SendRequest } from "@dfinity/nns-proto"; +import { Principal } from "@dfinity/principal"; import { arrayOfNumberToUint8Array } from "@dfinity/utils"; import { mock } from "jest-mock-extended"; import type { _SERVICE as LedgerService } from "../candid/ledger"; @@ -615,4 +616,366 @@ describe("LedgerCanister", () => { }); }); }); + + describe("icrc1Transfer", () => { + describe("no hardware wallet", () => { + const to = { + owner: Principal.fromHex("abcd"), + subaccount: [] as [], + }; + const amount = BigInt(100000); + + it("fetches transaction fee if not present", async () => { + const service = mock>(); + service.transfer_fee.mockResolvedValue({ + transfer_fee: { e8s: BigInt(10_000) }, + }); + service.icrc1_transfer.mockResolvedValue({ + Ok: BigInt(1234), + }); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + serviceOverride: service, + }); + await ledger.icrc1Transfer({ + to, + amount, + }); + + expect(service.transfer_fee).toBeCalled(); + }); + + it("calls transfer certified service with data", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Ok: BigInt(1234), + }); + const fee = BigInt(10_000); + const memo = new Uint8Array([3, 4, 5, 6]); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + }); + await ledger.icrc1Transfer({ + to, + amount, + fee, + memo, + }); + + expect(service.icrc1_transfer).toBeCalledWith({ + to, + fee: [fee], + amount, + memo: [memo], + created_at_time: [], + from_subaccount: [], + }); + }); + + it("sets a default memo if not passed", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Ok: BigInt(1234), + }); + const fee = BigInt(10_000); + const defaultMemo = new Uint8Array(); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + }); + await ledger.icrc1Transfer({ + to, + amount, + fee, + }); + + expect(service.icrc1_transfer).toBeCalledWith({ + to, + fee: [fee], + amount, + memo: [defaultMemo], + created_at_time: [], + from_subaccount: [], + }); + }); + + it("handles createdAt parameter", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Ok: BigInt(1234), + }); + const fee = BigInt(10_000); + const memo = new Uint8Array([3, 4, 5, 6]); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + }); + const createdAt = BigInt(123132223); + await ledger.icrc1Transfer({ + to, + amount, + fee, + memo, + createdAt, + }); + + expect(service.icrc1_transfer).toBeCalledWith({ + to, + fee: [fee], + amount, + memo: [memo], + created_at_time: [createdAt], + from_subaccount: [], + }); + }); + + it("handles from subaccount", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Ok: BigInt(1234), + }); + const fee = BigInt(10_000); + const memo = new Uint8Array(); + const fromSubAccount = new Uint8Array([ + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 1, + ]); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + }); + await ledger.icrc1Transfer({ + to, + amount, + fee, + memo, + fromSubAccount, + }); + + expect(service.icrc1_transfer).toBeCalledWith({ + to, + fee: [fee], + amount, + memo: [memo], + created_at_time: [], + from_subaccount: [fromSubAccount], + }); + }); + + it("handles to subaccount", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Ok: BigInt(1234), + }); + const fee = BigInt(10_000); + const memo = new Uint8Array(); + const toSubAccount = new Uint8Array([ + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 1, + ]); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + }); + await ledger.icrc1Transfer({ + to: { + ...to, + subaccount: [toSubAccount], + }, + amount, + fee, + memo, + }); + + expect(service.icrc1_transfer).toBeCalledWith({ + to: { + ...to, + subaccount: [toSubAccount], + }, + fee: [fee], + amount, + memo: [memo], + created_at_time: [], + from_subaccount: [], + }); + }); + + it("handles duplicate transaction", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Err: { + Duplicate: { + duplicate_of: BigInt(10), + }, + }, + }); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + serviceOverride: service, + }); + const call = async () => + await ledger.icrc1Transfer({ + to, + amount, + fee: BigInt(10_000), + }); + + expect(call).rejects.toThrowError(TxDuplicateError); + }); + + it("handles insufficient balance", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Err: { + InsufficientFunds: { + balance: BigInt(12312414), + }, + }, + }); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + serviceOverride: service, + }); + const call = async () => + await ledger.icrc1Transfer({ + to, + amount, + fee: BigInt(10_000), + }); + + expect(call).rejects.toThrowError(InsufficientFundsError); + }); + + it("handles old tx", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Err: { + TooOld: null, + }, + }); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + serviceOverride: service, + }); + const call = async () => + await ledger.icrc1Transfer({ + to, + amount, + fee: BigInt(10_000), + }); + + expect(call).rejects.toThrowError(TxTooOldError); + }); + + it("handles bad fee", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Err: { + BadFee: { + expected_fee: BigInt(1234), + }, + }, + }); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + serviceOverride: service, + }); + const call = async () => + await ledger.icrc1Transfer({ + to, + amount, + fee: BigInt(10_000), + }); + + expect(call).rejects.toThrowError(BadFeeError); + }); + + it("handles transaction created in the future", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Err: { + CreatedInFuture: { ledger_time: BigInt(1234) }, + }, + }); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + serviceOverride: service, + }); + const call = async () => + await ledger.icrc1Transfer({ + to, + amount, + fee: BigInt(10_000), + }); + + expect(call).rejects.toThrowError(TxCreatedInFutureError); + }); + }); + + describe("for hardware wallet", () => { + const to = { + owner: Principal.fromHex("abcd"), + subaccount: [] as [], + }; + const amount = BigInt(100000); + + it("should set a default fee for a transfer", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Ok: BigInt(1234), + }); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + serviceOverride: service, + hardwareWallet: true, + }); + + const memo = new Uint8Array(); + await ledger.icrc1Transfer({ + to, + amount, + memo, + }); + + expect(service.transfer_fee).not.toBeCalled(); + + expect(service.icrc1_transfer).toBeCalledWith({ + to, + fee: [BigInt(10000)], + amount, + memo: [memo], + created_at_time: [], + from_subaccount: [], + }); + }); + + it("should use custom fee for a transfer", async () => { + const service = mock>(); + service.icrc1_transfer.mockResolvedValue({ + Ok: BigInt(1234), + }); + const ledger = LedgerCanister.create({ + certifiedServiceOverride: service, + serviceOverride: service, + hardwareWallet: true, + }); + + const fee = BigInt(990_000); + const memo = new Uint8Array(); + await ledger.icrc1Transfer({ + to, + amount, + fee, + memo, + }); + + expect(service.transfer_fee).not.toBeCalled(); + + expect(service.icrc1_transfer).toBeCalledWith({ + to, + fee: [fee], + amount, + memo: [memo], + created_at_time: [], + from_subaccount: [], + }); + }); + }); + }); }); diff --git a/packages/nns/src/ledger.canister.ts b/packages/nns/src/ledger.canister.ts index 981cebe1..095352d9 100644 --- a/packages/nns/src/ledger.canister.ts +++ b/packages/nns/src/ledger.canister.ts @@ -8,11 +8,13 @@ import type { AccountIdentifier } from "./account_identifier"; import { subAccountNumbersToSubaccount, toICPTs, + toIcrc1TransferRawRequest, toTransferRawRequest, } from "./canisters/ledger/ledger.request.converts"; import { MAINNET_LEDGER_CANISTER_ID } from "./constants/canister_ids"; import { TRANSACTION_FEE } from "./constants/constants"; import { + mapIcrc1TransferError, mapTransferError, mapTransferProtoError, } from "./errors/ledger.errors"; @@ -21,7 +23,10 @@ import type { LedgerCanisterCall, LedgerCanisterOptions, } from "./types/ledger.options"; -import type { TransferRequest } from "./types/ledger_converters"; +import type { + Icrc1TransferRequest, + TransferRequest, +} from "./types/ledger_converters"; import { importNnsProto, queryCall, updateCall } from "./utils/proto.utils"; export class LedgerCanister { @@ -123,6 +128,29 @@ export class LedgerCanister { return response.Ok; }; + /** + * Transfer ICP from the caller to the destination `Account`. + * Returns the index of the block containing the tx if it was successful. + * + * @throws {@link TransferError} + */ + public icrc1Transfer = async ( + request: Icrc1TransferRequest, + ): Promise => { + // The transaction fee method is not supported by Ledger App yet. + if (request.fee === undefined) { + request.fee = this.hardwareWallet + ? TRANSACTION_FEE + : await this.transactionFee(); + } + const rawRequest = toIcrc1TransferRawRequest(request); + const response = await this.certifiedService.icrc1_transfer(rawRequest); + if ("Err" in response) { + throw mapIcrc1TransferError(response.Err); + } + return response.Ok; + }; + private accountBalanceHardwareWallet = async ({ accountIdentifier, certified = true, diff --git a/packages/nns/src/types/ledger_converters.ts b/packages/nns/src/types/ledger_converters.ts index 856b96bb..901f080f 100644 --- a/packages/nns/src/types/ledger_converters.ts +++ b/packages/nns/src/types/ledger_converters.ts @@ -1,3 +1,9 @@ +import type { + Account, + Icrc1Timestamp, + Icrc1Tokens, + SubAccount, +} from "../../candid/ledger"; import type { AccountIdentifier } from "../account_identifier"; import type { E8s } from "./common"; @@ -13,3 +19,15 @@ export type TransferRequest = { // https://github.com/dfinity/ICRC-1/blob/main/standards/ICRC-1/README.md#transaction_deduplication createdAt?: bigint; }; + +export type Icrc1TransferRequest = { + to: Account; + amount: Icrc1Tokens; + memo?: Uint8Array; + fee?: Icrc1Tokens; + fromSubAccount?: SubAccount; + // Nanoseconds since unix epoc to trigger deduplication and avoid other issues + // See the link for more details on deduplication + // https://github.com/dfinity/ICRC-1/blob/main/standards/ICRC-1/README.md#transaction_deduplication + createdAt?: Icrc1Timestamp; +};