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.

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

NNS1-2849: Add derived token stores #4461

Merged
merged 7 commits into from
Feb 12, 2024
Merged

NNS1-2849: Add derived token stores #4461

merged 7 commits into from
Feb 12, 2024

Conversation

dskloetd
Copy link
Contributor

@dskloetd dskloetd commented Feb 12, 2024

Motivation

tokensStore used to be keyed by universe ID but in order to reuse the IcrcWalletPage for the SnsWallet, it needed to be keyed by ledger canister ID. But there are other context where it is still more convenient to access the token by universe ID or root canister ID. It is currently temporarily keyed by both, which means it has the same tokens multiple times. But converting root canister ID to ledger canister ID every time would be tedious.
Additionally, the SNS tokens data already exists in the aggregator data, which means we now have several sources of truth for the tokens data.

The clean solution is to have 1 source of truth, and derived stores for everything else.

So with this PR:

  • tokensStore only holds non-SNS tokens
  • snsAggregatorStore becomes the only source of truth for SNS tokens
  • several derived tokens stores are added to be used to actually get a token by universe ID, root canister ID or ledger canister ID.

Changes

  1. Stop adding SNS tokens to tokensStore in both lib/services/$public/sns.services.ts (for production) and tests/utils/sns.test-utils.ts for testing.
  2. Add derived stores snsTokensByRootCanisterIdStore, snsTokensByLedgerCanisterIdStore, tokensByUniverseIdStore and tokensByLedgerCanisterIdStore.
  3. Use the derived stores as appropriate in places where the tokensStore was used directly before.
  4. In loadIcrcToken and loadToken, don't load the token if it's a known SNS token as it should already be available in the appropriate derived stores.
  5. Support setting a fee in the aggregator test token data.
  6. Make the lifecycle optional in the aggregator test data.

Tests

  1. Unit test for the added derived stores.
  2. Unit test that loadIcrcToken doesn't load SNS tokens.
  3. Existing unit tests updated to rely on token data from the aggregator data.

Todos

  • Add entry to changelog (if necessary).

@dskloetd dskloetd marked this pull request as ready for review February 12, 2024 13:39
Copy link
Contributor

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

Thanks!

Just a few comments, but it looks good!

Copy link
Contributor

@mstrasinskis mstrasinskis left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@dskloetd dskloetd changed the title Add derived token stores NNS1-2849: Add derived token stores Feb 12, 2024
@dskloetd dskloetd added this pull request to the merge queue Feb 12, 2024
Merged via the queue into main with commit 07a29a1 Feb 12, 2024
48 checks passed
@dskloetd dskloetd deleted the kloet/sns-token-store branch February 12, 2024 15:42
github-merge-queue bot pushed a commit that referenced this pull request Dec 9, 2024
# Motivation

This function `fillTokensStoreFromAggregatorData` was initially added in
#4455 and later removed in #4461. It appears to have been a temporary
function that is no longer necessary.

# Changes

- Removes `fillTokensStoreFromAggregatorData`

# Tests

- Deletes tests

# Todos

- [ ] Add entry to changelog (if necessary).
Not necessary
yhabib added a commit that referenced this pull request Dec 9, 2024
# Motivation

This function `fillTokensStoreFromAggregatorData` was initially added in
#4455 and later removed in #4461. It appears to have been a temporary
function that is no longer necessary.

# Changes

- Removes `fillTokensStoreFromAggregatorData`

# Tests

- Deletes tests

# Todos

- [ ] Add entry to changelog (if necessary).
Not necessary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants