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: improper implementation of handleBatchMethod + more robust tests #1065

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ecp4224
Copy link
Collaborator

@ecp4224 ecp4224 commented Oct 4, 2024

Explanation

This PR fixes a bug where the extension provider invokes the metamask_batch request logic correctly, but then also sends a metamask_batch request again. This (at best) causes the requests to be sent individually, then all at once or (at worst) causing metamask_batch to break entirely if the extension doesn't support the RPC

Currently, the latter case happens. The requests are sent individually, but then errors because it then tries to send metamask_batch and the extension doesn't support that

Screenshot 2024-10-04 at 2 17 14 PM

With the fixed logic in handleBatchMethod, the extension and SDK return the correct result

Screenshot 2024-10-04 at 2 38 59 PM

This PR also updates the unit tests for handleBatchMethod to be more robust to ensure this behavior doesn't regress.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • [N/A] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • [N/A] I've highlighted breaking changes using the "BREAKING" category above as appropriate

@ecp4224 ecp4224 self-assigned this Oct 4, 2024
@ecp4224 ecp4224 requested a review from a team as a code owner October 4, 2024 18:55
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.14%. Comparing base (23c2a19) to head (d7dafdd).

Files with missing lines Patch % Lines
...ider/extensionProviderHelpers/handleBatchMethod.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1065      +/-   ##
==========================================
- Coverage   78.16%   78.14%   -0.02%     
==========================================
  Files         179      179              
  Lines        4153     4155       +2     
  Branches     1020     1022       +2     
==========================================
+ Hits         3246     3247       +1     
- Misses        907      908       +1     

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

Copy link
Contributor

@elefantel elefantel left a comment

Choose a reason for hiding this comment

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

LGTM

@deeeed
Copy link

deeeed commented Oct 5, 2024

@christopherferreira9 Can you take a look at this PR to make sure everything is good with metamask_batch since we recently had regression?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants