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.

Sign up for GitHub

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

Jump to bottom

Price API perf improvements #3753

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Price API perf improvements #3753

merged 1 commit into from
Jan 9, 2024

Conversation

bergeron
Copy link
Contributor

@bergeron bergeron commented Jan 9, 2024

Explanation

Makes 2 perf improvements to price API calls:

  • Batch size of 30 instead of 100
  • Sort token addresses in query params for more cache hits

References

Changelog

@metamask/assets-controllers

  • CHANGED: Price API batch size reduced from 100 to 30

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

@bergeron bergeron requested a review from a team as a code owner January 9, 2024 18:33
bergeron
bergeron commented Jan 9, 2024
View reviewed changes
packages/assets-controllers/src/assetsUtil.ts
@@ -432,7 +432,7 @@ export async function fetchTokenContractExchangeRates({
Hex,
Awaited<ReturnType<AbstractTokenPricesService['fetchTokenPrices']>>
>({
values: tokenAddresses,
values: [...tokenAddresses].sort(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I allocated a new array because it's undesirable for a function to modify params passed by the caller. Caller may not expect it to mutate. But don't feel strongly if someone prefers in-place sort.

bergeron
bergeron commented Jan 9, 2024
View reviewed changes
packages/assets-controllers/src/assetsUtil.test.ts
Comment on lines -484 to -490
jest
.spyOn(mockPriceService, 'validateCurrencySupported')
.mockReturnValue(true);

jest
.spyOn(mockPriceService, 'validateChainIdSupported')
.mockReturnValue(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the mock price service already sets these to true so thought i'd simplify a bit

Gudahtt
Gudahtt approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@bergeron bergeron merged commit 78cbd61 into main Jan 9, 2024
134 checks passed
@bergeron bergeron deleted the brian/price-api-perf branch January 9, 2024 19:03
This was referenced Jan 10, 2024
Merged
Merged
bergeron added a commit to MetaMask/metamask-extension that referenced this pull request Jan 10, 2024
@bergeron
Bump assets controllers to v24 (#22478)
f34cb36 
## **Description**

Bumps assets controllers to v24. Brings in change to reduce batch size
of calls to price API (MetaMask/core#3753)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#1891

## **Manual testing steps**

- Ensure that token prices in fiat still appear on home page

## **Screenshots/Recordings**

No visible differences

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **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.
bergeron added a commit to MetaMask/metamask-mobile that referenced this pull request Jan 10, 2024
@bergeron
fix: Price API perf improvements (#8235)
3a3b066 
## **Description**

Patches batching and sorting changes to improve price API perf
(MetaMask/core#3753)

## **Related issues**

- MetaMask/MetaMask-planning#1891
- MetaMask/mobile-planning#1458

## **Manual testing steps**

Token fiat prices should still appear on home page

## **Screenshots/Recordings**

No visible changes

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.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-mobile/blob/main/.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.
danjm pushed a commit to MetaMask/metamask-extension that referenced this pull request Jan 10, 2024
@bergeron @danjm
Bump assets controllers to v24 (#22478)
b2f1270 
Bumps assets controllers to v24. Brings in change to reduce batch size
of calls to price API (MetaMask/core#3753)

Fixes: MetaMask/MetaMask-planning#1891

- Ensure that token prices in fiat still appear on home page

No visible differences

<!-- [screenshots/recordings] -->

<!-- [screenshots/recordings] -->

- [ ] 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".

- [ ] 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@danjm danjm danjm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bergeron @Gudahtt @danjm