-
-
Notifications
You must be signed in to change notification settings - Fork 183
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: add new rates controller for non-EVM chains #4242
Conversation
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
@metamaskbot publish-preview |
packages/assets-controllers/src/crypto-compare-service/crypto-compare.ts
Outdated
Show resolved
Hide resolved
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.
Hello! I have done a single pass on this and made some suggestions on conventions. I will do another pass later thinking about tightening up the controller API and also diving more closely into JSDocs.
packages/assets-controllers/src/RatesController/RatesController.ts
Outdated
Show resolved
Hide resolved
packages/assets-controllers/src/RatesController/RatesController.ts
Outdated
Show resolved
Hide resolved
packages/assets-controllers/src/RatesController/RatesController.ts
Outdated
Show resolved
Hide resolved
packages/assets-controllers/src/RatesController/RatesController.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
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.
LGTM! (thanks for all those small changes)
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.
Just a few more suggestions, mostly more naming-related things. All comment are minor at this point, everything is looking a lot better now.
packages/assets-controllers/src/RatesController/RatesController.ts
Outdated
Show resolved
Hide resolved
packages/assets-controllers/src/crypto-compare-service/crypto-compare.ts
Outdated
Show resolved
Hide resolved
…into feat/nonevm-rates
packages/assets-controllers/src/crypto-compare-service/crypto-compare.ts
Show resolved
Hide resolved
Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
…into feat/nonevm-rates
packages/assets-controllers/src/RatesController/RatesController.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
…#4572) ## Explanation We missed to rename that field in that PR: #4242 ## References N/A ## Changelog ### `@metamask/assets-controllers` - FIXED: Fix `RatesController.setCryptocurrencyList` - It was not using the correct field when updating the internal state (`fromCurrencies` -> `cryptocurrencies`) ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate
Explanation
As part of the non-EVM initiative, there's a need to develop a new controller capable of fetching the rates for these blockchains. Currently, we can't rely on the
CurrencyRatesController
since it's dependent on the activity of theNetworkController
, which is exclusively EVM oriented.References
Fixes https://github.com/MetaMask/accounts-planning/issues/436
Changelog
@metamask/assets-controllers
RatesController
to manage data related to rates for non-EVM blockchainsChecklist