feat: export exchange rates fct #3657

sahar-fehri · 2023-12-13T11:15:25Z

Explanation

Export a function from assets controller to fetch token exchange rates.

This function will be used by extension to compute user token fiat amount and display it on the import token modal confirmation page.

References

Fixes #12345
Related to #67890

Changelog

`@metamask/assets-controllers`

: Updated assetsUtil file to export fetchAndMapExchangeRates function

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've highlighted breaking changes using the "BREAKING" category above as appropriate

mcmire

Thanks for taking care of this! I had a few comments for improvements we can make.

packages/assets-controllers/src/assetsUtil.ts

packages/assets-controllers/src/assetsUtil.test.ts

packages/assets-controllers/src/assetsUtil.ts

packages/assets-controllers/src/token-prices-service/codefi-v2.ts

mcmire

A couple of comments, one about the change in codefi-v2.ts, and another for TokenRatesController, but this looks good otherwise!

packages/assets-controllers/src/token-prices-service/codefi-v2.ts

packages/assets-controllers/src/TokenRatesController.ts

mcmire

This makes sense to me, thanks! I'll approve it now though I'll keep tabs on it in case #3687 forces more changes.

packages/assets-controllers/src/assetsUtil.test.ts

MajorLift · 2023-12-20T21:23:55Z

packages/assets-controllers/src/assetsUtil.ts

+ * The maximum number of token addresses that should be sent to the Price API in
+ * a single request.
+ */
+export const TOKEN_PRICES_BATCH_SIZE = 100;


Should this be a package-level export as well? I assume it's only used inside of assets-controllers, but just wanted to check.

I had requested that it be moved from TokenRatesController so it wasn't a package-level export, as I didn't think anyone needed to refer to this. But I guess I made an assumption, so that's a fair question — do we anticipate this being used in client code?

For now i do not expect extension to be using this.
You guys think its fine to keep it in assetsUtil.ts in that case?

I don't anticipate that we'd need this anywhere else either.

It seems more appropriate for this to be part of the service itself, rather than here or in the controller. This choice in batch size is a recommendation specific to the CodeFi V2 price API in particular. It would be ideal if we could attach this batch size to the service instead.

This constant exists here because the batching code is outside of the service at the moment. Perhaps we can integrate that better in another PR?

MajorLift

Looks good!!

Gudahtt · 2023-12-21T16:02:46Z

packages/assets-controllers/src/assetsUtil.ts

