CCIP-Read + Wildcard resolution

[LeonmanRolls]

Description

The intention of this PR is to add CCIP-read support to web3.js.

CCIP-read TLDR: A contract that needs off-chain data can revert with a special error, which will point web3js to a web server where the info can be gathered. The error will also specify which contract function to subsequently call with this off-chain data.

Changes have been made in three main places:

web3-core-method: In order to hook into transaction errors and check for CCIP-read errors. web3-ccip-read: Functions that implement the CCIP-read protocol web3-http: Module for making simple http requests

This is a draft to check I'm going in the right direction.

Type of change

• [x] New feature (non-breaking change which adds functionality)

Checklist:

• [x] I have selected the correct base branch.
• [x] I have performed a self-review of my own code.
• [x] I have commented my code, particularly in hard-to-understand areas.
• [x] I have made corresponding changes to the documentation.
• [x] My changes generate no new warnings.
• [x] Any dependent changes have been merged and published in downstream modules.
• [x] I ran npm run dtslint with success and extended the tests and types if necessary.
• [x] I ran npm run test:unit with success.
• [ ] I ran npm run test:cov and my test cases cover all the lines and branches of the added code.
• [x] I ran npm run build and tested dist/web3.min.js in a browser.
• [x] I have tested my code on the live network.
• [ ] I have checked the Deploy Preview and it looks correct.
• [x] I have updated the CHANGELOG.md file in the root folder.

[lgtm-com[bot]]

This pull request fixes 1 alert when merging 20a04858e04a3b45d9d32af29ba4a9fbbdc91a3f into 2a10e2476b5421a0ac6776a70fc28cfc7b8bfa93 - view on LGTM.com

fixed alerts:

• 1 for Prototype-polluting assignment

[LeonmanRolls]

@nazarhussain Thanks very much for your feedback, will tighten up the code, address your comments and move this out of WIP/Draft shortly.

[jdevcs]

Hi, @LeonmanRolls , will you have time for discussion or addressing feedback of this PR? Thanks

[LeonmanRolls]

Hi, @LeonmanRolls , will you have time for discussion or addressing feedback of this PR? Thanks

Hi yes sure I will address your feedback sorry for the delay, i did also respond to one of your comments but I think you missed it so will post here:

I think you should use existing http provider package in web3.js and modify/add new functionality in that instead of creating new package here.

Hey are you suggesting modifying/adding functionality to the provider itself or adding a separate class to the provider package? Also I think that a provider as an abstraction is quite different to general http requests, and most of the other packages seem to have a single purpose. Happy to add the functionality to the provider package if you still think it should go there.

[LeonmanRolls]

I would second @jdevcs feedback. We should not introduce new package web3-http instead use existing web3-providers-http package. You can create a private instance of the provider and use .send method to achieve the same.

Happy to do it, I have a couple of points I need guidance on:

• The current provider doesn’t support GET requests, should I modify _prepareRequest to take a method option?
• The provider generally hides details about the request itself, which are needed by ccip-read, e.g. response status code and content-type header. I think I need to add another method to the provider that would expose these details.
• One possibility is moving the current web3-http methods over to the provider class.

Let me know what you think!

[lgtm-com[bot]]

This pull request introduces 1 alert when merging bfeafbf4751731a4846c52a118150c834f2b2811 into 63e73bb3f8d4720edcc31441c3b613758ad96cec - view on LGTM.com

new alerts:

• 1 for Useless conditional

[LeonmanRolls]

Hi @LeonmanRolls Thanks for your contributions in web3.js , could you remove package lock files from this PR:

• package-lock.json
• packages/web3-ccip-read/package-lock.json
• packages/web3-http/package-lock.json

Also currently following are not passing:

Build / lint (pull_request)
Build / e2e (unit_and_e2e_clients) (pull_request)
Build / e2e (e2e_browsers) (pull_request) 
Build / e2e (e2e_ganache) (pull_request)
Build / e2e_windows (pull_request) 

could you check and fix failing tests on your local system.

Hey @jdevcs , I'm presuming you wanted me to resolve the merge conflicts instead of removing the package-lock files so have done that (as they are there on the current 1.x branch).

Running linting and tests on my machine is a little tricky as many are failing on a fresh pull of the 1.x branch too. I do however now have the same number of failing tests on my branch locally as I do for 1.x, with all the new ones that Iv'e added passing.

Linting is a bit tricky too because it includes the built files (the ones inside the lib folders). I get 20k+ linting errors on a fresh pull of 1.x locally.

Please advise on how to proceed, may also be easier to talk on discord.

[lgtm-com[bot]]

This pull request introduces 6 alerts when merging 4c1c69b91a45c1e6e20d2314468a20c63f815288 into 38295dc03fc3ec8d24ab7ded4d9f933e73328318 - view on LGTM.com

new alerts:

• 4 for Unused variable, import, function or class
• 2 for Expression has no effect

[lgtm-com[bot]]

This pull request introduces 6 alerts when merging 48c3ce08eab54505bd8e36ead8365b36e5d92be8 into 38295dc03fc3ec8d24ab7ded4d9f933e73328318 - view on LGTM.com

new alerts:

• 4 for Unused variable, import, function or class
• 2 for Expression has no effect

[LeonmanRolls]

Hi @jdevcs @nazarhussain

I have moved the network request code to be part of the provider now.

I have also added in wildcard resolution as described here: https://docs.ens.domains/ens-improvement-proposals/ensip-10-wildcard-resolution. They work together to facilitate fetching off-chain ENS data so thought it would make sense to merge them as one PR.

I've gone through the checklist as best I can. A fresh pull of 1.x branch has 19 failing tests for me, so this branch has the same number of failing tests, none of the tests i've added have introduced new failing tests locally. I haven't checked the test:unit and test:cov boxes for this reason.

[Arachnid]

@jdevcs @nazarhussain would it be possible to take another look at this?

[makoto]

hi @nazarhussain Did you have time to review the PR?

[GregTheGreek]

~~deleted because my comment didn't make sense~~

[GregTheGreek]

@Arachnid @makoto @LeonmanRolls

This PR is looking to add a specific EIP as a new core package to web3.js. Unless this EIP is being widely used (mass adoption) in the community I do not think this is something we want to be including within web3.js, at least as its own package. If this is something that is crucial for ENS to work, please consider including it directly in the web3-ens package so it can be consumed by your users that way.

We do not want to set a precedent that any single EIP can be implemented into web3.js prior to adoption, with caveats for changes such as the wallet provider API.

[Arachnid]

I'm sorry to hear that. This change is crucial to ENS, so we're happy to implement it in the ENS package if that's your preference. Because it's a general-purpose standard for fetching offchain data, we figured it would be useful to web3 users to have it available outside ENS; it seems a shame to implement the functionality for ENS only.

Can you clarify what you mean by "prior to adoption"? ethers.js already supports EIP 3668, and it seems like a bit of a catch 22 to require that it be already used before you support it.

[LeonmanRolls]

Hi @GregTheGreek , I've moved the logic for CCIP-Read into the web3-core-method package (as a separate file) as that's the only place it's used at the moment, and including it in web3-eth-ens causes circular dependency issues.

[ghost]

@LeonmanRolls Could you rebase your branch and make sure it works with the new fetch API? We no longer use the buggy xhr2-cookies lib to make HTTP requests anymore.

[github-actions[bot]]

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[LeonmanRolls]

Will submit a corresponding PR for v4