From 0994d30dfe7cc410c944aec0d53c5ae62dc0b3f3 Mon Sep 17 00:00:00 2001 From: Max Strasinsky <98811342+mstrasinskis@users.noreply.github.com> Date: Tue, 16 Jul 2024 11:00:11 +0200 Subject: [PATCH] Token table ordering (#5189) # Motivation Currently, tokens table has a fixed list of tokens considered important on top ordered (ICP, ckBTC, ckETH, ckUSDC) followed by SNS tokens ordered by creation date. This ordering seems random for new people entering the ecosystem, and only takes into account user balances after a user takes action. ## Suggested order: 1. ICP is always on top regardless of user balance. 2. Tokens where a user has a higher than 0 balance: a. First important token list in predefined order - ckBTC, ckETH, ckUSDC b. Sns tokens ordered alphabetically 3. Tokens where a user has 0 balance (same sub-groups as for tokens with balance): a. First important token list in predefined order - ckBTC, ckETH, ckUSDC b. Sns tokens ordered alphabetically # Changes - New `compareTokensForTokensTable` merged comparator. - Apply sorting to sign-in and sign-out token table. # Tests - Old tests were updated to the new order. - Tests for new comparators. - Test that the Token page display tokens in the expected order (ckUSDC token was added to the mock data). - Updated the e2e test because the order changed after getting SNS tokens. # Todos - [x] Add entry to changelog (if necessary). --- CHANGELOG-Nns-Dapp-unreleased.md | 1 + frontend/src/lib/utils/tokens-table.utils.ts | 47 ++++++ .../routes/(app)/(nns)/tokens/+page.svelte | 13 +- frontend/src/tests/e2e/sns-governance.spec.ts | 8 +- .../lib/utils/tokens-table.utils.spec.ts | 140 ++++++++++++++++++ .../src/tests/routes/app/tokens/page.spec.ts | 110 +++++++++++++- 6 files changed, 310 insertions(+), 9 deletions(-) create mode 100644 frontend/src/lib/utils/tokens-table.utils.ts create mode 100644 frontend/src/tests/lib/utils/tokens-table.utils.spec.ts diff --git a/CHANGELOG-Nns-Dapp-unreleased.md b/CHANGELOG-Nns-Dapp-unreleased.md index 03828772a8c..37f3b9b3a5b 100644 --- a/CHANGELOG-Nns-Dapp-unreleased.md +++ b/CHANGELOG-Nns-Dapp-unreleased.md @@ -20,6 +20,7 @@ proposal is successful, the changes it released will be moved from this file to #### Changed * Reduce the frequency of checking if SNS neurons need to be refreshed. +* New token table order. #### Deprecated diff --git a/frontend/src/lib/utils/tokens-table.utils.ts b/frontend/src/lib/utils/tokens-table.utils.ts new file mode 100644 index 00000000000..a3d0354b5fd --- /dev/null +++ b/frontend/src/lib/utils/tokens-table.utils.ts @@ -0,0 +1,47 @@ +import { OWN_CANISTER_ID_TEXT } from "$lib/constants/canister-ids.constants"; +import { CKBTC_UNIVERSE_CANISTER_ID } from "$lib/constants/ckbtc-canister-ids.constants"; +import { CKETH_UNIVERSE_CANISTER_ID } from "$lib/constants/cketh-canister-ids.constants"; +import { CKUSDC_UNIVERSE_CANISTER_ID } from "$lib/constants/ckusdc-canister-ids.constants"; +import type { UserToken } from "$lib/types/tokens-page"; +import { + createAscendingComparator, + createDescendingComparator, + mergeComparators, +} from "$lib/utils/responsive-table.utils"; +import { TokenAmountV2 } from "@dfinity/utils"; + +const getTokenBalanceOrZero = (token: UserToken) => + token.balance instanceof TokenAmountV2 ? token.balance.toUlps() : 0n; + +export const compareTokensIcpFirst = createDescendingComparator( + (token: UserToken) => token.universeId.toText() === OWN_CANISTER_ID_TEXT +); + +export const compareTokensWithBalanceFirst = createDescendingComparator( + (token: UserToken) => getTokenBalanceOrZero(token) > 0n +); + +// These tokens should be placed before others (but after ICP) +// because they have significance within the Internet Computer ecosystem and deserve to be highlighted. +// Where the fixed order maps to a descending order in the market cap of the underlying native tokens. +const ImportantCkTokenIds = [ + CKBTC_UNIVERSE_CANISTER_ID.toText(), + CKETH_UNIVERSE_CANISTER_ID.toText(), + CKUSDC_UNIVERSE_CANISTER_ID.toText(), +] + // To place other tokens (which get an index of -1) at the bottom. + .reverse(); +export const compareTokensByImportance = createDescendingComparator( + (token: UserToken) => ImportantCkTokenIds.indexOf(token.universeId.toText()) +); + +export const compareTokensAlphabetically = createAscendingComparator( + ({ title }: UserToken) => title.toLowerCase() +); + +export const compareTokensForTokensTable = mergeComparators([ + compareTokensIcpFirst, + compareTokensWithBalanceFirst, + compareTokensByImportance, + compareTokensAlphabetically, +]); diff --git a/frontend/src/routes/(app)/(nns)/tokens/+page.svelte b/frontend/src/routes/(app)/(nns)/tokens/+page.svelte index 45085bfec28..0e92fc84f47 100644 --- a/frontend/src/routes/(app)/(nns)/tokens/+page.svelte +++ b/frontend/src/routes/(app)/(nns)/tokens/+page.svelte @@ -28,7 +28,7 @@ import type { Account } from "$lib/types/account"; import { ActionType, type Action } from "$lib/types/actions"; import type { CkBTCAdditionalCanisters } from "$lib/types/ckbtc-canisters"; - import type { UserTokenData } from "$lib/types/tokens-page"; + import type { UserToken, UserTokenData } from "$lib/types/tokens-page"; import type { Universe, UniverseCanisterIdText } from "$lib/types/universe"; import { isIcrcTokenUniverse, @@ -39,6 +39,7 @@ import { Principal } from "@dfinity/principal"; import { nonNullish } from "@dfinity/utils"; import { onMount } from "svelte"; + import { compareTokensForTokensTable } from "$lib/utils/tokens-table.utils"; onMount(() => { loadCkBTCTokens(); @@ -195,15 +196,21 @@ } } }; + + const sortTokens = (tokens: UserToken[]) => + [...tokens].sort(compareTokensForTokensTable); {#if $authSignedInStore} - + {:else} {/if} diff --git a/frontend/src/tests/e2e/sns-governance.spec.ts b/frontend/src/tests/e2e/sns-governance.spec.ts index 2ff282a6880..55cb56c6471 100644 --- a/frontend/src/tests/e2e/sns-governance.spec.ts +++ b/frontend/src/tests/e2e/sns-governance.spec.ts @@ -18,8 +18,7 @@ test("Test SNS governance", async ({ page, context }) => { .getTokensTable() .getSnsRows(); expect(snsUniverseRows.length).toBeGreaterThanOrEqual(1); - const snsUniverseRow = snsUniverseRows[0]; - const snsProjectName = await snsUniverseRow.getProjectName(); + const snsProjectName = await snsUniverseRows[0].getProjectName(); // Our first test SNS project is always named "Alfa Centauri". expect(snsProjectName).toBe("Alfa Centauri"); @@ -28,6 +27,11 @@ test("Test SNS governance", async ({ page, context }) => { const askedAmount = 20; await appPo.getSnsTokens({ amount: askedAmount, name: snsProjectName }); + const snsUniverseRow = await appPo + .getTokensPo() + .getTokensPagePo() + .getTokensTable() + .getRowByName(snsProjectName); expect(await snsUniverseRow.getBalanceNumber()).toEqual(askedAmount); step("Stake a neuron"); diff --git a/frontend/src/tests/lib/utils/tokens-table.utils.spec.ts b/frontend/src/tests/lib/utils/tokens-table.utils.spec.ts new file mode 100644 index 00000000000..ea02a064f85 --- /dev/null +++ b/frontend/src/tests/lib/utils/tokens-table.utils.spec.ts @@ -0,0 +1,140 @@ +import { CKBTC_UNIVERSE_CANISTER_ID } from "$lib/constants/ckbtc-canister-ids.constants"; +import { CKETH_UNIVERSE_CANISTER_ID } from "$lib/constants/cketh-canister-ids.constants"; +import { CKUSDC_UNIVERSE_CANISTER_ID } from "$lib/constants/ckusdc-canister-ids.constants"; +import { + compareTokensAlphabetically, + compareTokensByImportance, + compareTokensIcpFirst, + compareTokensWithBalanceFirst, +} from "$lib/utils/tokens-table.utils"; +import { principal } from "$tests/mocks/sns-projects.mock"; +import { + ckBTCTokenBase, + createIcpUserToken, + createUserToken, + createUserTokenLoading, +} from "$tests/mocks/tokens-page.mock"; +import { TokenAmountV2 } from "@dfinity/utils"; + +describe("tokens-table.utils", () => { + const tokenWithBalance = (amount: bigint) => + createUserToken({ + universeId: principal(Number(amount)), + balance: TokenAmountV2.fromUlps({ + amount, + token: { name: `T${amount}`, symbol: `T${amount}`, decimals: 8 }, + }), + }); + const ckBTCToken = createUserToken({ + universeId: CKBTC_UNIVERSE_CANISTER_ID, + }); + const ckETHTToken = createUserToken({ + universeId: CKETH_UNIVERSE_CANISTER_ID, + }); + const ckUSDCToken = createUserToken({ + universeId: CKUSDC_UNIVERSE_CANISTER_ID, + }); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe("compareTokensIcpFirst", () => { + it("should keep ICP first", () => { + const icpToken = createIcpUserToken(); + const ckBTCUserToken = createUserToken(ckBTCTokenBase); + + expect(compareTokensIcpFirst(icpToken, ckBTCUserToken)).toEqual(-1); + expect(compareTokensIcpFirst(ckBTCUserToken, icpToken)).toEqual(1); + expect(compareTokensIcpFirst(icpToken, icpToken)).toEqual(0); + }); + }); + + describe("compareTokensWithBalanceFirst", () => { + it("should keep tokens with balance first", () => { + expect( + compareTokensWithBalanceFirst( + tokenWithBalance(1n), + tokenWithBalance(0n) + ) + ).toEqual(-1); + expect( + compareTokensWithBalanceFirst( + tokenWithBalance(0n), + tokenWithBalance(1n) + ) + ).toEqual(1); + expect( + compareTokensWithBalanceFirst( + tokenWithBalance(0n), + tokenWithBalance(0n) + ) + ).toEqual(0); + expect( + compareTokensWithBalanceFirst( + tokenWithBalance(1n), + tokenWithBalance(1n) + ) + ).toEqual(0); + }); + + it("should treat token in loading state as having a balance of 0", () => { + expect( + compareTokensWithBalanceFirst( + createUserTokenLoading(), + tokenWithBalance(1n) + ) + ).toEqual(1); + expect( + compareTokensWithBalanceFirst( + tokenWithBalance(1n), + createUserTokenLoading() + ) + ).toEqual(-1); + }); + }); + + describe("compareTokensByImportance", () => { + it("should sort tokens by importance", () => { + const token0 = tokenWithBalance(0n); + const expectedOrder = [ckBTCToken, ckETHTToken, ckUSDCToken, token0]; + expect( + [token0, ckUSDCToken, ckETHTToken, ckBTCToken].sort( + compareTokensByImportance + ) + ).toEqual(expectedOrder); + expect( + [ckBTCToken, ckETHTToken, ckUSDCToken, token0].sort( + compareTokensByImportance + ) + ).toEqual(expectedOrder); + expect( + [ckETHTToken, ckBTCToken, token0, ckUSDCToken].sort( + compareTokensByImportance + ) + ).toEqual(expectedOrder); + }); + }); + + describe("compareTokensAlphabetically", () => { + const annaToken = createUserToken({ title: "Anna" }); + const arnyToken = createUserToken({ title: "Arny" }); + const albertTokenLowerCase = createUserToken({ title: "albert" }); + const zorroToken = createUserToken({ title: "Zorro" }); + + it("should sort tokens by importance", () => { + expect(compareTokensAlphabetically(annaToken, arnyToken)).toEqual(-1); + expect(compareTokensAlphabetically(zorroToken, arnyToken)).toEqual(1); + expect(compareTokensAlphabetically(arnyToken, arnyToken)).toEqual(0); + }); + + it("should sort by titles while ignoring case sensitivity.", () => { + expect( + compareTokensAlphabetically(albertTokenLowerCase, annaToken) + ).toEqual(-1); + expect( + compareTokensAlphabetically(annaToken, albertTokenLowerCase) + ).toEqual(1); + }); + }); +}); diff --git a/frontend/src/tests/routes/app/tokens/page.spec.ts b/frontend/src/tests/routes/app/tokens/page.spec.ts index 6f5dbe625ea..dd30e39844f 100644 --- a/frontend/src/tests/routes/app/tokens/page.spec.ts +++ b/frontend/src/tests/routes/app/tokens/page.spec.ts @@ -9,8 +9,14 @@ import { CKETHSEPOLIA_UNIVERSE_CANISTER_ID, CKETH_UNIVERSE_CANISTER_ID, } from "$lib/constants/cketh-canister-ids.constants"; +import { + CKUSDC_INDEX_CANISTER_ID, + CKUSDC_LEDGER_CANISTER_ID, + CKUSDC_UNIVERSE_CANISTER_ID, +} from "$lib/constants/ckusdc-canister-ids.constants"; import { overrideFeatureFlagsStore } from "$lib/stores/feature-flags.store"; import { icrcAccountsStore } from "$lib/stores/icrc-accounts.store"; +import { icrcCanistersStore } from "$lib/stores/icrc-canisters.store"; import { tokensStore } from "$lib/stores/tokens.store"; import { numberToUlps } from "$lib/utils/token.utils"; import TokensRoute from "$routes/(app)/(nns)/tokens/+page.svelte"; @@ -28,6 +34,7 @@ import { mockCkETHToken } from "$tests/mocks/cketh-accounts.mock"; import { mockMainAccount } from "$tests/mocks/icp-accounts.store.mock"; import { mockSnsToken, principal } from "$tests/mocks/sns-projects.mock"; import { rootCanisterIdMock } from "$tests/mocks/sns.api.mock"; +import { mockCkUSDCToken } from "$tests/mocks/tokens.mock"; import { TokensRoutePo } from "$tests/page-objects/TokensRoute.page-object"; import { JestPageObjectElement } from "$tests/page-objects/jest.page-object"; import { setAccountsForTesting } from "$tests/utils/accounts.test-utils"; @@ -64,6 +71,7 @@ describe("Tokens route", () => { const pacmanBalanceE8s = 314000000n; const ckBTCDefaultBalanceE8s = 444556699n; let ckBTCBalanceE8s = ckBTCDefaultBalanceE8s; + const ckUSDCBalanceE8s = 111000000n; const amountCkBTCTransaction = 2; const amountCkBTCTransactionUlps = numberToUlps({ amount: amountCkBTCTransaction, @@ -95,6 +103,7 @@ describe("Tokens route", () => { vi.clearAllMocks(); icrcAccountsStore.reset(); tokensStore.reset(); + icrcCanistersStore.reset(); ckBTCBalanceE8s = ckBTCDefaultBalanceE8s; ckETHBalanceUlps = ckETHDefaultBalanceUlps; tetrisBalanceE8s = tetrisDefaultBalanceE8s; @@ -105,6 +114,7 @@ describe("Tokens route", () => { [CKTESTBTC_UNIVERSE_CANISTER_ID.toText()]: mockCkTESTBTCToken, [CKETH_UNIVERSE_CANISTER_ID.toText()]: mockCkETHToken, [CKETHSEPOLIA_UNIVERSE_CANISTER_ID.toText()]: mockCkTESTBTCToken, + [CKUSDC_UNIVERSE_CANISTER_ID.toText()]: mockCkUSDCToken, }; if (isNullish(tokenMap[canisterId.toText()])) { throw new Error( @@ -123,6 +133,7 @@ describe("Tokens route", () => { [CKBTC_UNIVERSE_CANISTER_ID.toText()]: ckBTCBalanceE8s, [CKTESTBTC_UNIVERSE_CANISTER_ID.toText()]: ckBTCBalanceE8s, [CKETH_UNIVERSE_CANISTER_ID.toText()]: ckETHBalanceUlps, + [CKUSDC_UNIVERSE_CANISTER_ID.toText()]: ckUSDCBalanceE8s, [CKETHSEPOLIA_UNIVERSE_CANISTER_ID.toText()]: ckETHBalanceUlps, [ledgerCanisterIdTetris.toText()]: tetrisBalanceE8s, [ledgerCanisterIdPacman.toText()]: pacmanBalanceE8s, @@ -163,6 +174,15 @@ describe("Tokens route", () => { setAccountsForTesting({ main: { ...mockMainAccount, balanceUlps: icpBalanceE8s }, }); + + icrcCanistersStore.setCanisters({ + ledgerCanisterId: CKUSDC_LEDGER_CANISTER_ID, + indexCanisterId: CKUSDC_INDEX_CANISTER_ID, + }); + tokensStore.setToken({ + canisterId: CKUSDC_UNIVERSE_CANISTER_ID, + token: mockCkUSDCToken, + }); }); describe("when logged in", () => { @@ -198,8 +218,9 @@ describe("Tokens route", () => { "Internet Computer", "ckBTC", "ckETH", - "Tetris", + "ckUSDC", "Pacman", + "Tetris", ]); }); @@ -211,8 +232,9 @@ describe("Tokens route", () => { { projectName: "Internet Computer", balance: "1.23 ICP" }, { projectName: "ckBTC", balance: "4.45 ckBTC" }, { projectName: "ckETH", balance: "4.14 ckETH" }, - { projectName: "Tetris", balance: "2.22 TST" }, + { projectName: "ckUSDC", balance: "111.00 ckUSDC" }, { projectName: "Pacman", balance: "3.14 PCMN" }, + { projectName: "Tetris", balance: "2.22 TST" }, ]); }); @@ -466,6 +488,84 @@ describe("Tokens route", () => { }); }); + describe("table sorting", () => { + describe("when logged in", () => { + beforeEach(() => { + resetIdentity(); + }); + + it("should display tokens in specific order", async () => { + const po = await renderPage(); + const tokensPagePo = po.getTokensPagePo(); + expect(await tokensPagePo.getTokenNames()).toEqual([ + "Internet Computer", + "ckBTC", + "ckETH", + "ckUSDC", + "Pacman", + "Tetris", + ]); + }); + + describe("taking balance into account", () => { + beforeEach(() => { + vi.spyOn(icrcLedgerApi, "queryIcrcBalance").mockImplementation( + async ({ canisterId }) => { + const balancesMap = { + [CKBTC_UNIVERSE_CANISTER_ID.toText()]: ckBTCBalanceE8s, + [CKTESTBTC_UNIVERSE_CANISTER_ID.toText()]: ckBTCBalanceE8s, + [CKETH_UNIVERSE_CANISTER_ID.toText()]: 0n, + [CKUSDC_UNIVERSE_CANISTER_ID.toText()]: ckUSDCBalanceE8s, + [CKETHSEPOLIA_UNIVERSE_CANISTER_ID.toText()]: + ckETHBalanceUlps, + [ledgerCanisterIdTetris.toText()]: tetrisBalanceE8s, + [ledgerCanisterIdPacman.toText()]: 0n, + }; + if (isNullish(balancesMap[canisterId.toText()])) { + throw new Error( + `Account not found for canister ${canisterId.toText()}` + ); + } + return balancesMap[canisterId.toText()]; + } + ); + }); + + it("should display token with balance first", async () => { + const po = await renderPage(); + const tokensPagePo = po.getTokensPagePo(); + expect(await tokensPagePo.getTokenNames()).toEqual([ + "Internet Computer", + "ckBTC", + "ckUSDC", + "Tetris", + "ckETH", + "Pacman", + ]); + }); + }); + + describe("when logged out", () => { + beforeEach(() => { + setNoIdentity(); + }); + + it("should display token in specific order", async () => { + const po = await renderPage(); + const tablePo = po.getSignInTokensPagePo().getTokensTablePo(); + expect(await tablePo.getTokenNames()).toEqual([ + "Internet Computer", + "ckBTC", + "ckETH", + "ckUSDC", + "Pacman", + "Tetris", + ]); + }); + }); + }); + }); + describe("when logged out", () => { beforeEach(() => { setNoIdentity(); @@ -499,8 +599,9 @@ describe("Tokens route", () => { "Internet Computer", "ckBTC", "ckETH", - "Tetris", + "ckUSDC", "Pacman", + "Tetris", ]); }); }); @@ -518,8 +619,9 @@ describe("Tokens route", () => { expect(await signInPo.getTokenNames()).toEqual([ "Internet Computer", "ckETH", - "Tetris", + "ckUSDC", "Pacman", + "Tetris", ]); }); });