Populate icrc store with imported tokens #5378
Conversation
mstrasinskis
changed the title
| }), | ||
| }); | ||
|
|
||
| if (!certified && notForceCallStrategy()) { |
There was a problem hiding this comment.
Why is importedTokensStore populated regardless of certified but icrcCanistersStore only if certified?
There was a problem hiding this comment.
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.
| 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({ |
There was a problem hiding this comment.
We shouldn't populated 2 different stores with the same data. Instead we should have a derived store which combines the relevant stores.
There was a problem hiding this comment.
Like AllIcrcCanistersStore ?
There was a problem hiding this comment.
Yes, or we rename icrcCanisterStore to something else like, defaultIcrcCanistersStore or something.
| @@ -7,7 +7,7 @@ import { writable } from "svelte/store"; | |||
|
|
|||
| export interface IcrcCanisters { | |||
| ledgerCanisterId: Principal; | |||
| indexCanisterId: Principal; | |||
| indexCanisterId: Principal | undefined; | |||
There was a problem hiding this comment.
This means the work-around can now be removed, right?
There was a problem hiding this comment.
Only after the feature in not behind the flag.
There was a problem hiding this comment.
How come? With this change, the type of indexCanisterId is Principal | undefined regardless of whether the feature flag is enabled.
# Motivation We need to make the NNS-dapp treat imported tokens as ICRC tokens, just as it does with the current tokens, since imported tokens follow the ICRC standard. The only differences are that the index canister is optional (and the nns-dapp already handles this), and they are added by the user. Note: This PR includes a solution suggested in [another approach PR](#5378 (comment)). # Changes 1. Rename icrcCanistersStore to defaultIcrcCanistersStore. 2. Introduce a new derived store that contains both the current ICRC canister IDs and imported token IDs, and give this store the old name icrcCanistersStore. 3. Adjust the NNS-dapp code so that it reads the canister IDs from the derived store and writes to the renamed defaultIcrcCanistersStore. # Tests - Adjust tests to reflect the changes. - Test for icrcCanistersStore. - Tested manually that there are not current functionality changes. # Todos - [ ] Add entry to changelog (if necessary). Not necessary.
Motivation
The imported tokens are ICRC tokens. The only new difference is that the index canister is optional. To make the NNS-dapp work with them, the only change needed is to modify the ICRC store to support entries without the index canister (here).
Once the ICRC store supports tokens without an index canister, we can add imported tokens to the ICRC store, and they will be displayed in the tokens table and on the wallet page.
Changes
IcrcCanistersStore.IcrcCanistersStoreafter loading them.Tests
The existence of the work-around suggests that not specifying an index canister already works.
The index canister is optional in IcrcWallet.
The transaction list is now shown when no index canister.
{#if nonNullish($selectedAccountStore.account) && nonNullish($selectedIcrcTokenUniverseIdStore) && nonNullish(indexCanisterId)} <IcrcWalletTransactionsListIndex canister is not used in GetTokensModal.
The index canister is not used in the logic of derived stores that uses icrcStore:
icrc-universes.derivedselectable-universe.derivedselected-universe.derived storeTodos
Not necessary.