CCIP-Read + Wildcard resolution #4759
Conversation
|
This pull request fixes 1 alert when merging 20a0485 into 2a10e24 - view on LGTM.com fixed alerts:
|
jdevcs
requested review from
nazarhussain,
spacesailor24,
luu-alex and
jdevcs
|
@nazarhussain Thanks very much for your feedback, will tighten up the code, address your comments and move this out of WIP/Draft shortly. |
LeonmanRolls
force-pushed
the
ccip-read
branch
from
61c9563
to
6353c3e
Compare
packages/web3-http/package.json
Outdated
| @@ -0,0 +1,21 @@ | |||
| { | |||
| "name": "web3-http", | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
|
This pull request introduces 6 alerts when merging 48c3ce0 into 38295dc - view on LGTM.com new alerts:
|
LeonmanRolls
force-pushed
the
ccip-read
branch
6 times, most recently
from
3b4bb31
to
838816f
Compare
|
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 |
LeonmanRolls
changed the title
|
@jdevcs @nazarhussain would it be possible to take another look at this? |
|
hi @nazarhussain Did you have time to review the PR? |
|
|
|
@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 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. |
|
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. |
|
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. |
|
@LeonmanRolls Could you rebase your branch and make sure it works with the new fetch API? We no longer use the buggy |
|
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. |
|
Will submit a corresponding PR for v4 |
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
Checklist:
npm run dtslintwith success and extended the tests and types if necessary.npm run test:unitwith success.npm run test:covand my test cases cover all the lines and branches of the added code.npm run buildand testeddist/web3.min.jsin a browser.CHANGELOG.mdfile in the root folder.