-
-
Notifications
You must be signed in to change notification settings - Fork 252
feat: support adding non-evm tokens #7016
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
base: main
Are you sure you want to change the base?
Conversation
packages/assets-controllers/src/MultichainAssetsController/MultichainAssetsController.ts
Show resolved
Hide resolved
| * @returns The updated asset list for the account. | ||
| */ | ||
| async addAsset( | ||
| assetId: CaipAssetType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be an array of assetIds since we want to support selecting and adding multiple assets. I also think the function name should be addAssets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current implementation in mobile assets are added in a loop 1 by 1. I can change the implementation. Should I also rename the function to addAssets? Or should we have separate functions and if so in which case do we need addAsset (singular)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call.
If we can add multiple at the controller level, this should reduce state updates and computations. (Update all at once instead of one at a time).
In favour of renaming this if not yet used on clients.
| // Triggered when an asset is added to the accountAssets list manually | ||
| for (const assetId of assetsWithoutBalance) { | ||
| state.balances[accountId][assetId] = { amount: '0', unit: '' }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Bug
The balance update logic has two main issues: it prevents existing asset balances from refreshing with new snap data, potentially leading to stale values or incorrect zeroing of assets. Also, new accounts don't initialize zero balances for assets not returned by the snap, causing inconsistencies.
Explanation
The feature to hide tokens is now available for NON-EVM assets but there is no way to bring the assets back. This task involves implementing a token import feature for non-EVM assets. This is crucial for improving user experience by allowing users to import. The implementation should be prioritized to align with upcoming Solana campaigns.
References
https://consensyssoftware.atlassian.net/browse/ASSETS-1425
Checklist
Note
Adds
MultichainAssetsController:addAssetto manually add non‑EVM assets (with metadata refresh, dedupe, unignore, and event emission) and updatesMultichainBalancesControllerto set zero balance for newly added assets missing snap balances.addAssettoMultichainAssetsController(registers action handler, updatesaccountsAssets, refreshes metadata, removes fromallIgnoredAssets, dedupes, and publishesMultichainAssetsController:accountAssetListUpdated).MultichainAssetsControllerActionswithMultichainAssetsControllerAddAssetAction.src/MultichainAssetsController/MultichainAssetsController.test.ts).MultichainAssetsController:accountAssetListUpdated, populate balances for new assets and set balance to{ amount: '0', unit: '' }when snap returns no balance (uses accounts map to track added assets).src/MultichainBalancesController/MultichainBalancesController.test.ts).addAssetaddition inpackages/assets-controllers/CHANGELOG.md.Written by Cursor Bugbot for commit e4a6194. This will update automatically on new commits. Configure here.