Skip to content

Commit

Permalink
SECFIND-244: Add notifyAndAttachCanister to canisters API (#6126)
Browse files Browse the repository at this point in the history
# Motivation

We want to recover from interrupted canister creation in the frontend
code.
To do this we should be able to notify the CMC and attach a canister
without first making a transfer, as the transfer has already happened
before the process got interrupted.
See [this
doc](https://docs.google.com/document/d/1hjMSTzjnVbU9Q4rJk233M3uNcKC-RLhme25nahHpTZg/edit?tab=t.0#heading=h.qtiuv2x3gsjy)
for details.

# Changes

1. Add `notifyAndAttachCanister` which can be used to recover canister
creation based on an existing ICP transfer.

# Tests

1. Added unit tests.
2. Cleaned up the existing test on which the new test was based, by
using fake time instead of stubbing out `setTimeout` and not mocking
`console.log` as nothing gets logged and this just makes debugging
harder.
3. Tested manually in a branch with the end-to-end solution.

# Todos

- [ ] Add entry to changelog (if necessary).
not yet
  • Loading branch information
dskloetd authored Jan 9, 2025
1 parent c83c4e3 commit 8e3c2e6
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 9 deletions.
46 changes: 46 additions & 0 deletions frontend/src/lib/api/canisters.api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,52 @@ export const createCanister = async ({
return canisterId;
};

// Creates a canister based on a transaction that was performed before.
// Used in case the canister creation process is interrupted after ICP are
// transferred to the CMC account.
//
// If we see a funding transaction for canister creation, but no canister it can
// mean either of 2 things:
// 1. The canister was not created.
// 2. The canister was created but not attached to the user account.
//
// This function takes care of both cases because notifying the CMC will create
// the canister if it doesn't exist, or return the same response as the first
// time if it does exist. So in either case this will give us the canister ID so
// that we can attach it to the user account.
export const notifyAndAttachCanister = async ({
identity,
blockIndex,
}: {
identity: Identity;
blockIndex: bigint;
}): Promise<Principal> => {
logWithTimestamp("Notify and attach canister...");

const { cmc, nnsDapp } = await canisters(identity);

const controller = identity.getPrincipal();

const canisterId = await pollNotifyCreateCanister({
cmc,
controller,
blockHeight: blockIndex,
});

// Attach the canister to the user in the nns-dapp.
await nnsDapp.attachCanister({
// We don't know the name the user originally chose so they will have to
// rename it, if they want, after we recover the canister.
name: "",
canisterId,
blockIndex,
});

logWithTimestamp("Notify and attach canister complete.");

return canisterId;
};

// Polls CMC waiting for a reponse that is not a ProcessingError.
const pollNotifyTopUpCanister = async ({
cmc,
Expand Down
84 changes: 75 additions & 9 deletions frontend/src/tests/lib/api/canisters.api.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
createCanister,
detachCanister,
getIcpToCyclesExchangeRate,
notifyAndAttachCanister,
notifyTopUpCanister,
queryCanisterDetails,
queryCanisters,
Expand All @@ -26,6 +27,10 @@ import {
mockCanisterSettings,
} from "$tests/mocks/canisters.mock";
import { mockSubAccount } from "$tests/mocks/icp-accounts.store.mock";
import {
advanceTime,
runResolvedPromises,
} from "$tests/utils/timers.test-utils";
import { CMCCanister, ProcessingError } from "@dfinity/cmc";
import {
AccountIdentifier,
Expand Down Expand Up @@ -220,13 +225,6 @@ describe("canisters-api", () => {
});

describe("createCanister", () => {
beforeEach(() => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
vi.spyOn(global, "setTimeout").mockImplementation((cb: any) => cb());
// Avoid to print errors during test
vi.spyOn(console, "log").mockImplementation(() => undefined);
});

it("should make a transfer, notify and attach the canister", async () => {
const blockIndex = 10n;
mockLedgerCanister.transfer.mockResolvedValue(blockIndex);
Expand Down Expand Up @@ -277,13 +275,14 @@ describe("canisters-api", () => {
.mockRejectedValueOnce(new ProcessingError())
.mockResolvedValue(mockCanisterDetails.id);

const response = await createCanister({
const responsePromise = createCanister({
identity: mockIdentity,
amount: 300_000_000n,
fee,
});
await advanceTime();
expect(mockCMCCanister.notifyCreateCanister).toHaveBeenCalledTimes(2);
expect(response).toEqual(mockCanisterDetails.id);
expect(await responsePromise).toEqual(mockCanisterDetails.id);
});

it("handles creating from subaccounts", async () => {
Expand Down Expand Up @@ -359,6 +358,73 @@ describe("canisters-api", () => {
});
});

describe("notifyAndAttachCanister", () => {
it("should notify the CMC and attach the canister", async () => {
const blockIndex = 10n;
mockCMCCanister.notifyCreateCanister.mockResolvedValue(
mockCanisterDetails.id
);

const response = await notifyAndAttachCanister({
identity: mockIdentity,
blockIndex,
});
expect(mockCMCCanister.notifyCreateCanister).toBeCalledWith({
block_index: blockIndex,
controller: mockIdentity.getPrincipal(),
settings: [],
subnet_selection: [],
subnet_type: [],
});
expect(mockCMCCanister.notifyCreateCanister).toBeCalledTimes(1);
expect(mockNNSDappCanister.attachCanister).toBeCalledWith({
name: "",
canisterId: mockCanisterDetails.id,
blockIndex,
});
expect(response).toEqual(mockCanisterDetails.id);
});

it("should notify twice if the first call returns Processing", async () => {
const blockIndex = 10n;
mockCMCCanister.notifyCreateCanister
.mockRejectedValueOnce(new ProcessingError())
.mockResolvedValue(mockCanisterDetails.id);

const responsePromise = notifyAndAttachCanister({
identity: mockIdentity,
blockIndex,
});

await runResolvedPromises();
const expectedNotifyParams = {
block_index: blockIndex,
controller: mockIdentity.getPrincipal(),
settings: [],
subnet_selection: [],
subnet_type: [],
};
expect(mockCMCCanister.notifyCreateCanister).toBeCalledTimes(1);
expect(mockCMCCanister.notifyCreateCanister).toHaveBeenNthCalledWith(
1,
expectedNotifyParams
);
await advanceTime();

expect(mockCMCCanister.notifyCreateCanister).toBeCalledTimes(2);
expect(mockCMCCanister.notifyCreateCanister).toHaveBeenNthCalledWith(
2,
expectedNotifyParams
);
expect(mockNNSDappCanister.attachCanister).toBeCalledWith({
name: "",
canisterId: mockCanisterDetails.id,
blockIndex,
});
expect(await responsePromise).toEqual(mockCanisterDetails.id);
});
});

describe("topUpCanister", () => {
beforeEach(() => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down

0 comments on commit 8e3c2e6

Please sign in to comment.