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.

Sign up for GitHub

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

Jump to bottom

[bug] CCIP read is not working with a provider managed by this middleware #335

Closed
mirceanis opened this issue Sep 18, 2024 · 3 comments
Closed

[bug] CCIP read is not working with a provider managed by this middleware #335

mirceanis opened this issue Sep 18, 2024 · 3 comments
Assignees
@mikesposito
Labels
bug team-wallet-framework

Comments

@mirceanis
Copy link

ENS resolution sometimes uses CCIP read (eip-3668) to resolve an address.
That relies on a reverted execution being interpreted as a redirect.
When the provider used to perform such a read is managed by this middleware, the read operation fails.
My guess is that the reverted execution is somehow wrapped such that the error is no longer interpretable.

This bug showed up after the upgrade from v13 to v14 of this lib in the MetaMask extension.

The environment to reproduce it has [email protected] as a dependency, and a simple test to reproduce would go along the lines of:

const provider = new BrowserProvider(setup.provider); // provider managed by this middleware
const sampleDomain = 'jwt.ro'; // resolving a linked DNS domain uses CCIP read
const ensResolver = await provider.getResolver(sampleDomain);
const ethAddress = await ensResolver?.getAddress();
expect(ethAddress).toBe('0xd08E08a0551575eE6bcE6d56180148EB68Bca061');
@FrederikBolding
Copy link
Member

I have looked into this for a bit and I am pretty confident this isn't a bug in this library. Instead it is a symptom of mixing the old and new behaviour of serializeError in eth-rpc-errors / @metamask/rpc-errors.

Most of the stack, including this library is using the new error serialization either directly through @metamask/rpc-errors or through @metamask/json-rpc-engine, but the extension is still using the old version: https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/metamask-controller.js#L18

As far as I can tell, simply bumping json-rpc-engine in the extension should fix this: MetaMask/metamask-extension#22875

Gudahtt pushed a commit to MetaMask/test-dapp that referenced this issue Oct 10, 2024
@legobeat @Gudahtt
feat: Add ENS resolution
3216e2a 
A section has been added for resolving ENS addresses. This was made to
assist with reproducing this bug: MetaMask/eth-json-rpc-middleware#335
@Gudahtt Gudahtt mentioned this issue Oct 10, 2024
Merged
Gudahtt added a commit to MetaMask/test-dapp that referenced this issue Oct 11, 2024
@Gudahtt
feat: Add ENS resolution
5818ff8 
A section has been added for resolving ENS addresses. This was made to
assist with reproducing this bug: MetaMask/eth-json-rpc-middleware#335
@Gudahtt Gudahtt mentioned this issue Oct 11, 2024
Closed
This was referenced Oct 17, 2024
Merged
Closed
Gudahtt added a commit to MetaMask/test-dapp that referenced this issue Oct 17, 2024
@Gudahtt
feat: Add ENS resolution (#362)
21efc60 
A section has been added for resolving ENS addresses. This was made to
assist with reproducing this bug: MetaMask/eth-json-rpc-middleware#335
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this issue Oct 18, 2024
@legobeat
chore(deps): upgrade from json-rpc-engine to @metamask/json-rpc-engine (
eedeb24 
#22875)

## **Description**

## **Related issues**

- #27784
- MetaMask/eth-json-rpc-middleware#335
- #27917
- #18510
- #15250
- MetaMask/metamask-improvement-proposals#36

### Blocked by
- [x] #24496

### Follow-up to
- #24496

## **Manual testing steps**

## **Screenshots/Recordings**

### **Before**

### **After**

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [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-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **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.
vinistevam pushed a commit to MetaMask/metamask-extension that referenced this issue Oct 20, 2024
@legobeat @vinistevam
chore(deps): upgrade from json-rpc-engine to @metamask/json-rpc-engine (
dd34756 
#22875)

## **Description**

## **Related issues**

- #27784
- MetaMask/eth-json-rpc-middleware#335
- #27917
- #18510
- #15250
- MetaMask/metamask-improvement-proposals#36

### Blocked by
- [x] #24496

### Follow-up to
- #24496

## **Manual testing steps**

## **Screenshots/Recordings**

### **Before**

### **After**

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [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-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **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.
@mirceanis
Copy link
Author

I just tested this with MetaMask/metamask-extension#22875 and it seems to have fixed it.

@Gudahtt
Copy link
Member

Gudahtt commented Oct 21, 2024

Since this is not a bug in this library, but rather a result of using it with an incompatible version of json-rpc-engine, I will close this as fixed.

@Gudahtt Gudahtt closed this as completed Oct 21, 2024
Gudahtt pushed a commit to MetaMask/metamask-extension that referenced this issue Oct 21, 2024
@legobeat @Gudahtt
chore(deps): upgrade from json-rpc-engine to @metamask/json-rpc-engine (
0914812 
#22875)

- #27784
- MetaMask/eth-json-rpc-middleware#335
- #27917
- #18510
- #15250
- MetaMask/metamask-improvement-proposals#36

- [x] #24496

- #24496

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [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-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

- [ ] 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mikesposito mikesposito

Labels
bug team-wallet-framework
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mirceanis @FrederikBolding @Gudahtt @mikesposito