+ * @param args.chainId - The chainId of the tokens.
+ * @returns The prices for the requested tokens.
+ */
+export async function fetchTokenContractExchangeRates({


Have you considered incorporating this into the token price service itself? That would let us remove the duplication between here and the controller.

Sorry — this was my recommendation to bring over the batching code from the controller. My thought was that we could refactor the controller later to use this function, but at least at the moment, if the batching were in this function too, the behavior would be the same regardless of whether you used this function or the controller. Wondering if we could take care of moving the batching in another PR? That should be a non-breaking change.

Well, it might result in breaking changes, depending on how we merge the two. If we move it into the service, the other methods on the serve might no longer be needed, letting us remove those from the API. Though of course we could do that in two steps.

And overall yes I agree, we can merge this and refactor it later.

Just created an issue to track this suggestion: #4117

packages/assets-controllers/src/assetsUtil.test.ts

Gudahtt

LGTM!

I left some suggestions, one of which might require substantial changes but would leave this in a state that is easier to maintain (moving this into the token service). But I don't object to merging this as-is if you'd prefer.

## **Description** The goal of this PR is to fetch user token fiat balance on the import modal confirmation page. There is an ongoing PR on core to enable this: MetaMask/core#3657 ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/MMASSETS-88 also design wise relates to this PR #21704 ## **Manual testing steps** 1. Go to home page 2. Click on import tokens button 3. Select a token 4. Click import 5. Notice the token balance in fiat on the confirmation page ## **Screenshots/Recordings**  ### **Before** ![image](https://github.com/MetaMask/metamask-extension/assets/10994169/a7513ded-2960-4c6d-972d-8bb1d673f0df) ### **After** ![image](https://github.com/MetaMask/metamask-extension/assets/10994169/1232ba09-9769-476d-b4f8-12415ac55f2c) ## **Pre-merge author checklist** - [] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've clearly explained what problem this PR is solving and how it is solved. - [ ] I've linked related issues - [ ] I've included manual testing steps - [ ] I've included screenshots/recordings if applicable - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

sahar-fehri requested a review from a team as a code owner December 13, 2023 11:15

sahar-fehri changed the title ~~fix: export exchange rates fct~~ feat: export exchange rates fct Dec 13, 2023

sahar-fehri force-pushed the feat/MMASSSETS-88-Add-TokenFiatBalance-before-import-v2 branch from aec12f9 to 584bcef Compare December 13, 2023 11:26

sahar-fehri mentioned this pull request Dec 13, 2023

feat: fetch token fiat amount on import confirmation modal MetaMask/metamask-extension#22263

Merged

12 tasks

sahar-fehri self-assigned this Dec 13, 2023

mcmire reviewed Dec 14, 2023

View reviewed changes

sahar-fehri force-pushed the feat/MMASSSETS-88-Add-TokenFiatBalance-before-import-v2 branch from 584bcef to b40d809 Compare December 18, 2023 13:51

sahar-fehri requested a review from mcmire December 18, 2023 15:15

mcmire reviewed Dec 18, 2023

View reviewed changes

packages/assets-controllers/src/assetsUtil.test.ts Outdated Show resolved Hide resolved

packages/assets-controllers/src/assetsUtil.ts Outdated Show resolved Hide resolved

mcmire mentioned this pull request Dec 18, 2023

feat: Add onBreak handler to token price service #3677

Merged

3 tasks

sahar-fehri commented Dec 18, 2023

View reviewed changes

packages/assets-controllers/src/token-prices-service/codefi-v2.ts Outdated Show resolved Hide resolved

sahar-fehri requested a review from mcmire December 19, 2023 13:03

mcmire reviewed Dec 19, 2023

View reviewed changes

packages/assets-controllers/src/token-prices-service/codefi-v2.ts Outdated Show resolved Hide resolved

packages/assets-controllers/src/TokenRatesController.ts Outdated Show resolved Hide resolved

sahar-fehri force-pushed the feat/MMASSSETS-88-Add-TokenFiatBalance-before-import-v2 branch from c6b1de6 to eaa67e6 Compare December 19, 2023 23:46

mcmire previously approved these changes Dec 20, 2023

View reviewed changes

MajorLift reviewed Dec 20, 2023

View reviewed changes

packages/assets-controllers/src/assetsUtil.test.ts Outdated Show resolved Hide resolved

MajorLift reviewed Dec 20, 2023

View reviewed changes

MajorLift previously approved these changes Dec 20, 2023

View reviewed changes

sahar-fehri dismissed stale reviews from MajorLift and mcmire via 258edef December 21, 2023 11:53

sahar-fehri force-pushed the feat/MMASSSETS-88-Add-TokenFiatBalance-before-import-v2 branch 2 times, most recently from aba4038 to cca73cc Compare December 21, 2023 11:58

Gudahtt reviewed Dec 21, 2023

View reviewed changes

packages/assets-controllers/src/assetsUtil.test.ts Outdated Show resolved Hide resolved

Gudahtt previously approved these changes Dec 21, 2023

View reviewed changes

sahar-fehri force-pushed the feat/MMASSSETS-88-Add-TokenFiatBalance-before-import-v2 branch from cca73cc to 6634b1c Compare December 21, 2023 17:58

fix: export exchange rates fct

6c474eb

sahar-fehri force-pushed the feat/MMASSSETS-88-Add-TokenFiatBalance-before-import-v2 branch from 6634b1c to 6c474eb Compare December 21, 2023 18:03

fix: fix unit test

6b93ae8

sahar-fehri dismissed Gudahtt’s stale review via 6b93ae8 December 21, 2023 18:20

Gudahtt approved these changes Dec 21, 2023

View reviewed changes

sahar-fehri merged commit cd78c1f into main Dec 21, 2023
136 checks passed

sahar-fehri deleted the feat/MMASSSETS-88-Add-TokenFiatBalance-before-import-v2 branch December 21, 2023 18:29

Gudahtt mentioned this pull request Mar 27, 2024

[assets-controllers] Migrate Token price batch update to token prices service #4117

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: export exchange rates fct #3657

feat: export exchange rates fct #3657

sahar-fehri commented Dec 13, 2023 •

edited

Loading

mcmire left a comment

mcmire left a comment •

edited

Loading

mcmire left a comment

MajorLift Dec 20, 2023

mcmire Dec 21, 2023

sahar-fehri Dec 21, 2023

Gudahtt Dec 21, 2023

mcmire Dec 21, 2023

MajorLift left a comment

Gudahtt Dec 21, 2023

mcmire Dec 21, 2023

Gudahtt Dec 21, 2023

Gudahtt Mar 27, 2024

Gudahtt left a comment

feat: export exchange rates fct #3657

feat: export exchange rates fct #3657

Conversation

sahar-fehri commented Dec 13, 2023 • edited Loading

Explanation

References

Changelog

@metamask/assets-controllers

Checklist

mcmire left a comment

Choose a reason for hiding this comment

mcmire left a comment • edited Loading

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

sahar-fehri commented Dec 13, 2023 •

edited

Loading

`@metamask/assets-controllers`

mcmire left a comment •

edited

Loading