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: upgrade transaction-controller to 32.0.0 to fix mmi #24947

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

dbrans
Copy link
Contributor

@dbrans dbrans commented May 31, 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

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

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.

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.

@dbrans dbrans added the team-transactions Transactions team label May 31, 2024
@dbrans
Copy link
Contributor Author

dbrans commented May 31, 2024

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

Copy link

socket-security bot commented May 31, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/nonce-tracker@5.0.0 None 0 0 B
npm/@metamask/transaction-controller@30.0.0 None +2 81.4 kB

View full report↗︎

@dbrans dbrans marked this pull request as ready for review May 31, 2024 18:47
@dbrans dbrans requested review from a team as code owners May 31, 2024 18:47
@dbrans dbrans changed the title fix: upgrade transaction-controller to fix mmi fix: upgrade transaction-controller to 31.0.0 fix mmi May 31, 2024
@dbrans dbrans changed the title fix: upgrade transaction-controller to 31.0.0 fix mmi fix: upgrade transaction-controller to 32.0.0 to fix mmi May 31, 2024
@dbrans
Copy link
Contributor Author

dbrans commented May 31, 2024

@metamaskbot update-policies

@legobeat
Copy link
Contributor

legobeat commented May 31, 2024

@dbrans Can we get this rebased on top of #24861 so we can update it 29 -> 30 -> 32? :)

Would be great to have the intermediary update to 30 merged independently so changed can be observed separately, I think?

@metamaskbot
Copy link
Collaborator

Policies updated

zone-live
zone-live previously approved these changes Jun 3, 2024
Copy link
Contributor

@zone-live zone-live left a comment

Choose a reason for hiding this comment

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

LGTM!

@legobeat
Copy link
Contributor

legobeat commented Jun 3, 2024

@zone-live #24861

@legobeat legobeat marked this pull request as draft June 3, 2024 09:40
@zone-live
Copy link
Contributor

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@zone-live
Copy link
Contributor

Hi @dbrans, I noticed it had some conflicts with develop, so I went ahead and fixed them, think it looks good 👍🏼

@dbrans dbrans marked this pull request as ready for review June 4, 2024 12:59
@metamaskbot
Copy link
Collaborator

Builds ready [83d3ffa]
Page Load Metrics (54 ± 5 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6611884115
domContentLoaded9391263
load437954105
domInteractive9391263

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.71%. Comparing base (d27a233) to head (83d3ffa).
Report is 242 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #24947      +/-   ##
===========================================
- Coverage    67.37%   65.71%   -1.66%     
===========================================
  Files         1278     1369      +91     
  Lines        49881    54397    +4516     
  Branches     12944    14155    +1211     
===========================================
+ Hits         33605    35745    +2140     
- Misses       16276    18652    +2376     

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

@zone-live zone-live merged commit b210b6f into develop Jun 5, 2024
79 checks passed
@zone-live zone-live deleted the dbrans/txc-upgrade branch June 5, 2024 09:09
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2024
@metamaskbot metamaskbot added release-11.18.0 release-11.16.8 Issue or pull request that will be included in release 11.16.8 and removed release-11.18.0 labels Jun 5, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-11.16.8 on PR. Adding release label release-11.16.8 on PR and removing other release labels(release-11.18.0), as PR was cherry-picked in branch 11.16.8.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.16.8 Issue or pull request that will be included in release 11.16.8 team-transactions Transactions team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants