metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard
verified publisher icon MetaMask

feat: Throw when encountering unexpected RPC method hooks

Open rekmarks opened this issue 1 year ago • 6 comments

Description

Removes RPC handler hooks that had become unused. In order to prevent this from occurring again, also adds a check to fail immediately on dapp connections if extraneous hooks are provided. Finally, adds tests for createMethodMiddleware.js.

Note for reviewers

  • In an indication that this is useful, you'll note that the set of hooks removed in this PR is different from that the original PR (#20580).
  • In the commit history, you'll see an added and reverted TypeScript conversion of the method middleware and its tests. The RPC method handler types are a mess and we should probably wait to convert until we've resolved some upstream issues. Either way, the conversion would be too distracting for this PR.

Pre-merge author checklist

  • [x] I’ve followed MetaMask Coding Standards.
  • [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 format if applicable
  • [x] 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.

rekmarks avatar May 02 '24 21:05 rekmarks

Codecov Report

Attention: Patch coverage is 78.57143% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 67.46%. Comparing base (d27a233) to head (38cf3c7). Report is 50 commits behind head on develop.

:exclamation: Current head 38cf3c7 differs from pull request most recent head 2c1d1a3. Consider uploading reports for the commit 2c1d1a3 to get more accurate results

Files Patch % Lines
...ib/rpc-method-middleware/createMethodMiddleware.js 77.78% 6 Missing :warning:
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #24357      +/-   ##
===========================================
+ Coverage    67.37%   67.46%   +0.09%     
===========================================
  Files         1278     1286       +8     
  Lines        49881    50111     +230     
  Branches     12944    13006      +62     
===========================================
+ Hits         33605    33805     +200     
- Misses       16276    16306      +30

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 05 '24 01:05 codecov[bot]

Builds ready [350a322]
Page Load Metrics (659 ± 503 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint5512473147
domContentLoaded7161021
load4426916591048503
domInteractive7161021
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -260 Bytes (-0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar May 05 '24 01:05 metamaskbot

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.

github-actions[bot] avatar May 05 '24 06:05 github-actions[bot]

Builds ready [3a118aa]
Page Load Metrics (297 ± 345 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint5410970126
domContentLoaded86112115
load432475297719345
domInteractive86012115
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -260 Bytes (-0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar May 05 '24 17:05 metamaskbot

Builds ready [de5d01f]
Page Load Metrics (634 ± 553 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint58143832110
domContentLoaded8261242
load4735186341152553
domInteractive8261242
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -260 Bytes (-0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar May 05 '24 19:05 metamaskbot

Builds ready [62bbc83]
Page Load Metrics (984 ± 668 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint751431022211
domContentLoaded106318126
load6137259841392668
domInteractive106318126
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -260 Bytes (-0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar May 07 '24 16:05 metamaskbot

It does seem like this could be handled more consistently at the type level by TypeScript, but I'm assuming the issue is that there is still a lot of unmigrated js code that will benefit from this in the meantime?

legobeat avatar May 10 '24 02:05 legobeat

@legobeat there's definitely unmigrated JS code, but also the types for the method middleware and its handler + hooks pattern are currently broken and implemented in 3 different repositories.

🙃We're all trying to find the guy who did this...

I may try to wrangle the types once this is in.

rekmarks avatar May 10 '24 02:05 rekmarks

Builds ready [38cf3c7]
Page Load Metrics (736 ± 563 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint68142882211
domContentLoaded9391594
load5530677361172563
domInteractive9391594
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -260 Bytes (-0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar May 10 '24 02:05 metamaskbot

Builds ready [5edfc0d]
Page Load Metrics (745 ± 567 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint5914384209
domContentLoaded9281363
load4728937451181567
domInteractive9281363
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -260 Bytes (-0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar May 10 '24 18:05 metamaskbot

Missing release label release-11.17.0 on PR. Adding release label release-11.17.0 on PR and removing other release labels(release-11.18.0), as PR was added to branch 11.17.0 when release was cut.

metamaskbot avatar May 23 '24 23:05 metamaskbot