Skip to content

Conversation

@tommasini
Copy link
Contributor

@tommasini tommasini commented Jun 24, 2024

Description

This PR is blocked due to breaking changes on the errors thrown to the dapps. More info in this thread

This PR includes:

Related issues

Fixes:

Manual testing steps

  1. Go to in-app browser
  2. Test connect with multiple dapps
  3. Perform transaciton on test dapp

Screenshots/Recordings

Screen.Recording.2024-06-24.at.22.46.41.mov

Before

After

Pre-merge author checklist

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.

@tommasini tommasini requested a review from a team as a code owner June 24, 2024 21:50
@tommasini tommasini requested a review from a team June 24, 2024 21:50
@tommasini tommasini requested review from a team as code owners June 24, 2024 21:50
@tommasini tommasini changed the title feat: Update json-rpc-provider & json-rpc-middleware & json-rpc-filters chore: Update json-rpc-provider & json-rpc-middleware & json-rpc-filters Jun 24, 2024
@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.

@tommasini tommasini added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-platform Mobile Platform team Run Smoke E2E labels Jun 24, 2024
@socket-security
Copy link

socket-security bot commented Jun 24, 2024

@socket-security
Copy link

socket-security bot commented Jun 24, 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.

View full report↗︎

@sonarqubecloud
Copy link

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 49.49%. Comparing base (b013c71) to head (0debc05).
Report is 122 commits behind head on main.

Files Patch % Lines
app/core/WalletConnect/WalletConnectV2.ts 0.00% 2 Missing ⚠️
app/core/BackgroundBridge/BackgroundBridge.js 66.66% 1 Missing ⚠️
app/core/Snaps/SnapBridge.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10098      +/-   ##
==========================================
+ Coverage   47.24%   49.49%   +2.24%     
==========================================
  Files        1370     1427      +57     
  Lines       33304    34420    +1116     
  Branches     3586     3801     +215     
==========================================
+ Hits        15736    17035    +1299     
+ Misses      16607    16323     -284     
- Partials      961     1062     +101     

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

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

params: [helloWorldMessage],
};
const expectedError = rpcErrors.internal('Missing signature parameter');
const expectedError = rpcErrors.internal('Internal JSON-RPC error.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know if there's a possibility of breaking UI since the error shown is different than what was there 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.

Amazing question! There is a thread going on around this!

@christopherferreira9
Copy link
Contributor

Did a light check on SDK and it seems to be all good ✅

@tommasini tommasini added blocked DO-NOT-MERGE Pull requests that should not be merged labels Jun 26, 2024
@tommasini tommasini marked this pull request as draft June 26, 2024 20:22

"@metamask/eslint-plugin-design-tokens@^1.0.0":
version "1.1.0"
resolved "https://registry.yarnpkg.com/@metamask/eslint-plugin-design-tokens/-/eslint-plugin-design-tokens-1.1.0.tgz#c9d4471f04f62bfb307aa261d11b7a674eb27961"
Copy link

Choose a reason for hiding this comment

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

There's not a single additive change to dependencies outside of @metamask packages. Nothing to review in terms of supply-chain security specifically in this PR

@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions.

@github-actions github-actions bot added the stale Issues that have not had activity in the last 90 days label Sep 26, 2024
@legobeat legobeat removed the stale Issues that have not had activity in the last 90 days label Sep 26, 2024
@legobeat
Copy link
Contributor

This has been picked up in #11952

github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2024
## **Description**

- This PR is a rebase of #10098, including:
    * #9925
    * #9930
- Bump `@metamask/eth-json-rpc-filters` to `^7.0.0`
- Bump `@metamask/json-rpc-engine` to `^10.0.0`
- Bump `@metamask/eth-json-rpc-middleware` to `^15.0.0`
- Migrate from `json-rpc-middleware-stream` to
`@metamask/json-rpc-middleware-stream`
- Upgrade `@metamask/providers` from v13 to v16
  - Also broken out separately as #12085 
- Revert `Internal JSON-RPC error` message change to accomodate for
`@metamask/rpc-errors` v7


## **Related issues**

Expected to fix the following issues:

- [x] #11163
- [x] #11129
- [ ] #11105
- [ ] #9715
- [ ] #8308
- [x] #7926
- [x] #4621
- [x] #4646
- [ ] #12634

#### Blocked by
- [x] #12085
- [x] #12047
  - [x] #12024
    - [x] #11980 
- [x] #12008
- [x] #11978 

## **Manual testing steps**

1. Go to in-app browser
2. Test connect with multiple dapps
3. Perform transaciton on test dapp
1. Go to this page...

## **Screenshots/Recordings**


https://github.com/MetaMask/metamask-mobile/assets/46944231/c608d957-6684-40e2-8963-67a11dc610df

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] 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).
- [ ] 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: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
runway-github bot added a commit that referenced this pull request Dec 13, 2024
## **Description**

- This PR is a rebase of #10098, including:
    * #9925
    * #9930
- Bump `@metamask/eth-json-rpc-filters` to `^7.0.0`
- Bump `@metamask/json-rpc-engine` to `^10.0.0`
- Bump `@metamask/eth-json-rpc-middleware` to `^15.0.0`
- Migrate from `json-rpc-middleware-stream` to
`@metamask/json-rpc-middleware-stream`
- Upgrade `@metamask/providers` from v13 to v16
  - Also broken out separately as #12085 
- Revert `Internal JSON-RPC error` message change to accomodate for
`@metamask/rpc-errors` v7


## **Related issues**

Expected to fix the following issues:

