Skip to content

Conversation

@legobeat
Copy link
Contributor

@legobeat legobeat commented Apr 17, 2024

  • Remove dependency on deprecated web3-provider-engine
  • Remove dependency on legacy ethjs packages by bumping transtitive eth-method-registry
  • Upgrade eth-sig-util with bugfixes
  • Align transitive usage of @metamask/eth-query with direct package version

This seems to be the highest we can bump while still having support for BaseControllerV1 but also the lowest without requiring downgrades of peer dependencies. It also aligns @metamask/controller-utils to a single version. So seems like a sweet spot for the next upgrade cycle.

This is a breaking change.

Blocked by

@socket-security
Copy link

socket-security bot commented Apr 17, 2024

👍 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.

Ignoring: npm/@ethersproject/providers@5.7.2, npm/@ethersproject/web@5.7.1, npm/@metamask/eth-json-rpc-infura@9.1.0, npm/@metamask/eth-json-rpc-middleware@12.1.0, npm/@metamask/safe-event-emitter@3.1.1, npm/bech32@1.1.4, npm/eth-block-tracker@8.1.0, npm/eth-json-rpc-middleware@8.1.0, npm/eth-method-registry@3.0.0

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

@legobeat
Copy link
Contributor Author

@SocketSecurity ignore npm/eth-block-tracker@8.1.0
@SocketSecurity ignore npm/eth-method-registry@3.0.0
@SocketSecurity ignore npm/@metamask/eth-json-rpc-infura@9.1.0
@SocketSecurity ignore npm/@metamask/safe-event-emitter@3.1.1

new authors ok

@legobeat
Copy link
Contributor Author

@SocketSecurity ignore npm/@metamask/eth-json-rpc-middleware@12.1.0

network access ok

Copy link

@nikoferro nikoferro left a comment

Choose a reason for hiding this comment

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

LGTM, makes sense to update it to this version also, specially since we are including this in a new major

@legobeat legobeat force-pushed the deps-gas-fee-controller-12 branch from 23db9a2 to ee4cfc5 Compare April 17, 2024 10:21
@legobeat legobeat marked this pull request as ready for review April 17, 2024 20:40
@legobeat legobeat requested a review from dan437 as a code owner April 17, 2024 20:40
@legobeat legobeat force-pushed the deps-gas-fee-controller-12 branch from ee4cfc5 to 59843e5 Compare April 17, 2024 20:40
@legobeat legobeat requested a review from a team April 17, 2024 20:40
@legobeat legobeat requested a review from dan437 as a code owner April 17, 2024 20:40
@legobeat legobeat requested a review from a team April 17, 2024 20:40
@legobeat

This comment was marked as resolved.

@legobeat legobeat requested a review from nikoferro April 17, 2024 21:41
@legobeat

This comment was marked as resolved.

@legobeat
Copy link
Contributor Author

legobeat commented May 6, 2024

@nikoferro Do you see any blockers or outstanding issues here?

@legobeat
Copy link
Contributor Author

legobeat commented May 9, 2024

To help reviewing, this has been published as @metamask/swaps-controller@9.0.0-rc1 and integration-test builds for mobile are available in MetaMask/metamask-mobile#9584.

@legobeat

This comment was marked as resolved.

@legobeat legobeat requested a review from infiniteflower May 9, 2024 21:32
@legobeat
Copy link
Contributor Author

legobeat commented May 13, 2024

If anyone else wants to merge, feel free to go ahead. I'm holding off until we have an identified resolution for the regression behind the revert in:

(#159 (comment))

@nikoferro
Copy link

@legobeat this one looks good, we should still merge first a fix for the revert on a potential 8.0.1 version

revert: MetaMask/metamask-mobile#9612
potential solution: #241

@legobeat
Copy link
Contributor Author

this one looks good, we should still merge first a fix for the revert on a potential 8.0.1 version

Thanks, and those were my thoughts too but I guess we're going for an 8.1.0 and then an 8.1.1 with the fix included, considering that #229 is now merged?

@legobeat legobeat force-pushed the deps-gas-fee-controller-12 branch 2 times, most recently from badf201 to 11cf032 Compare May 16, 2024 08:26
@desi desi requested a review from a team May 23, 2024 19:23
@legobeat legobeat force-pushed the deps-gas-fee-controller-12 branch from 11cf032 to 03545bc Compare May 24, 2024 00:15
legobeat added 10 commits May 25, 2024 08:34
- devDeps: bump @metamask/composable-controller to match base-controller
  version
required to match types for transitive deps from metamask controllers
required to align transitive @metamask/eth-query
- peerDeps: @metamask/composable-controller@^3.0.3->^4.0.0
  - to match @metamask/base-controller versions
  - latest version supporting BaseControllerV1
@legobeat legobeat force-pushed the deps-gas-fee-controller-12 branch from 03545bc to 042c55e Compare May 24, 2024 23:34
@legobeat legobeat merged commit 8e91777 into main May 24, 2024
@legobeat legobeat deleted the deps-gas-fee-controller-12 branch May 24, 2024 23:36
nikoferro added a commit to MetaMask/metamask-mobile that referenced this pull request Jun 10, 2024
## **Description**

- Upgrade `@metamask/swaps-controller` from v8 to v9
  - Remove redundant patches 

Resolves a bunch of outstanding legacy, including `web-provider-engine`.

Also enables Base Network for Swaps.

## **Related issues**

- #9286 `@metamask/swaps-controller`
  - v8 upgrade 
- #9612
  - reverted v8 upgrade 
- MetaMask/swaps-controller#241
  - fixed issue that motivated revert of v8 upgrade 

#### Blocked by
- [x] MetaMask/swaps-controller#245
  - [x] MetaMask/swaps-controller#219

This PR also patches `swaps-controller@9.0.0` to fix the `web3` import
that works differently between the test environment and the actual
mobile app / E2E test environment. This should be removed once a
`swaps-controller` version addressing this is published.
(MetaMask/swaps-controller#255)

## **Manual testing steps**

Regression over swaps functionality in the 4 different scenarios:

ETH -> WETH
WETH -> ETH
ETH -> ERC20
ERC20 -> ETH

## **Screenshots/Recordings**

### **Before**

### **After**

![1](https://github.com/MetaMask/metamask-mobile/assets/8658960/894a8d06-e23c-4da1-a081-dd711b37de04)

![2](https://github.com/MetaMask/metamask-mobile/assets/8658960/064d5615-b4a0-4f15-8063-9ca60062d28c)

![3](https://github.com/MetaMask/metamask-mobile/assets/8658960/9f459e50-2840-4168-b7e5-e3526df4d957)

![4](https://github.com/MetaMask/metamask-mobile/assets/8658960/cd770b39-4d35-4725-aa49-0e84e51e72ac)

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [x] 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-mobile/blob/main/.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).
- [x] 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: nikoferro <nicolaspatricioferro@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants