Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Populate icrc store with imported tokens #5378

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
+61 −6
Draft

Populate icrc store with imported tokens #5378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
638cf81
Make indexCanisterId optional in IcrcCanistersStore
mstrasinskis Aug 30, 2024
f3e8fae
Add imported tokens to icrcCanistersStore
mstrasinskis Aug 30, 2024
8515ad4
test: should add imported tokens to the icrcCanistersStore
mstrasinskis Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions frontend/src/lib/services/imported-tokens.services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type { ImportedTokens } from "$lib/canisters/nns-dapp/nns-dapp.types";
import { MAX_IMPORTED_TOKENS } from "$lib/constants/imported-tokens.constants";
import { FORCE_CALL_STRATEGY } from "$lib/constants/mockable.constants";
import { getAuthenticatedIdentity } from "$lib/services/auth.services";
import { icrcCanistersStore } from "$lib/stores/icrc-canisters.store";
import { importedTokensStore } from "$lib/stores/imported-tokens.store";
import { toastsError, toastsSuccess } from "$lib/stores/toasts.store";
import type { ImportedTokenData } from "$lib/types/imported-tokens";
Expand All @@ -19,6 +20,7 @@ import {
toImportedTokenData,
} from "$lib/utils/imported-tokens.utils";
import { isNullish } from "@dfinity/utils";
import { get } from "svelte/store";
import { queryAndUpdate } from "./utils.services";

/** Load imported tokens from the `nns-dapp` backend and update the `importedTokensStore` store.
Expand All @@ -32,11 +34,31 @@ export const loadImportedTokens = async ({
return queryAndUpdate<ImportedTokens, unknown>({
request: (options) => getImportedTokens(options),
strategy: FORCE_CALL_STRATEGY,
onLoad: ({ response: { imported_tokens: importedTokens }, certified }) =>
onLoad: ({
response: { imported_tokens: rawImportedTokens },
certified,
}) => {
const importedTokens = rawImportedTokens.map(toImportedTokenData);
importedTokensStore.set({
importedTokens: importedTokens.map(toImportedTokenData),
importedTokens,
certified,
}),
});

if (!certified && notForceCallStrategy()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is importedTokensStore populated regardless of certified but icrcCanistersStore only if certified?

Copy link
Contributor Author

@mstrasinskis mstrasinskis Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a certified property in the importedTokensStore, so that we always know the version. Since we skip adding a token when it’s already in the store, I thought it made sense to use only the certified version. However, it’s not a strong opinion. Especially now, when only query request is used.

return;
}

// Populate icrcCanistersStore with the imported tokens.
for (const { ledgerCanisterId, indexCanisterId } of importedTokens) {
// If the imported token is not already in the store, add it.
if (isNullish(get(icrcCanistersStore)[ledgerCanisterId.toText()])) {
icrcCanistersStore.setCanisters({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't populated 2 different stores with the same data. Instead we should have a derived store which combines the relevant stores.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like AllIcrcCanistersStore ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, or we rename icrcCanisterStore to something else like, defaultIcrcCanistersStore or something.

ledgerCanisterId,
indexCanisterId,
});
}
}
},
onError: ({ error: err, certified }) => {
console.error(err);

Expand Down Expand Up @@ -97,8 +119,6 @@ export const addImportedToken = async ({
tokenToAdd: ImportedTokenData;
importedTokens: ImportedTokenData[];
}): Promise<{ success: boolean }> => {
// TODO: validate importedToken (not sns, not ck, is unique, etc.)

const tokens = [...importedTokens, tokenToAdd];
const { err } = await saveImportedToken({ tokens });

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/lib/stores/icrc-canisters.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { writable } from "svelte/store";

export interface IcrcCanisters {
ledgerCanisterId: Principal;
indexCanisterId: Principal;
indexCanisterId: Principal | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means the work-around can now be removed, right?

nns-dapp/frontend/src/lib/stores/icrc-canisters.store.ts

Line 82 in 8515ad4

: (undefined as unknown as Principal) /* use at your own risk */,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only after the feature in not behind the flag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come? With this change, the type of indexCanisterId is Principal | undefined regardless of whether the feature flag is enabled.

}

export type IcrcCanistersStoreData = Record<
Expand Down
35 changes: 35 additions & 0 deletions frontend/src/tests/lib/services/imported-tokens.services.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ import {
loadImportedTokens,
removeImportedTokens,
} from "$lib/services/imported-tokens.services";
import { icrcCanistersStore } from "$lib/stores/icrc-canisters.store";
import { importedTokensStore } from "$lib/stores/imported-tokens.store";
import * as toastsStore from "$lib/stores/toasts.store";
import type { ImportedTokenData } from "$lib/types/imported-tokens";
import { mockIdentity, resetIdentity } from "$tests/mocks/auth.store.mock";
import { principal } from "$tests/mocks/sns-projects.mock";
import { get } from "svelte/store";
import { runResolvedPromises } from "../../utils/timers.test-utils";

describe("imported-tokens-services", () => {
const importedTokenA: ImportedToken = {
Expand All @@ -39,6 +41,7 @@ describe("imported-tokens-services", () => {
vi.clearAllMocks();
resetIdentity();
importedTokensStore.reset();
icrcCanistersStore.reset();
vi.spyOn(console, "error").mockReturnValue();
});

Expand Down Expand Up @@ -74,6 +77,38 @@ describe("imported-tokens-services", () => {
});
});

it("should add imported tokens to the icrcCanistersStore", async () => {
const spyGetImportedTokens = vi
.spyOn(importedTokensApi, "getImportedTokens")
.mockResolvedValueOnce({
imported_tokens: [importedTokenA, importedTokenB],
});

expect(spyGetImportedTokens).toBeCalledTimes(0);
expect(get(icrcCanistersStore)).toEqual({});

expect(get(importedTokensStore)).toEqual({
importedTokens: undefined,
certified: undefined,
});

await loadImportedTokens();
// Wait for the store to update.
await runResolvedPromises();

expect(spyGetImportedTokens).toBeCalledTimes(2);
expect(get(icrcCanistersStore)).toEqual({
[importedTokenDataA.ledgerCanisterId.toText()]: {
ledgerCanisterId: importedTokenDataA.ledgerCanisterId,
indexCanisterId: importedTokenDataA.indexCanisterId,
},
[importedTokenDataB.ledgerCanisterId.toText()]: {
ledgerCanisterId: importedTokenDataB.ledgerCanisterId,
indexCanisterId: importedTokenDataB.indexCanisterId,
},
});
});

it("should display toast on error", async () => {
const spyToastError = vi.spyOn(toastsStore, "toastsError");
const spyGetImportedTokens = vi
Expand Down