-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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: integrate multichain assets rates controller to extension UI #30291
feat: integrate multichain assets rates controller to extension UI #30291
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
- Add `MultichainAssetsController` to the controller init list - Fix issues with `MultichainBalanceController` - Fix issues with `MultichainTransactionsController` - Revert type changes
6cd7757
to
48aa482
Compare
...cripts/controller-init/messengers/multichain/multichain-assets-rates-controller-messenger.ts
Outdated
Show resolved
Hide resolved
...cripts/controller-init/messengers/multichain/multichain-assets-rates-controller-messenger.ts
Outdated
Show resolved
Hide resolved
app/scripts/controller-init/multichain/multichain-rates-assets-controller-init.ts
Outdated
Show resolved
Hide resolved
Builds ready [04ae981]
Page Load Metrics (1883 ± 64 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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.
You need to add the controller name here: https://github.com/MetaMask/metamask-extension/blob/main/app/scripts/controller-init/utils.ts#L39
You might want to wait for https://github.com/MetaMask/metamask-extension/pull/30301to land on main as it will create a merge conflict
app/scripts/controller-init/multichain/multichain-rates-assets-controller-init.test.ts
Outdated
Show resolved
Hide resolved
Can re-approve once conflicts are resolved after #30301 |
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.
Still missing the controller's name here: https://github.com/MetaMask/metamask-extension/blob/main/app/scripts/controller-init/utils.ts#L39
resolved |
Builds ready [ae46e89]
Page Load Metrics (1513 ± 43 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
* @param state - Redux state object. | ||
* @returns An object containing non-EVM assets per accounts. | ||
*/ | ||
export function getAssetsRates(state: AssetsRatesState) { |
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.
Quick question, will this selector replace the existing getMultichainCoinRates
Asking because it's being used in the getMultichainConversionRate
cc @salimtb @ccharly
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.
@zone-live The one you mentioned is limited to Native tokens, whereas the one in this PR supports both SPL tokens and Native tokens.
So, the answer is yes—if we need comprehensive data, including SPL tokens, we should use the one in this PR.
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.
Thank you @salimtb 💪🏼
Description
This PR aims to integrate the new multichainAssetsRatesControllers into the extension. To achieve this, an upgrade of the assets controller to version 49 is required. As a result, the target branch is set accordingly:
MetaMask PR #30250.
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist