Skip to content

Commit

Permalink
Don't build wrappers for aborted SNSes (#5742)
Browse files Browse the repository at this point in the history
# Motivation

We were seeing errors in NNS dapp because it tried to call
`get_buyer_state` on the swap canister of an aborted swap while that
canister was out of cycles. But we should not be trying to call anything
on aborted swaps because we don't care about aborted swaps.

Multiple things were happening.

1. We load aggregator data into
`snsAggregatorIncludingAbortedProjectsStore`,
[here](https://github.com/dfinity/nns-dapp/blob/94df88c1e01a9677ccbd760f6483edde53c6c11f/frontend/src/lib/services/public/sns.services.ts#L51C5-L51C47).
From that store, we derive
[snsAggregatorStore](https://github.com/dfinity/nns-dapp/blob/94df88c1e01a9677ccbd760f6483edde53c6c11f/frontend/src/lib/stores/sns-aggregator.store.ts#L34),
which has aborted SNS filtered out.
2. But on the Launchpad, when we try to load the swap commitments of the
user, we don't use the list of SNSes from the aggregator. We call
[loadSnsWrappers](https://github.com/dfinity/nns-dapp/blob/94df88c1e01a9677ccbd760f6483edde53c6c11f/frontend/src/lib/api/sns-wrapper.api.ts#L157),
which gets the list from SNSes from the SNS-W and then from
rootCanisterId, it gets the other canister IDs. This will fail for 8 of
the root canisters because they no longer exist. We detect these
failures and ignore those SNSes. The successful SNSes are then cached.
3. But when the aggregator data is loaded, we [populate this same
cache](https://github.com/dfinity/nns-dapp/blob/94df88c1e01a9677ccbd760f6483edde53c6c11f/frontend/src/lib/services/public/sns.services.ts#L33-L42)
with wrappers, without having to get canister IDs from the root
canisters, but before filtering out the aborted SNSes.
4. So if after this, we again try to get swap commitments, the aborted
SNSes are now included in the wrappers cache and then we try to call
`get_buyer_state` on them.

In this PR we just skip the aborted SNSes when populating the wrappers
cache from aggregator data.

This makes the error go away. But ideally we only construct wrappers
from aggregator data and stop calling SNS-W and root canisters
altogether. That's for future PRs.

# Changes

1. Skip aborted SNSes when creating SNS wrappers from aggregator data.

# Tests

1. Unit test added.
2. Tested against mainnet before the offending canister was topped up,
which also made the error go away.

# Todos

- [x] Add entry to changelog (if necessary).
  • Loading branch information
dskloetd authored Nov 8, 2024
1 parent 699e875 commit 8509c8c
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 2 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG-Nns-Dapp-unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ proposal is successful, the changes it released will be moved from this file to

#### Fixed

* Stop trying to get swap commitments from aborted SNSes.

#### Security

#### Not Published
Expand Down
6 changes: 5 additions & 1 deletion frontend/src/lib/services/public/sns.services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ 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";
import { SnsSwapLifecycle } from "@dfinity/sns";
import { getCurrentIdentity } from "../auth.services";

export const loadSnsProjects = async (): Promise<void> => {
Expand All @@ -19,7 +20,7 @@ export const loadSnsProjects = async (): Promise<void> => {
// We load the wrappers to avoid making calls to SNS-W and Root canister for each project.
// The SNS Aggregator gives us the canister ids of the SNS projects.
await Promise.all(
aggregatorData.map(async ({ canister_ids }) => {
aggregatorData.map(async ({ canister_ids, lifecycle }) => {
const canisterIds = {
rootCanisterId: Principal.fromText(canister_ids.root_canister_id),
swapCanisterId: Principal.fromText(canister_ids.swap_canister_id),
Expand All @@ -29,6 +30,9 @@ export const loadSnsProjects = async (): Promise<void> => {
ledgerCanisterId: Principal.fromText(canister_ids.ledger_canister_id),
indexCanisterId: Principal.fromText(canister_ids.index_canister_id),
};
if (lifecycle.lifecycle === SnsSwapLifecycle.Aborted) {
return;
}
// Build certified and uncertified wrappers because SNS aggregator gives certified data.
await buildAndStoreWrapper({
identity,
Expand Down
47 changes: 46 additions & 1 deletion frontend/src/tests/lib/services/public/sns.services.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import { clearSnsAggregatorCache } from "$lib/api-services/sns-aggregator.api-service";
import * as agent from "$lib/api/agent.api";
import * as aggregatorApi from "$lib/api/sns-aggregator.api";
import { clearWrapperCache, wrapper } from "$lib/api/sns-wrapper.api";
import {
clearWrapperCache,
wrappers as getWrappers,
wrapper,
} from "$lib/api/sns-wrapper.api";
import { snsFunctionsStore } from "$lib/derived/sns-functions.derived";
import { snsTotalTokenSupplyStore } from "$lib/derived/sns-total-token-supply.derived";
import { loadSnsProjects } from "$lib/services/public/sns.services";
Expand All @@ -26,6 +30,7 @@ import {
} from "$tests/mocks/sns.api.mock";
import { blockAllCallsTo } from "$tests/utils/module.test-utils";
import type { HttpAgent } from "@dfinity/agent";
import { SnsSwapLifecycle } from "@dfinity/sns";
import { get } from "svelte/store";
import { mock } from "vitest-mock-extended";

Expand Down Expand Up @@ -164,5 +169,45 @@ describe("SNS public services", () => {
expect(data).not.toBeUndefined();
expect(data?.totalSupply).toEqual(BigInt(totalSupply));
});

it("should build and store wrappers, only for non-aborted SNSes", async () => {
const committedSns1 = aggregatorSnsMockWith({
rootCanisterId: principal(0).toText(),
lifecycle: SnsSwapLifecycle.Committed,
});
const committedSns2 = aggregatorSnsMockWith({
rootCanisterId: principal(1).toText(),
lifecycle: SnsSwapLifecycle.Committed,
});
const abortedSns1 = aggregatorSnsMockWith({
rootCanisterId: principal(2).toText(),
lifecycle: SnsSwapLifecycle.Aborted,
});
const abortedSns2 = aggregatorSnsMockWith({
rootCanisterId: principal(3).toText(),
lifecycle: SnsSwapLifecycle.Aborted,
});

vi.spyOn(aggregatorApi, "querySnsProjects").mockResolvedValue([
committedSns1,
abortedSns1,
committedSns2,
abortedSns2,
]);

await loadSnsProjects();

const wrappers = await getWrappers({
identity: mockIdentity,
certified: true,
});
expect(wrappers).toHaveLength(2);
expect(wrappers.has(committedSns1.canister_ids.root_canister_id)).toBe(
true
);
expect(wrappers.has(committedSns2.canister_ids.root_canister_id)).toBe(
true
);
});
});
});

0 comments on commit 8509c8c

Please sign in to comment.