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

feat(frontend): fallback for non-working index canisters #3761

Merged

Conversation

AntonioVentilii
Copy link
Collaborator

@AntonioVentilii AntonioVentilii commented Nov 26, 2024

Motivation

It may happen that an ICRC token has an Index Canister but it is not working. For example, it is out of cycles.

In those cases, we don't want to show an error anymore, but leverage the same wallet service used for tokens without Index Canister.

That means, that in those cases, we will just fetch the balance, and show an explanatory warning for not having the transacitons.

Changes

  • Change function onLoadTransactionsError to accept a fallback function.
  • If the error is raised during the ICRC transaction fetching, the fallback restarts the worker, but providing only the lLedger Canister, and not the Index Canister. This will allow the service to treat it in the same way of a token without index canister (i.e. fetching balance only).

Note

The changes in the UI will be done with:

Tests

I simulated a wrong index canister in the local replica for ckSepoliaUSDC. In the video we can see how I switch between the correct Index canister and the wrong one, back and forth.

Screen.Recording.2024-11-26.at.13.16.36.mov

@AntonioVentilii AntonioVentilii requested a review from a team as a code owner November 26, 2024 13:32

// In case of error, we start the listener again, but only with the ledgerCanisterId,
// to make it request only the balance and not the transactions
restartWorkerWithLedgerOnly();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterpeterparker as discussed; do you think it is too much to have an internal function? I would not externalize it, just because I need to pass all props (worker, ledger, env)

Copy link
Member

Choose a reason for hiding this comment

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

I think it's good 👍

Copy link
Member

@peterpeterparker peterpeterparker left a comment

Choose a reason for hiding this comment

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

LGTM, thx

PR discussed and reviewed offline as well, cool stuff!

}) => {
icTransactionsStore.reset(tokenId);

// We get transactions and balance for the same end point therefore if getting certified transactions fails, it also means the balance is incorrect.
balancesStore.reset(tokenId);

if (silent) {
// We print the error to console just for debugging purposes
console.error(`${get(i18n).transactions.error.loading_transactions}:`, err);
Copy link
Member

Choose a reason for hiding this comment

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

console.warn if it's just for debugging purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!


// In case of error, we start the listener again, but only with the ledgerCanisterId,
// to make it request only the balance and not the transactions
restartWorkerWithLedgerOnly();
Copy link
Member

Choose a reason for hiding this comment

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

I think it's good 👍

@AntonioVentilii AntonioVentilii enabled auto-merge (squash) November 26, 2024 14:57
@AntonioVentilii AntonioVentilii merged commit ff711c3 into main Nov 26, 2024
17 checks passed
@AntonioVentilii AntonioVentilii deleted the feat(frontend)/fallback-for-non-working-index-canisters branch November 26, 2024 15:00
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.

2 participants