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

fix: bump assets controller to 30.0.0 #24913

Merged
merged 42 commits into from
Jun 5, 2024
Merged

Conversation

salimtb
Copy link
Contributor

@salimtb salimtb commented May 30, 2024

Description

Upgrades to version 30 of assets controllers, bringing a new marketData object to the token rates controller.

Open in GitHub Codespaces

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • 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). Not required for external contributors.

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.

@salimtb salimtb requested a review from a team as a code owner May 30, 2024 17:55
@salimtb salimtb added the INVALID-PR-TEMPLATE PR's body doesn't match template label May 30, 2024
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.

@salimtb salimtb marked this pull request as draft May 30, 2024 17:55
@metamaskbot metamaskbot removed the INVALID-PR-TEMPLATE PR's body doesn't match template label May 30, 2024
@salimtb salimtb added the INVALID-PR-TEMPLATE PR's body doesn't match template label May 30, 2024
@bergeron
Copy link
Contributor

I'm looking through remaining occurrences of contractExchangeRates and contractExchangeRatesByChainId in state / mocked state

yarn.lock Outdated Show resolved Hide resolved
@salimtb salimtb force-pushed the bump-assets-controller-version branch from 6dd2c8d to c4ec3b2 Compare May 31, 2024 14:56
@salimtb salimtb added team-assets needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. labels May 31, 2024
@bergeron
Copy link
Contributor

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@sahar-fehri
Copy link
Contributor

sahar-fehri commented Jun 4, 2024

NFT detection flow works as expected.
Import/hide NFT working as expected.
When trying to send an NFT i have this error in the console and the transaction fails
SES_UNHANDLED_REJECTION: EthereumRpcError: header not found
Are you guys able to reproduce this too?
I suspect same error happens when you try to send an ERC20.
(video)

Screen.Recording.2024-06-04.at.15.08.44.mov

@sahar-fehri
Copy link
Contributor

sahar-fehri commented Jun 4, 2024

I confirm that i have seen same behavior happen when trying to send an ERC2O.
Also, noticed some glitches in the send flow, where sometimes when i try to send DAI, i see error
Error: Unable to determine contract standard
(video)

Screen.Recording.2024-06-04.at.15.16.05.mov

@metamaskbot
Copy link
Collaborator

Builds ready [87dd240]
Page Load Metrics (51 ± 4 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6812082136
domContentLoaded8201031
load42755194
domInteractive8201031
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.01 KiB (0.03%)
  • ui: 72 Bytes (0.00%)
  • common: 22.93 KiB (0.37%)

@metamaskbot
Copy link
Collaborator

Builds ready [9fb556f]
Page Load Metrics (141 ± 173 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7213495178
domContentLoaded8161221
load401710141360173
domInteractive8161221
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.01 KiB (0.03%)
  • ui: 72 Bytes (0.00%)
  • common: 22.93 KiB (0.37%)

bergeron
bergeron previously approved these changes Jun 4, 2024
"oidc.api.cx.metamask.io"
"oidc.api.cx.metamask.io",
"price.api.cx.metamask.io",
"token.api.cx.metamask.io"
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the inclusion of "token.api.cx.metamask.io" should "token-api.metaswap.codefi.network", be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to remove it, but there was still a usage somewhere. I will tackle it separately from this upgrade.

Copy link
Contributor

@legobeat legobeat Jun 4, 2024

Choose a reason for hiding this comment

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

Looks like it's becuase this comes from @metamask/controller-utils, and since this PR introduces the new version but is still using the older one(s) simultaneously, it will hit a different endpoint depending on which of the versions is used.

https://github.com/MetaMask/metamask-extension/pull/24913/files#diff-51e4f558fae534656963876761c95b83b6ef5da5103c4adef6768219ed76c2deL4770

Copy link
Contributor

@cryptodev-2s cryptodev-2s Jun 5, 2024

Choose a reason for hiding this comment

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

@danjm @legobeat @bergeron there's this PR which is currently blocked by this one that removes old metaswap, metafi urls -> we can keep both urls for now until mine is merged

zone-live added a commit that referenced this pull request Jun 5, 2024
> [!NOTE]
> This PR is intended to be cherry-picked into RC 11.17.0 after being
merged into develop

## **Description**
Upgrade transaction-controller to get this fix:
MetaMask/core#4343

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24947?quickstart=1)

## **Related issues**

Fixes:
A recent update to the transaction-controller has made the
TransactionMeta object passed to the afterSign hook frozen. This change
prevents adding new properties, leading to the error: “Cannot add
property custodyId, object is not extensible.” This bug is breaking all
transactions for MMI as the original txMeta cannot store required
properties like custodyId.

Blocked by #24861 
Blocked by #24913

## **Manual testing steps**
I followed these steps to test the fix:
1. yarn && yarn start:mmi
2. create a new wallet
3. visit https://saturn-custody-ui.dev.metamask-institutional.io/ and
add two custodial addresses
4. send 0ETH from one custodial address to the other

You should see a popup with an Approve button. Before this fix, the
transaction would appear in the activity tab with an error.


## **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 completed the PR template to the best of my ability
- [ ] 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.

## **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 <metamaskbot@users.noreply.github.com>
Co-authored-by: Antonio Regadas <antonio.regadas@consensys.net>
danjm
danjm previously approved these changes Jun 5, 2024
@salimtb salimtb dismissed stale reviews from danjm, legobeat, and bergeron via 1ee4a84 June 5, 2024 10:04
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

Approving the privacy-snapshot.json update on behalf of privacy reviewers

@bergeron bergeron merged commit 3c4e17b into develop Jun 5, 2024
76 checks passed
@bergeron bergeron deleted the bump-assets-controller-version branch June 5, 2024 14:57
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2024
@gauthierpetetin gauthierpetetin added the release-12.0.0 Issue or pull request that will be included in release 12.0.0 label Jun 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-12.0.0 Issue or pull request that will be included in release 12.0.0 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants