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.

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

bump @metamask/assets-controllers to ^18.0.0 #21549

Merged
merged 48 commits into from
Nov 14, 2023

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Oct 25, 2023

Description

Bumps @metamask/assets-controllers to v18.0.0 which includes breaking changes for the CurrencyRateController. Notably the structure of the state has changed significantly. Part of making the state change so drastic (as opposed to making the refactor towards Multichain be as small/incremental as possible), is that the usage of pendingNativeCurrency, pendingCurrentCurrency, and how the controller handles state change in general causes unexpected behavior with concurrent updates. The update to CurrencyRateController also solves this issue.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1025
Related: MetaMask/core#1687

Manual testing steps

Nothing should change for the end user.

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • 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 format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines).
  • 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.

@github-actions
Copy link
Contributor

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.

Copy link

socket-security bot commented Nov 2, 2023

Updated and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@metamask/assets-controllers 16.0.0...18.0.0 None +0/-2 678 kB metamaskbot

🚮 Removed packages: @metamask/[email protected]

Copy link

socket-security bot commented Nov 3, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

jiexi added a commit that referenced this pull request Nov 6, 2023
## **Description**

* Bump `@metamask/network-controller` to `15.1.0`
* Replace `eth-query` with `@metamask/eth-query`
* Bump `@metamask/eth-query` to `3.0.1` 

Needed for bumping `@metamask/assets-controller` to 18.0.0 from 16

## **Related issues**

See: #21549

## **Manual testing steps**

Nothing should change for the end user

## **Screenshots/Recordings**

<!-- 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**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] 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
- [x] 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.

---------

Co-authored-by: MetaMask Bot <[email protected]>
@jiexi
Copy link
Contributor Author

jiexi commented Nov 6, 2023

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (f307eb8) 68.40% compared to head (32ac98b) 68.44%.
Report is 2 commits behind head on develop.

Files Patch % Lines
app/scripts/metamask-controller.js 72.73% 3 Missing ⚠️
ui/pages/asset/asset.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #21549      +/-   ##
===========================================
+ Coverage    68.40%   68.44%   +0.04%     
===========================================
  Files         1046     1046              
  Lines        41689    41647      -42     
  Branches     11112    11101      -11     
===========================================
- Hits         28514    28503      -11     
+ Misses       13175    13144      -31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [86bba7b]
Page Load Metrics (562 ± 279 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint891219984
domContentLoaded7011287126
load821428562581279
domInteractive7011287126
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -4.33 KiB (-0.11%)
  • ui: 307 Bytes (0.00%)
  • common: 1.12 KiB (0.02%)

Comment on lines -1379 to +1390
await this.currencyRateController.setNativeCurrency(ticker);
if (
this.preferencesController.store.getState().useCurrencyRateCheck
) {
await this.currencyRateController.stopAllPolling();
this.currencyRateController.startPollingByNetworkClientId(
this.networkController.state.selectedNetworkClientId,
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this changing the behavior here? Previously calling setNativeCurrency didn't start polling it just updated the exchangerate once?

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why we wouldn't want to poll here but I'm not seeing that we did before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not as we guard starting the poll with a call to this.preferencesController.store.getState().useCurrencyRateCheck

Copy link
Contributor

@adonesky1 adonesky1 Nov 13, 2023

Choose a reason for hiding this comment

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

right but previously we didn't start polling regardless of whether or not the useCurrencyRateCheck was set to true? We just checked once? Again I feel like we would want to stop/start polling here but it does seems like the behavior would change given your changes? But might be missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, previously we did not start/stop polling for the CurrencyRateController on networkDidChange from MetaMaskController. That's because previously, it was possible to tell the CurrencyRateController that you wanted to "queue up" an update for the exchange rates for XYZ native currency and let the controller poll for it on its own time whenever that next poll iteration would happen. Now it's not possible to tell the CurrencyRateController to "queue up" a nativeCurrency. It's either polling or it isn't. Because of that, we need to keep track of useCurrencyRateCheck outside of the CurrencyRateController and handling the starting/stopping of polling ourselves

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not have the CurrencyRateController's messenger subscribe to NetworkController changes and do this start/stop polling in the controller itself as we do with most other controllers?

I suppose it could. That would require adding a guard to check useCurrencyRateCheck on each polling loop as a way to pause polling as opposed to just stopping the polling itself directly. That would mean if useCurrencyRateCheck is set to false, and i call currencyRateController.startPollingByNetworkClientId('123'), then the polling would be active, but will always exit early until useCurrencyRateCheck is set to true.

Does this seem to be more inline with how you were thinking we should approach this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we not check useCurrencyRateCheck in the frontend an only call startPollingByNetworkClientId if its true?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry that didn't make sense in this context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well couldn't we check useCurrencyRateCheck in the handler for networkStateChange on the CurrencyRateController and only start polling if its true?

Copy link
Contributor

Choose a reason for hiding this comment

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

I won't block on this. We can leave it as is for now.

@@ -140,8 +141,8 @@ export const use4ByteResolutionSelector = (state) =>
state.metamask.use4ByteResolution;
export const currentCurrencySelector = (state) =>
state.metamask.currentCurrency;
export const conversionRateSelector = (state) => state.metamask.conversionRate;

export const conversionRateSelector = (state) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this a duplicate of the getConversionRate selector in ducks/metamask/metamask.js. I guess there are a bunch of duplicates in here?

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

Just a few small questions! Otherwise LGTM! Also looks like it needs a rebase

@jiexi
Copy link
Contributor Author

jiexi commented Nov 13, 2023

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@metamaskbot
Copy link
Collaborator

Builds ready [d550ce2]
Page Load Metrics (733 ± 286 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint88131104147
domContentLoaded7012894189
load821468733596286
domInteractive7012894189
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -4.33 KiB (-0.11%)
  • ui: 307 Bytes (0.00%)
  • common: 1.12 KiB (0.02%)

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [7181ef8]
Page Load Metrics (561 ± 275 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint87132107157
domContentLoaded7512494147
load831489561573275
domInteractive7512494147
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -4.33 KiB (-0.11%)
  • ui: 307 Bytes (0.00%)
  • common: 1.12 KiB (0.02%)

@metamaskbot
Copy link
Collaborator

Builds ready [32ac98b]
Page Load Metrics (641 ± 296 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint91130105105
domContentLoaded7212194146
load841658641617296
domInteractive7212194146
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -4.33 KiB (-0.11%)
  • ui: 307 Bytes (0.00%)
  • common: 1.12 KiB (0.02%)

@jiexi jiexi merged commit 82ea5a0 into develop Nov 14, 2023
65 checks passed
@jiexi jiexi deleted the jl/mmp-1025/multichain-v1-currency-rate-controller branch November 14, 2023 17:18
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2023
@metamaskbot metamaskbot added the release-11.7.0 Issue or pull request that will be included in release 11.7.0 label Nov 14, 2023
jiexi added a commit that referenced this pull request Dec 18, 2023
## **Description**

Cryptocompare reported a spike in API requests. This is the result of
incorrectly starting CurrencyRateController polling on MetaMask
extension background startup. This PR fixes the issue by removing the
logic that was starting CurrencyRateController polling in the
constructor.

## **Related issues**

Fixes: #21549

## **Manual testing steps**

1. Open network debug on background.html
2. Restart extension
3. No cryptocompare requests should be seen
4. Open wallet UI
5. One cryptocompare request should be seen
6. Wait 3mins
7. Another cryptocompare request should be seen
8. Close UI
9. Wait 3mins
10. No additional cryptocompare requests should be seen 

## **Screenshots/Recordings**

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

### **Before**

Note the requests made before the UI is open, but that they are stopped
after UI is opened then closed for the first time.


https://github.com/MetaMask/metamask-extension/assets/918701/c66eabd0-363b-467a-87e3-5628daa95395

### **After** 


https://github.com/MetaMask/metamask-extension/assets/918701/3676fa9d-4b6f-4c4d-941d-eac7b0ab7194

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [x] 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.
jiexi added a commit that referenced this pull request Dec 18, 2023
## **Description**

Cryptocompare reported a spike in API requests. This is the result of
incorrectly starting CurrencyRateController polling on MetaMask
extension background startup. This PR fixes the issue by removing the
logic that was starting CurrencyRateController polling in the
constructor.

## **Related issues**

Fixes: #21549

## **Manual testing steps**

1. Open network debug on background.html
2. Restart extension
3. No cryptocompare requests should be seen
4. Open wallet UI
5. One cryptocompare request should be seen
6. Wait 3mins
7. Another cryptocompare request should be seen
8. Close UI
9. Wait 3mins
10. No additional cryptocompare requests should be seen 

## **Screenshots/Recordings**

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

### **Before**

Note the requests made before the UI is open, but that they are stopped
after UI is opened then closed for the first time.


https://github.com/MetaMask/metamask-extension/assets/918701/c66eabd0-363b-467a-87e3-5628daa95395

### **After** 


https://github.com/MetaMask/metamask-extension/assets/918701/3676fa9d-4b6f-4c4d-941d-eac7b0ab7194

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [x] 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.
jiexi added a commit that referenced this pull request Dec 18, 2023
## **Description**

Cryptocompare reported a spike in API requests. This is the result of
incorrectly starting CurrencyRateController polling on MetaMask
extension background startup. This PR fixes the issue by removing the
logic that was starting CurrencyRateController polling in the
constructor.

## **Related issues**

Fixes: #21549

## **Manual testing steps**

1. Open network debug on background.html
2. Restart extension
3. No cryptocompare requests should be seen
4. Open wallet UI
5. One cryptocompare request should be seen
6. Wait 3mins
7. Another cryptocompare request should be seen
8. Close UI
9. Wait 3mins
10. No additional cryptocompare requests should be seen 

## **Screenshots/Recordings**

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

### **Before**

Note the requests made before the UI is open, but that they are stopped
after UI is opened then closed for the first time.


https://github.com/MetaMask/metamask-extension/assets/918701/c66eabd0-363b-467a-87e3-5628daa95395

### **After** 


https://github.com/MetaMask/metamask-extension/assets/918701/3676fa9d-4b6f-4c4d-941d-eac7b0ab7194

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [x] 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 that referenced this pull request Dec 18, 2023
…#22326) (#22328)

## **Description**

Cryptocompare reported a spike in API requests. This is the result of
incorrectly starting CurrencyRateController polling on MetaMask
extension background startup. This PR fixes the issue by removing the
logic that was starting CurrencyRateController polling in the
constructor.

## **Related issues**

Fixes: #21549

## **Manual testing steps**

1. Open network debug on background.html
2. Restart extension
3. No cryptocompare requests should be seen
4. Open wallet UI
5. One cryptocompare request should be seen
6. Wait 3mins
7. Another cryptocompare request should be seen
8. Close UI
9. Wait 3mins
10. No additional cryptocompare requests should be seen 

## **Screenshots/Recordings**

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

### **Before**

Note the requests made before the UI is open, but that they are stopped
after UI is opened then closed for the first time.



https://github.com/MetaMask/metamask-extension/assets/918701/c66eabd0-363b-467a-87e3-5628daa95395

### **After** 



https://github.com/MetaMask/metamask-extension/assets/918701/3676fa9d-4b6f-4c4d-941d-eac7b0ab7194

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [x] 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.

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- 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-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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.7.0 Issue or pull request that will be included in release 11.7.0 team-wallet-api-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants