From eafbd8a08361581109cc837167a7711e9aa42d19 Mon Sep 17 00:00:00 2001 From: David de Kloet <122978264+dskloetd@users.noreply.github.com> Date: Mon, 7 Oct 2024 11:26:24 +0200 Subject: [PATCH] Simplify 'last call' logic for queryAndUpdate (#5563) # Motivation With `queryAndUpdate` we make 2 calls but sometimes we only want to do something for 1 of the calls. For example, if there is an error in the query call, we just ignore it and wait for the update call. The problem is that `queryAndUpdate` doesn't always do 2 calls and there is some logic to determine whether to do 2 calls or not. If the user is not logged in, we only do a query call and it's also possible to pass in a strategy, which might also depend on the value of `FORCE_CALL_STRATEGY`. Altogether this makes knowing which strategy is currently being used, and thus whether we are doing 1 or 2 calls, and thus whether the current call is the last call or if another call is still following, quite fragile. In this PR, we make `queryAndUpdate` pass the actually used strategy to the `onLoad` and `onError` callback and also add a `isLastCall` function to determine if the current call is either the update call of query+update or the only call of a single query or update call. # Changes 1. Make `queryAndUpdate` pass `strategy` to its callbacks. 2. Add `isLastCall` utility function. 3. Use the utility function in all the places where it makes sense. # Tests 1. Add unit tests for `isLastCall`. 2. Update unit tests for `queryAndUpdate`. 3. Add a lot of missing test coverage because it seems that all these "is this the last call" code paths were untested. # Todos - [ ] Add entry to changelog (if necessary). not necessary --- .../src/lib/services/canisters.services.ts | 6 +- .../src/lib/services/ckbtc-info.services.ts | 6 +- .../lib/services/icrc-accounts.services.ts | 9 +- .../lib/services/imported-tokens.services.ts | 6 +- frontend/src/lib/services/neurons.services.ts | 10 +- .../lib/services/public/proposals.services.ts | 54 ++++--- .../src/lib/services/public/sns.services.ts | 8 +- .../src/lib/services/sns-neurons.services.ts | 6 +- frontend/src/lib/services/sns.services.ts | 17 +-- frontend/src/lib/services/utils.services.ts | 6 +- frontend/src/lib/utils/env.utils.ts | 13 ++ .../lib/services/canisters.services.spec.ts | 50 +++++- .../lib/services/ckbtc-info.services.spec.ts | 62 +++++++- .../services/icrc-accounts.services.spec.ts | 16 ++ .../services/imported-tokens.services.spec.ts | 20 +++ .../lib/services/neurons.services.spec.ts | 83 +++++++++- .../public/proposals.services.spec.ts | 17 +++ .../lib/services/sns-neurons.services.spec.ts | 27 +++- .../tests/lib/services/sns.services.spec.ts | 143 +++++++++++++++++- .../tests/lib/services/utils.services.spec.ts | 55 ++++++- .../src/tests/lib/utils/env.utils.spec.ts | 21 +++ 21 files changed, 562 insertions(+), 73 deletions(-) diff --git a/frontend/src/lib/services/canisters.services.ts b/frontend/src/lib/services/canisters.services.ts index 4c0fd4842db..6031208c9ff 100644 --- a/frontend/src/lib/services/canisters.services.ts +++ b/frontend/src/lib/services/canisters.services.ts @@ -22,7 +22,7 @@ import type { Account } from "$lib/types/account"; import { LedgerErrorMessage } from "$lib/types/ledger.errors"; import { assertEnoughAccountFunds } from "$lib/utils/accounts.utils"; import { isController } from "$lib/utils/canisters.utils"; -import { notForceCallStrategy } from "$lib/utils/env.utils"; +import { isLastCall } from "$lib/utils/env.utils"; import { mapCanisterErrorToToastMessage, toToastError, @@ -48,10 +48,10 @@ export const listCanisters = async ({ strategy: FORCE_CALL_STRATEGY, onLoad: ({ response: canisters, certified }) => canistersStore.setCanisters({ canisters, certified }), - onError: ({ error: err, certified }) => { + onError: ({ error: err, certified, strategy }) => { console.error(err); - if (!certified && notForceCallStrategy()) { + if (!isLastCall({ strategy, certified })) { return; } diff --git a/frontend/src/lib/services/ckbtc-info.services.ts b/frontend/src/lib/services/ckbtc-info.services.ts index 4a1f2a655f2..7e5cd92723e 100644 --- a/frontend/src/lib/services/ckbtc-info.services.ts +++ b/frontend/src/lib/services/ckbtc-info.services.ts @@ -5,7 +5,7 @@ import { ckBTCInfoStore } from "$lib/stores/ckbtc-info.store"; import { toastsError } from "$lib/stores/toasts.store"; import type { CkBTCAdditionalCanisters } from "$lib/types/ckbtc-canisters"; import type { UniverseCanisterId } from "$lib/types/universe"; -import { notForceCallStrategy } from "$lib/utils/env.utils"; +import { isLastCall } from "$lib/utils/env.utils"; import { isUniverseCkBTC } from "$lib/utils/universe.utils"; import type { MinterInfo } from "@dfinity/ckbtc"; import { isNullish } from "@dfinity/utils"; @@ -50,8 +50,8 @@ export const loadCkBTCInfo = async ({ canisterId: universeId, info, }), - onError: ({ error: err, certified }) => { - if (!certified && notForceCallStrategy()) { + onError: ({ error: err, certified, strategy }) => { + if (!isLastCall({ strategy, certified })) { return; } diff --git a/frontend/src/lib/services/icrc-accounts.services.ts b/frontend/src/lib/services/icrc-accounts.services.ts index 58da969b756..ad610839479 100644 --- a/frontend/src/lib/services/icrc-accounts.services.ts +++ b/frontend/src/lib/services/icrc-accounts.services.ts @@ -20,6 +20,7 @@ import { toastsError } from "$lib/stores/toasts.store"; import { tokensStore } from "$lib/stores/tokens.store"; import type { Account } from "$lib/types/account"; import type { IcrcTokenMetadata } from "$lib/types/icrc"; +import { isLastCall } from "$lib/utils/env.utils"; import { toToastError } from "$lib/utils/error.utils"; import { isImportedToken } from "$lib/utils/imported-tokens.utils"; import { ledgerErrorToToastError } from "$lib/utils/sns-ledger.utils"; @@ -79,10 +80,8 @@ export const loadIcrcToken = ({ return; } - const strategy = certified ? FORCE_CALL_STRATEGY : "query"; - return queryAndUpdate({ - strategy, + strategy: certified ? FORCE_CALL_STRATEGY : "query", identityType: "current", request: ({ certified, identity }) => queryIcrcToken({ @@ -92,9 +91,9 @@ export const loadIcrcToken = ({ }), onLoad: async ({ response: token, certified }) => tokensStore.setToken({ certified, canisterId: ledgerCanisterId, token }), - onError: ({ error: err, certified }) => { + onError: ({ error: err, certified, strategy }) => { // Explicitly handle only UPDATE errors - if (!certified && strategy !== "query") { + if (!isLastCall({ strategy, certified })) { return; } diff --git a/frontend/src/lib/services/imported-tokens.services.ts b/frontend/src/lib/services/imported-tokens.services.ts index 0388d7ba697..af7696f78bb 100644 --- a/frontend/src/lib/services/imported-tokens.services.ts +++ b/frontend/src/lib/services/imported-tokens.services.ts @@ -17,7 +17,7 @@ import { } from "$lib/stores/imported-tokens.store"; import { toastsError, toastsSuccess } from "$lib/stores/toasts.store"; import type { ImportedTokenData } from "$lib/types/imported-tokens"; -import { notForceCallStrategy } from "$lib/utils/env.utils"; +import { isLastCall } from "$lib/utils/env.utils"; import { fromImportedTokenData, toImportedTokenData, @@ -45,7 +45,7 @@ export const loadImportedTokens = async ({ }); failedImportedTokenLedgerIdsStore.reset(); }, - onError: ({ error: err, certified }) => { + onError: ({ error: err, certified, strategy }) => { console.error(err); if (ignoreAccountNotFoundError && err instanceof AccountNotFoundError) { @@ -59,7 +59,7 @@ export const loadImportedTokens = async ({ return; } - if (!certified && notForceCallStrategy()) { + if (!isLastCall({ strategy, certified })) { return; } diff --git a/frontend/src/lib/services/neurons.services.ts b/frontend/src/lib/services/neurons.services.ts index acce0682bf1..408cfc8a5aa 100644 --- a/frontend/src/lib/services/neurons.services.ts +++ b/frontend/src/lib/services/neurons.services.ts @@ -30,7 +30,7 @@ import { assertEnoughAccountFunds, isAccountHardwareWallet, } from "$lib/utils/accounts.utils"; -import { notForceCallStrategy } from "$lib/utils/env.utils"; +import { isLastCall } from "$lib/utils/env.utils"; import { errorToString, mapNeuronErrorToToastMessage, @@ -266,8 +266,8 @@ export const listNeurons = async ({ callback?.(certified); }, - onError: ({ error, certified }) => { - if (!certified && notForceCallStrategy()) { + onError: ({ error, certified, strategy }) => { + if (!isLastCall({ strategy, certified })) { return; } @@ -908,10 +908,10 @@ export const loadNeuron = ({ } setNeuron({ neuron, certified }); }, - onError: ({ error, certified }) => { + onError: ({ error, certified, strategy }) => { console.error(error); - if (!certified && notForceCallStrategy()) { + if (!isLastCall({ strategy, certified })) { return; } catchError(error); diff --git a/frontend/src/lib/services/public/proposals.services.ts b/frontend/src/lib/services/public/proposals.services.ts index 53c3b08624b..fffc4b7e7c3 100644 --- a/frontend/src/lib/services/public/proposals.services.ts +++ b/frontend/src/lib/services/public/proposals.services.ts @@ -18,14 +18,14 @@ import { } from "$lib/stores/proposals.store"; import { toastsError, toastsShow } from "$lib/stores/toasts.store"; import { hashCode } from "$lib/utils/dev.utils"; -import { isForceCallStrategy } from "$lib/utils/env.utils"; +import { isLastCall } from "$lib/utils/env.utils"; import { errorToString, isPayloadSizeError } from "$lib/utils/error.utils"; import { excludeProposals, proposalsHaveSameIds, } from "$lib/utils/proposals.utils"; -import type { Identity } from "@dfinity/agent"; import type { ProposalId, ProposalInfo } from "@dfinity/nns"; +import { nonNullish } from "@dfinity/utils"; import { get } from "svelte/store"; import { getCurrentIdentity } from "../auth.services"; import { @@ -38,20 +38,16 @@ import { const handleFindProposalsError = ({ error: err, certified, - identity, + strategy, mutableProposalsStore, }: { error: unknown; certified: boolean; - identity: Identity; + strategy: QueryAndUpdateStrategy; mutableProposalsStore: SingleMutationProposalsStore; }) => { console.error(err); - if ( - certified || - identity.getPrincipal().isAnonymous() || - isForceCallStrategy() - ) { + if (isLastCall({ strategy, certified })) { mutableProposalsStore.setProposals({ proposals: [], certified }); const resultsTooLarge = isPayloadSizeError(err); @@ -88,7 +84,7 @@ export const listProposals = async ({ onError: (onErrorParams: { error: unknown; certified: boolean; - identity: Identity; + strategy: QueryAndUpdateStrategy; }) => { handleFindProposalsError({ ...onErrorParams, @@ -136,7 +132,7 @@ export const listNextProposals = async ({ onError: (onErrorParams: { error: unknown; certified: boolean; - identity: Identity; + strategy: QueryAndUpdateStrategy; }) => { handleFindProposalsError({ ...onErrorParams, @@ -163,7 +159,7 @@ const findProposals = async ({ const validateResponses = ({ trustedProposals, untrustedProposals, - }:{ + }: { trustedProposals: ProposalInfo[]; untrustedProposals: ProposalInfo[]; }) => { @@ -203,10 +199,10 @@ const findProposals = async ({ includeStatus: filters.status, certified, }), - onLoad: ({ response: proposals, certified }) => { + onLoad: ({ response: proposals, certified, strategy }) => { if (!certified) { uncertifiedProposals = proposals; - onLoad({ response: proposals, certified }); + onLoad({ response: proposals, certified, strategy }); return; } @@ -217,7 +213,7 @@ const findProposals = async ({ }); } - onLoad({ response: proposals, certified }); + onLoad({ response: proposals, certified, strategy }); }, onError, logMessage: `Syncing proposals ${ @@ -252,12 +248,14 @@ export const loadProposal = async ({ ) => { console.error(erroneusResponse); - if (silentErrorMessages !== true && ( - erroneusResponse.certified || ( - strategy === "query" || - identity.getPrincipal().isAnonymous() - ) - )) { + if ( + silentErrorMessages !== true && + nonNullish(erroneusResponse) && + isLastCall({ + strategy: erroneusResponse.strategy, + certified: erroneusResponse.certified, + }) + ) { const details = errorToString(erroneusResponse?.error); toastsShow({ labelKey: "error.proposal_not_found", @@ -274,9 +272,9 @@ export const loadProposal = async ({ try { return await getProposal({ proposalId, - onLoad: ({ response: proposal, certified }) => { + onLoad: ({ response: proposal, certified, strategy }) => { if (!proposal) { - catchError({ certified, error: undefined, identity }); + catchError({ certified, strategy, error: undefined, identity }); return; } @@ -288,7 +286,15 @@ export const loadProposal = async ({ strategy, }); } catch (error: unknown) { - catchError({ certified: true, error, identity }); + catchError({ + // `certified` and `strategy` are not meaningful since we are outside the + // `queryAndUpdate` context. We just pass values that act as if this is + // the last call. + certified: true, + strategy: "query_and_update", + error, + identity, + }); } }; diff --git a/frontend/src/lib/services/public/sns.services.ts b/frontend/src/lib/services/public/sns.services.ts index f5624b7983c..e9ae3ce2269 100644 --- a/frontend/src/lib/services/public/sns.services.ts +++ b/frontend/src/lib/services/public/sns.services.ts @@ -6,7 +6,7 @@ import { queryAndUpdate } from "$lib/services/utils.services"; import { snsAggregatorIncludingAbortedProjectsStore } from "$lib/stores/sns-aggregator.store"; import { snsProposalsStore } from "$lib/stores/sns.store"; import { toastsError } from "$lib/stores/toasts.store"; -import { isForceCallStrategy } from "$lib/utils/env.utils"; +import { isLastCall } from "$lib/utils/env.utils"; import { toToastError } from "$lib/utils/error.utils"; import { ProposalStatus, Topic, type ProposalInfo } from "@dfinity/nns"; import { Principal } from "@dfinity/principal"; @@ -79,13 +79,11 @@ export const loadProposalsSnsCF = async (): Promise => { proposals, certified, }), - onError: ({ error: err, certified, identity }) => { + onError: ({ error: err, certified, strategy }) => { console.error(err); if ( - certified || - identity.getPrincipal().isAnonymous() || - isForceCallStrategy() + isLastCall({ strategy, certified }) ) { snsProposalsStore.reset(); diff --git a/frontend/src/lib/services/sns-neurons.services.ts b/frontend/src/lib/services/sns-neurons.services.ts index b70004e5447..6803738d58b 100644 --- a/frontend/src/lib/services/sns-neurons.services.ts +++ b/frontend/src/lib/services/sns-neurons.services.ts @@ -31,7 +31,7 @@ import { } from "$lib/stores/sns-neurons.store"; import { toastsError, toastsSuccess } from "$lib/stores/toasts.store"; import type { Account } from "$lib/types/account"; -import { notForceCallStrategy } from "$lib/utils/env.utils"; +import { isLastCall } from "$lib/utils/env.utils"; import { toToastError } from "$lib/utils/error.utils"; import { ledgerErrorToToastError } from "$lib/utils/sns-ledger.utils"; import { @@ -89,10 +89,10 @@ export const syncSnsNeurons = async ( certified, }); }, - onError: ({ error: err, certified }) => { + onError: ({ error: err, certified, strategy }) => { console.error(err); - if (!certified && notForceCallStrategy()) { + if (!isLastCall({ strategy, certified })) { return; } diff --git a/frontend/src/lib/services/sns.services.ts b/frontend/src/lib/services/sns.services.ts index 3cecfaa9539..0e887245e37 100644 --- a/frontend/src/lib/services/sns.services.ts +++ b/frontend/src/lib/services/sns.services.ts @@ -14,10 +14,7 @@ import { } from "$lib/stores/sns.store"; import { toastsError } from "$lib/stores/toasts.store"; import type { SnsSwapCommitment } from "$lib/types/sns"; -import { - isForceCallStrategy, - notForceCallStrategy, -} from "$lib/utils/env.utils"; +import { isLastCall } from "$lib/utils/env.utils"; import { toToastError } from "$lib/utils/error.utils"; import { getSwapCanisterAccount } from "$lib/utils/sns.utils"; import type { AccountIdentifier } from "@dfinity/ledger-icp"; @@ -73,10 +70,10 @@ export const loadSnsSwapCommitments = async (): Promise => { }); } }, - onError: ({ error: err, certified }) => { + onError: ({ error: err, certified, strategy }) => { console.error(err); - if (!certified && notForceCallStrategy()) { + if (!isLastCall({ strategy, certified })) { return; } @@ -123,14 +120,10 @@ export const loadSnsSwapCommitment = async ({ }), onLoad: ({ response: swapCommitment, certified }) => snsSwapCommitmentsStore.setSwapCommitment({ swapCommitment, certified }), - onError: ({ error: err, certified, identity }) => { + onError: ({ error: err, certified, strategy }) => { console.error(err); - if ( - certified || - identity.getPrincipal().isAnonymous() || - isForceCallStrategy() - ) { + if (isLastCall({ strategy, certified })) { toastsError( toToastError({ err, diff --git a/frontend/src/lib/services/utils.services.ts b/frontend/src/lib/services/utils.services.ts index 73b4423b527..e6686702498 100644 --- a/frontend/src/lib/services/utils.services.ts +++ b/frontend/src/lib/services/utils.services.ts @@ -8,11 +8,13 @@ import { export type QueryAndUpdateOnResponse = (options: { certified: boolean; + strategy: QueryAndUpdateStrategy; response: R; }) => void; export type QueryAndUpdateOnError = (options: { certified: boolean; + strategy: QueryAndUpdateStrategy; error: E; // The identity used for the request identity: Identity; @@ -83,12 +85,12 @@ export const queryAndUpdate = async ({ request({ certified, identity }) .then((response) => { if (certifiedDone) return; - onLoad({ certified, response }); + onLoad({ certified, strategy: currentStrategy, response }); log({ postfix: ` ${certified ? "update" : "query"} complete.` }); }) .catch((error: E) => { if (certifiedDone) return; - onError?.({ certified, error, identity }); + onError?.({ certified, strategy: currentStrategy, error, identity }); }) .finally(() => (certifiedDone = certifiedDone || certified)); diff --git a/frontend/src/lib/utils/env.utils.ts b/frontend/src/lib/utils/env.utils.ts index 593a9e89680..924f6433570 100644 --- a/frontend/src/lib/utils/env.utils.ts +++ b/frontend/src/lib/utils/env.utils.ts @@ -48,4 +48,17 @@ export const isLocalhost = (hostname: string) => export const isForceCallStrategy = (): boolean => FORCE_CALL_STRATEGY === "query"; +// Given the used strategy and whether the current call is certified, returns +// whether this is the last call. +// +// If the strategy is "query_and_update", the last call is the certied one. +// Otherwise there is only a single call and the current call is the last one. +export const isLastCall = ({ + strategy, + certified, +}: { + strategy: "query_and_update" | "query" | "update"; + certified: boolean; +}): boolean => certified || strategy !== "query_and_update"; + export const notForceCallStrategy = (): boolean => !isForceCallStrategy(); diff --git a/frontend/src/tests/lib/services/canisters.services.spec.ts b/frontend/src/tests/lib/services/canisters.services.spec.ts index 0a9f4994a42..02438d10ad5 100644 --- a/frontend/src/tests/lib/services/canisters.services.spec.ts +++ b/frontend/src/tests/lib/services/canisters.services.spec.ts @@ -58,7 +58,7 @@ describe("canisters-services", () => { let spyGetExchangeRate: SpyInstance; beforeEach(() => { - vi.clearAllMocks(); + vi.restoreAllMocks(); vi.spyOn(console, "error").mockImplementation(() => undefined); toastsStore.reset(); @@ -127,6 +127,54 @@ describe("canisters-services", () => { resetIdentity(); }); + + it("should reset canister and show toast on error", async () => { + const errorMessage = "no canisters found"; + spyQueryCanisters = vi + .spyOn(api, "queryCanisters") + .mockRejectedValue(new Error(errorMessage)); + + canistersStore.setCanisters({ + canisters: mockCanisters, + certified: true, + }); + + expect(get(canistersStore).canisters).not.toEqual([]); + expect(get(toastsStore)).toEqual([]); + + await listCanisters({}); + + expect(get(canistersStore).canisters).toEqual([]); + expect(get(toastsStore)[0]).toMatchObject({ + level: "error", + text: `There was an unexpected issue while searching for the canisters. ${errorMessage}`, + }); + }); + + it("should not reset canisters or show toast on uncertified error", async () => { + const errorMessage = "no canisters found"; + spyQueryCanisters = vi + .spyOn(api, "queryCanisters") + .mockImplementation(async ({ certified }) => { + if (!certified) { + throw new Error(errorMessage); + } + return mockCanisters; + }); + + canistersStore.setCanisters({ + canisters: mockCanisters, + certified: true, + }); + + expect(get(canistersStore).canisters).not.toEqual([]); + expect(get(toastsStore)).toEqual([]); + + await listCanisters({}); + + expect(get(canistersStore).canisters).not.toEqual([]); + expect(get(toastsStore)).toEqual([]); + }); }); describe("attachCanister", () => { diff --git a/frontend/src/tests/lib/services/ckbtc-info.services.spec.ts b/frontend/src/tests/lib/services/ckbtc-info.services.spec.ts index 8ec7657a7cf..8d42a2698f0 100644 --- a/frontend/src/tests/lib/services/ckbtc-info.services.spec.ts +++ b/frontend/src/tests/lib/services/ckbtc-info.services.spec.ts @@ -13,13 +13,15 @@ import { resetIdentity, } from "$tests/mocks/auth.store.mock"; import { mockCkBTCMinterInfo } from "$tests/mocks/ckbtc-minter.mock"; +import { toastsStore } from "@dfinity/gix-components"; import { waitFor } from "@testing-library/svelte"; import { get } from "svelte/store"; describe("ckbtc-info-services", () => { beforeEach(() => { - vi.clearAllMocks(); + vi.restoreAllMocks(); ckBTCInfoStore.reset(); + toastsStore.reset(); resetIdentity(); vi.spyOn(authServices, "getAuthenticatedIdentity").mockImplementation( mockGetIdentity @@ -61,6 +63,64 @@ describe("ckbtc-info-services", () => { }); }); + it("should reset univers and show toast on error", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); + const errorMessage = "Error message"; + spyGetMinterInfo = vi + .spyOn(minterApi, "minterInfo") + .mockRejectedValue(new Error(errorMessage)); + + ckBTCInfoStore.setInfo({ + info: mockCkBTCMinterInfo, + certified: false, + canisterId: CKBTC_UNIVERSE_CANISTER_ID, + }); + + expect(get(ckBTCInfoStore)).not.toEqual({}); + expect(get(toastsStore)).toEqual([]); + + await services.loadCkBTCInfo({ + universeId: CKBTC_UNIVERSE_CANISTER_ID, + minterCanisterId: CKBTC_MINTER_CANISTER_ID, + }); + + expect(get(ckBTCInfoStore)).toEqual({}); + expect(get(toastsStore)[0]).toMatchObject({ + level: "error", + text: `Sorry, there was an error loading the ckBTC minter information. ${errorMessage}`, + }); + }); + + it("should not reset univers or show toast on uncertified error", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); + const errorMessage = "Error message"; + spyGetMinterInfo = vi + .spyOn(minterApi, "minterInfo") + .mockImplementation(async ({ certified }) => { + if (!certified) { + throw new Error(errorMessage); + } + return mockCkBTCMinterInfo; + }); + + ckBTCInfoStore.setInfo({ + info: mockCkBTCMinterInfo, + certified: false, + canisterId: CKBTC_UNIVERSE_CANISTER_ID, + }); + + expect(get(ckBTCInfoStore)).not.toEqual({}); + expect(get(toastsStore)).toEqual([]); + + await services.loadCkBTCInfo({ + universeId: CKBTC_UNIVERSE_CANISTER_ID, + minterCanisterId: CKBTC_MINTER_CANISTER_ID, + }); + + expect(get(ckBTCInfoStore)).not.toEqual({}); + expect(get(toastsStore)).toEqual([]); + }); + it("should not call api if no universe is provided", async () => { await services.loadCkBTCInfo({ universeId: undefined, diff --git a/frontend/src/tests/lib/services/icrc-accounts.services.spec.ts b/frontend/src/tests/lib/services/icrc-accounts.services.spec.ts index 6e7544e9b61..24ba3b12271 100644 --- a/frontend/src/tests/lib/services/icrc-accounts.services.spec.ts +++ b/frontend/src/tests/lib/services/icrc-accounts.services.spec.ts @@ -560,6 +560,22 @@ describe("icrc-accounts-services", () => { ]); }); + it("displays no toast on uncertified error", async () => { + vi.spyOn(ledgerApi, "queryIcrcToken").mockImplementation( + async ({ certified }) => { + if (!certified) { + throw new Error("test"); + } + return mockToken; + } + ); + expect(ledgerApi.queryIcrcToken).not.toBeCalled(); + expect(get(toastsStore)).toEqual([]); + await loadIcrcToken({ ledgerCanisterId }); + expect(ledgerApi.queryIcrcToken).toBeCalledTimes(2); + expect(get(toastsStore)).toEqual([]); + }); + it("doesn't load imported token if in failed imported tokens store", async () => { importedTokensStore.set({ importedTokens: [ diff --git a/frontend/src/tests/lib/services/imported-tokens.services.spec.ts b/frontend/src/tests/lib/services/imported-tokens.services.spec.ts index 32ff9790ff8..4e9fb1af25b 100644 --- a/frontend/src/tests/lib/services/imported-tokens.services.spec.ts +++ b/frontend/src/tests/lib/services/imported-tokens.services.spec.ts @@ -106,6 +106,26 @@ describe("imported-tokens-services", () => { }); }); + it("should not display toast on uncertified error", async () => { + const spyToastError = vi.spyOn(toastsStore, "toastsError"); + vi.spyOn(importedTokensApi, "getImportedTokens").mockImplementation( + async ({ certified }) => { + if (!certified) { + throw testError; + } + return { + imported_tokens: [importedTokenA, importedTokenB], + }; + } + ); + + expect(spyToastError).not.toBeCalled(); + + await loadImportedTokens(); + + expect(spyToastError).not.toBeCalled(); + }); + it("should reset store on error", async () => { vi.spyOn(importedTokensApi, "getImportedTokens").mockRejectedValue( testError diff --git a/frontend/src/tests/lib/services/neurons.services.spec.ts b/frontend/src/tests/lib/services/neurons.services.spec.ts index 8f35b6465f3..bc30c2b6bce 100644 --- a/frontend/src/tests/lib/services/neurons.services.spec.ts +++ b/frontend/src/tests/lib/services/neurons.services.spec.ts @@ -345,9 +345,11 @@ describe("neurons-services", () => { }); describe("list neurons", () => { - const spyQueryNeurons = vi - .spyOn(api, "queryNeurons") - .mockResolvedValue(neurons); + const spyQueryNeurons = vi.spyOn(api, "queryNeurons"); + + beforeEach(() => { + spyQueryNeurons.mockResolvedValue(neurons); + }); it("should list neurons", async () => { const oldNeuronsList = get(definedNeuronsStore); @@ -372,6 +374,44 @@ describe("neurons-services", () => { expect(newNeuronsList).toEqual(neurons); }); + it("should reset store and show toast on error", async () => { + spyConsoleError.mockReturnValue(); + const errorMessage = "Test error"; + spyQueryNeurons.mockRejectedValue(new Error(errorMessage)); + + neuronsStore.pushNeurons({ neurons, certified: true }); + + expect(get(definedNeuronsStore)).not.toEqual([]); + expect(get(toastsStore)).toEqual([]); + + await listNeurons(); + + expect(get(definedNeuronsStore)).toEqual([]); + expectToastError( + `There was an unexpected issue while searching for the neurons. ${errorMessage}` + ); + }); + + it("should not reset store or show toast on uncertified error", async () => { + const errorMessage = "Test error"; + spyQueryNeurons.mockImplementation(async ({ certified }) => { + if (!certified) { + throw new Error(errorMessage); + } + return neurons; + }); + + neuronsStore.pushNeurons({ neurons, certified: true }); + + expect(get(definedNeuronsStore)).not.toEqual([]); + expect(get(toastsStore)).toEqual([]); + + await listNeurons(); + + expect(get(definedNeuronsStore)).not.toEqual([]); + expect(get(toastsStore)).toEqual([]); + }); + it("should not call api when called twice and cache is not reset", async () => { expect(spyQueryNeurons).not.toBeCalled(); await listNeurons(); @@ -1657,6 +1697,43 @@ describe("neurons-services", () => { expect(spyGetNeuron).toBeCalledTimes(2); }); + it("should show a toast on error", async () => { + spyConsoleError.mockReturnValue(); + const errorMessage = "error message"; + spyGetNeuron.mockRejectedValue(new Error(errorMessage)); + + expect(get(toastsStore)).toEqual([]); + + await loadNeuron({ + neuronId: mockNeuron.neuronId, + setNeuron: vi.fn(), + }); + + expectToastError( + `An error occurred while loading the neuron. ${errorMessage}` + ); + }); + + it("should not show a toast on uncertified error", async () => { + spyConsoleError.mockReturnValue(); + const errorMessage = "error message"; + spyGetNeuron.mockImplementation(async ({ certified }) => { + if (!certified) { + throw new Error(errorMessage); + } + return mockNeuron; + }); + + expect(get(toastsStore)).toEqual([]); + + await loadNeuron({ + neuronId: mockNeuron.neuronId, + setNeuron: vi.fn(), + }); + + expect(get(toastsStore)).toEqual([]); + }); + it("should call setNeuron even if the neuron doesn't have fullNeuron", async () => { const neuronId = 333_333n; const publicInfoNeuron = { diff --git a/frontend/src/tests/lib/services/public/proposals.services.spec.ts b/frontend/src/tests/lib/services/public/proposals.services.spec.ts index 0c53e5181a0..bb58b485a59 100644 --- a/frontend/src/tests/lib/services/public/proposals.services.spec.ts +++ b/frontend/src/tests/lib/services/public/proposals.services.spec.ts @@ -168,6 +168,23 @@ describe("proposals-services", () => { text: "There was an unexpected issue while searching for the proposals. Error message from api.", }); }); + + it("show no error message from api on uncertified error", async () => { + const errorMessage = "Error message from api."; + vi.spyOn(api, "queryProposals").mockImplementation(async ({certified}) => { + if (!certified) { + throw new Error(errorMessage); + } + return mockProposals; + }); + await listProposals({ + loadFinished: () => { + // do nothing here + }, + }); + + expect(get(toastsStore)).toEqual([]); + }); }); describe("load", () => { diff --git a/frontend/src/tests/lib/services/sns-neurons.services.spec.ts b/frontend/src/tests/lib/services/sns-neurons.services.spec.ts index ba168aa5741..47aea8bf6d7 100644 --- a/frontend/src/tests/lib/services/sns-neurons.services.spec.ts +++ b/frontend/src/tests/lib/services/sns-neurons.services.spec.ts @@ -98,7 +98,7 @@ describe("sns-neurons-services", () => { }; beforeEach(() => { - vi.clearAllMocks(); + vi.restoreAllMocks(); resetIdentity(); resetMockedConstants(); resetSnsProjects(); @@ -147,6 +147,31 @@ describe("sns-neurons-services", () => { expect(store[mockPrincipal.toText()]).toBeUndefined(); expect(spyQuery).toBeCalled(); }); + + it("should not empty store if query call fails but update call succeeds", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); + + snsNeuronsStore.setNeurons({ + rootCanisterId: mockPrincipal, + neurons: [mockSnsNeuron], + certified: true, + }); + const spyQuery = vi + .spyOn(governanceApi, "querySnsNeurons") + .mockImplementation(async ({ certified }) => { + if (!certified) { + throw new Error(); + } + return [neuron]; + }); + + await syncSnsNeurons(mockPrincipal); + + await tick(); + const store = get(snsNeuronsStore); + expect(store[mockPrincipal.toText()]).not.toBeUndefined(); + expect(spyQuery).toBeCalled(); + }); }); describe("loadSnsNeurons", () => { diff --git a/frontend/src/tests/lib/services/sns.services.spec.ts b/frontend/src/tests/lib/services/sns.services.spec.ts index cabc000bc29..d42308bbc51 100644 --- a/frontend/src/tests/lib/services/sns.services.spec.ts +++ b/frontend/src/tests/lib/services/sns.services.spec.ts @@ -19,6 +19,7 @@ import { advanceTime, runResolvedPromises, } from "$tests/utils/timers.test-utils"; +import { toastsStore } from "@dfinity/gix-components"; import { AccountIdentifier } from "@dfinity/ledger-icp"; import type { SnsGetDerivedStateResponse, @@ -45,10 +46,11 @@ describe("sns-services", () => { resetIdentity(); vi.useFakeTimers(); vi.clearAllTimers(); - vi.clearAllMocks(); + vi.restoreAllMocks(); snsSwapCommitmentsStore.reset(); resetSnsProjects(); snsDerivedStateStore.reset(); + toastsStore.reset(); }); describe("getSwapAccount", () => { @@ -73,6 +75,86 @@ describe("sns-services", () => { expect(store).toHaveLength(commitments.length); }); + it("should reset store and show toast on error", async () => { + vi.spyOn(console, "error").mockReturnValue(); + const errorMessage = "Error fetching commitments"; + const spy = vi + .spyOn(api, "querySnsSwapCommitments") + .mockRejectedValue(new Error(errorMessage)); + + setSnsProjects([ + { + rootCanisterId: rootCanisterId1, + lifecycle: SnsSwapLifecycle.Open, + }, + { + rootCanisterId: rootCanisterId2, + lifecycle: SnsSwapLifecycle.Open, + }, + ]); + + snsSwapCommitmentsStore.setSwapCommitment({ + swapCommitment: mockSnsSwapCommitment(principal(0)), + certified: true, + }); + + expect(get(snsSwapCommitmentsStore)).not.toBe(undefined); + expect(get(toastsStore)).toEqual([]); + + await loadSnsSwapCommitments(); + expect(spy).toBeCalled(); + + expect(get(snsSwapCommitmentsStore)).toBe(undefined); + expect(get(toastsStore)).toMatchObject([ + { + level: "error", + text: `There was an unexpected error while loading the commitments of all deployed projects. ${errorMessage}`, + }, + ]); + }); + + it("should not reset store or show toast on uncertified error", async () => { + vi.spyOn(console, "error").mockReturnValue(); + const commitment1 = mockSnsSwapCommitment(principal(0)); + const commitment2 = mockSnsSwapCommitment(principal(1)); + const commitments = [commitment1, commitment2]; + + const errorMessage = "Error fetching commitments"; + const spy = vi + .spyOn(api, "querySnsSwapCommitments") + .mockImplementation(async ({ certified }) => { + if (!certified) { + throw new Error(errorMessage); + } + return commitments; + }); + + setSnsProjects([ + { + rootCanisterId: rootCanisterId1, + lifecycle: SnsSwapLifecycle.Open, + }, + { + rootCanisterId: rootCanisterId2, + lifecycle: SnsSwapLifecycle.Open, + }, + ]); + + snsSwapCommitmentsStore.setSwapCommitment({ + swapCommitment: mockSnsSwapCommitment(principal(0)), + certified: true, + }); + + expect(get(snsSwapCommitmentsStore)).not.toBe(undefined); + expect(get(toastsStore)).toEqual([]); + + await loadSnsSwapCommitments(); + expect(spy).toBeCalled(); + + expect(get(snsSwapCommitmentsStore)).not.toBe(undefined); + expect(get(toastsStore)).toEqual([]); + }); + it("should not call api if they are loaded in store", async () => { setSnsProjects([ { @@ -228,6 +310,65 @@ describe("sns-services", () => { expect(commitmentInStore.swapCommitment).toEqual(commitment1); }); + it("should show toast on error", async () => { + vi.spyOn(console, "error").mockReturnValue(); + const errorMessage = "Error fetching commitment"; + queryCommitmentSpy = vi + .spyOn(api, "querySnsSwapCommitment") + .mockRejectedValue(new Error(errorMessage)); + + expect(get(snsSwapCommitmentsStore)).toBeUndefined(); + expect(queryCommitmentSpy).toBeCalledTimes(0); + expect(get(toastsStore)).toEqual([]); + + await loadSnsSwapCommitment({ + rootCanisterId: commitment1.rootCanisterId.toText(), + forceFetch: false, + }); + await runResolvedPromises(); + + expect(get(snsSwapCommitmentsStore)).toBeUndefined(); + expect(queryCommitmentSpy).toBeCalledTimes(2); + expect(get(toastsStore)).toMatchObject([ + { + level: "error", + text: `There was an unexpected error while loading the commitment of the project. ${errorMessage}`, + }, + ]); + }); + + it("should not show toast on uncertified error", async () => { + vi.spyOn(console, "error").mockReturnValue(); + const errorMessage = "Error fetching commitment"; + const commitment = mockSnsSwapCommitment(principal(0)); + queryCommitmentSpy = vi + .spyOn(api, "querySnsSwapCommitment") + .mockImplementation(async ({ certified }) => { + if (!certified) { + throw new Error(errorMessage); + } + return commitment; + }); + + expect(get(snsSwapCommitmentsStore)).toBeUndefined(); + expect(queryCommitmentSpy).toBeCalledTimes(0); + expect(get(toastsStore)).toEqual([]); + + await loadSnsSwapCommitment({ + rootCanisterId: commitment1.rootCanisterId.toText(), + forceFetch: false, + }); + await runResolvedPromises(); + + const commitmentInStore = get(snsSwapCommitmentsStore).find( + ({ swapCommitment: { rootCanisterId } }) => + commitment1.rootCanisterId.toText() === rootCanisterId.toText() + ); + expect(commitmentInStore.swapCommitment).toEqual(commitment); + expect(queryCommitmentSpy).toBeCalledTimes(2); + expect(get(toastsStore)).toEqual([]); + }); + it("should not call api if they are loaded in store", async () => { snsSwapCommitmentsStore.setSwapCommitment({ swapCommitment: commitment1, diff --git a/frontend/src/tests/lib/services/utils.services.spec.ts b/frontend/src/tests/lib/services/utils.services.spec.ts index 8fccc3902cd..3776947a960 100644 --- a/frontend/src/tests/lib/services/utils.services.spec.ts +++ b/frontend/src/tests/lib/services/utils.services.spec.ts @@ -34,6 +34,29 @@ describe("api-utils", () => { expect(call).rejects.toThrowError(); }); + + it("should use 'query' strategy by default", async () => { + const testResponse = "test"; + const request = vi + .fn() + .mockImplementation(() => Promise.resolve(testResponse)); + const onLoad = vi.fn(); + const onError = vi.fn(); + + await queryAndUpdate({ + request, + onLoad, + onError, + identityType: "current", + }); + + expect(onLoad).toHaveBeenCalledTimes(1); + expect(onLoad).toHaveBeenCalledWith({ + certified: false, + strategy: "query", + response: testResponse, + }); + }); }); describe("logged in user", () => { @@ -42,9 +65,10 @@ describe("api-utils", () => { resetIdentity(); }); it("should request twice", async () => { + const response = { certified: true }; const request = vi .fn() - .mockImplementation(() => Promise.resolve({ certified: true })); + .mockImplementation(() => Promise.resolve(response)); const onLoad = vi.fn(); const onError = vi.fn(); @@ -56,6 +80,16 @@ describe("api-utils", () => { expect(request).toHaveBeenCalledTimes(2); expect(onLoad).toHaveBeenCalledTimes(2); + expect(onLoad).toHaveBeenCalledWith({ + certified: false, + strategy: "query_and_update", + response, + }); + expect(onLoad).toHaveBeenCalledWith({ + certified: true, + strategy: "query_and_update", + response, + }); expect(onError).not.toBeCalled(); }); @@ -114,6 +148,12 @@ describe("api-utils", () => { }); expect(requestCertified.sort()).toEqual([false]); + expect(onLoad).toHaveBeenCalledTimes(1); + expect(onLoad).toHaveBeenCalledWith({ + certified: false, + strategy: "query", + response: undefined, + }); }); it('should support "update" strategy', async () => { @@ -133,6 +173,12 @@ describe("api-utils", () => { }); expect(requestCertified.sort()).toEqual([true]); + expect(onLoad).toHaveBeenCalledTimes(1); + expect(onLoad).toHaveBeenCalledWith({ + certified: true, + strategy: "update", + response: undefined, + }); }); it("should catch errors", async () => { @@ -152,6 +198,13 @@ describe("api-utils", () => { expect(onError).toBeCalledTimes(2); expect(onError).toBeCalledWith({ certified: false, + strategy: "query_and_update", + error: "test", + identity: mockIdentity, + }); + expect(onError).toBeCalledWith({ + certified: true, + strategy: "query_and_update", error: "test", identity: mockIdentity, }); diff --git a/frontend/src/tests/lib/utils/env.utils.spec.ts b/frontend/src/tests/lib/utils/env.utils.spec.ts index 01053e1cb41..456155df5d9 100644 --- a/frontend/src/tests/lib/utils/env.utils.spec.ts +++ b/frontend/src/tests/lib/utils/env.utils.spec.ts @@ -1,5 +1,6 @@ import { addRawToUrl, + isLastCall, isLocalhost, isNnsAlternativeOrigin, } from "$lib/utils/env.utils"; @@ -127,4 +128,24 @@ describe("env-utils", () => { expect(isLocalhost("xxxx.localhost")).toBe(true); }); }); + + describe("isLastCall", () => { + it("should return true when certified", () => { + expect( + isLastCall({ strategy: "query_and_update", certified: true }) + ).toBe(true); + expect(isLastCall({ strategy: "update", certified: true })).toBe(true); + }); + + it("should return true for single call", () => { + expect(isLastCall({ strategy: "query", certified: false })).toBe(true); + expect(isLastCall({ strategy: "update", certified: true })).toBe(true); + }); + + it("should return false for query of query_and_update ", () => { + expect( + isLastCall({ strategy: "query_and_update", certified: false }) + ).toBe(false); + }); + }); });