- [x] #11163
- [x] #11129
- [ ] #11105
- [ ] #9715
- [ ] #8308
- [x] #7926
- [x] #4621
- [x] #4646
- [ ] #12634

#### Blocked by
- [x] #12085
- [x] #12047
  - [x] #12024
    - [x] #11980 
- [x] #12008
- [x] #11978 

## **Manual testing steps**

1. Go to in-app browser
2. Test connect with multiple dapps
3. Perform transaciton on test dapp
1. Go to this page...

## **Screenshots/Recordings**


https://github.com/MetaMask/metamask-mobile/assets/46944231/c608d957-6684-40e2-8963-67a11dc610df

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] 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).
- [ ] 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: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
runway-github bot added a commit that referenced this pull request Dec 13, 2024
## **Description**

- This PR is a rebase of #10098, including:
    * #9925
    * #9930
- Bump `@metamask/eth-json-rpc-filters` to `^7.0.0`
- Bump `@metamask/json-rpc-engine` to `^10.0.0`
- Bump `@metamask/eth-json-rpc-middleware` to `^15.0.0`
- Migrate from `json-rpc-middleware-stream` to
`@metamask/json-rpc-middleware-stream`
- Upgrade `@metamask/providers` from v13 to v16
  - Also broken out separately as #12085
- Revert `Internal JSON-RPC error` message change to accomodate for
`@metamask/rpc-errors` v7

## **Related issues**

Expected to fix the following issues:

- [x] #11163
- [x] #11129
- [ ] #11105
- [ ] #9715
- [ ] #8308
- [x] #7926
- [x] #4621
- [x] #4646
- [ ] #12634

#### Blocked by
- [x] #12085
- [x] #12047
  - [x] #12024
    - [x] #11980
- [x] #12008
- [x] #11978

## **Manual testing steps**

1. Go to in-app browser
2. Test connect with multiple dapps
3. Perform transaciton on test dapp
1. Go to this page...

## **Screenshots/Recordings**

https://github.com/MetaMask/metamask-mobile/assets/46944231/c608d957-6684-40e2-8963-67a11dc610df

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] 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).
- [ ] 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: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
runway-github bot pushed a commit that referenced this pull request Dec 13, 2024
)

## **Description**

- This PR is a rebase of #10098, including:
    * #9925
    * #9930
- Bump `@metamask/eth-json-rpc-filters` to `^7.0.0`
- Bump `@metamask/json-rpc-engine` to `^10.0.0`
- Bump `@metamask/eth-json-rpc-middleware` to `^15.0.0`
- Migrate from `json-rpc-middleware-stream` to
`@metamask/json-rpc-middleware-stream`
- Upgrade `@metamask/providers` from v13 to v16
  - Also broken out separately as #12085 
- Revert `Internal JSON-RPC error` message change to accomodate for
`@metamask/rpc-errors` v7


## **Related issues**

Expected to fix the following issues:

- [x] #11163
- [x] #11129
- [ ] #11105
- [ ] #9715
- [ ] #8308
- [x] #7926
- [x] #4621
- [x] #4646
- [ ] #12634

#### Blocked by
- [x] #12085
- [x] #12047
  - [x] #12024
    - [x] #11980 
- [x] #12008
- [x] #11978 

## **Manual testing steps**

1. Go to in-app browser
2. Test connect with multiple dapps
3. Perform transaciton on test dapp
1. Go to this page...

## **Screenshots/Recordings**


https://github.com/MetaMask/metamask-mobile/assets/46944231/c608d957-6684-40e2-8963-67a11dc610df

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] 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).
- [ ] 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: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
sethkfman added a commit that referenced this pull request Dec 13, 2024
)

- fix: replace legacy eth-json-rpc deps (#11952)

## **Description**

- This PR is a rebase of #10098, including:
    * #9925
    * #9930
- Bump `@metamask/eth-json-rpc-filters` to `^7.0.0`
- Bump `@metamask/json-rpc-engine` to `^10.0.0`
- Bump `@metamask/eth-json-rpc-middleware` to `^15.0.0`
- Migrate from `json-rpc-middleware-stream` to
`@metamask/json-rpc-middleware-stream`
- Upgrade `@metamask/providers` from v13 to v16
  - Also broken out separately as #12085 
- Revert `Internal JSON-RPC error` message change to accomodate for
`@metamask/rpc-errors` v7


## **Related issues**

Expected to fix the following issues:

- [x] #11163
- [x] #11129
- [ ] #11105
- [ ] #9715
- [ ] #8308
- [x] #7926
- [x] #4621
- [x] #4646
- [ ] #12634

#### Blocked by
- [x] #12085
- [x] #12047
  - [x] #12024
    - [x] #11980 
- [x] #12008
- [x] #11978 

## **Manual testing steps**

1. Go to in-app browser
2. Test connect with multiple dapps
3. Perform transaciton on test dapp
1. Go to this page...

## **Screenshots/Recordings**



https://github.com/MetaMask/metamask-mobile/assets/46944231/c608d957-6684-40e2-8963-67a11dc610df

### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding

Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] 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).
- [ ] 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: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
[d967a76](d967a76)

Co-authored-by: legobeat <[email protected]>
Co-authored-by: Aslau Mario-Daniel <[email protected]>
Co-authored-by: tommasini <[email protected]>
Co-authored-by: kylanhurt <[email protected]>
Co-authored-by: sethkfman <[email protected]>
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
@tommasini tommasini closed this Jan 17, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

blocked DO-NOT-MERGE Pull requests that should not be merged needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-platform Mobile Platform team

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

8 participants