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

CCIP Read support #2457

Merged
merged 4 commits into from
Jun 1, 2022
Merged

CCIP Read support #2457

merged 4 commits into from
Jun 1, 2022

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented May 2, 2022 •
edited
Loading

What was wrong?

How was it fixed?

  • CCIP Read / EIP-3668 support was added:
    • New OffchainLookup exception added with payload property to be filled with appropriate data from the contract revert.
    • Sync and async implementations added via ccip_read_enabled flags on contract call / eth_call.
    • The bulk of offchain lookup payload handling was abstracted to new public utils apis for exception handling: web3/utils/exception_handling.py and web3/utils/async_exception_handling.py.
    • Test contracts created and added to geth fixture.
    • Test fixtures for offchain_lookup_contract added where setup was needed.
    • ENS offchain resolver support tested separately via a new contract setup within tests/ens.
    • GET request support added and general, non-json-rpc, POST request support added.
  • Some refactoring:
    • Refactored requests.py to account for json payloads for both sync and async POST and GET requests.

Todo:

  • Rebase with master once the ENSIP-10 PR is merged in
  • Add entry to the release notes
  • Add cute animal picture

Cute Animal Picture

@fselmo fselmo force-pushed the ccip-read-support branch 8 times, most recently from 56d46c8 to 2995523 Compare May 5, 2022 17:09
@fselmo fselmo force-pushed the ccip-read-support branch 4 times, most recently from 206626b to 05da735 Compare May 11, 2022 01:24
@fselmo fselmo changed the title [WIP] CCIP Read support CCIP Read support May 11, 2022
@fselmo fselmo force-pushed the ccip-read-support branch 14 times, most recently from 3332bec to 17e34fe Compare May 16, 2022 05:33
@fselmo fselmo marked this pull request as ready for review May 16, 2022 05:38
@fselmo fselmo requested review from kclowes and wolovim May 16, 2022 05:38
@fselmo
Copy link
Collaborator Author

fselmo commented May 17, 2022 •
edited
Loading

We should probably add some docs

Indeed, was waiting to iron out the implementation beforehand.

It doesn't look like this has been implemented for transact or estimate_gas. I think the spec mentions both of those methods. Do you think those belong in a new PR or should they go in this one?

I went a bit back and forth on this... I think we should talk about how this might look like / maybe add this in a future PR. See my reply to your comment on Contract.call() vs Contract.transact() above.

I'm probably missing something, but I'm curious about the decision to eliminate the explicit data param that gets passed in to make_post_request here

Having data as a positional argument and then passing it through to session.post() as a keyword argument just got a bit muddied when I was refactoring this code having data be either bytes or dict since this is now the case. Most of these issues came from linting so now that all of the rest is linted I don't mind taking another look.

kclowes
kclowes reviewed May 18, 2022
View reviewed changes
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Okay, I got through the rest! Just a few nitpicky comments

web3/_utils/request.py Show resolved Hide resolved
web3/_utils/transactions.py Outdated Show resolved Hide resolved
web3/eth.py Show resolved Hide resolved
web3/_utils/method_formatters.py Show resolved Hide resolved
web3/_utils/module_testing/eth_module.py Outdated Show resolved Hide resolved
fselmo added a commit to fselmo/web3.py that referenced this pull request May 20, 2022
@fselmo
Docs and refactoring from discussions on PR ethereum#2457
8bfacba 
- Add documentation
- Refactor out the provider flag since there is no support for sending transactions w/ CCIP Read support
- Refactor some around the way the ``eth_call`` is made, add flag directly to ``eth_call`` and make ``durin_call()`` and internal ``_durin_call()``
fselmo added a commit to fselmo/web3.py that referenced this pull request May 20, 2022
@fselmo
Docs and refactoring from discussions on PR ethereum#2457
234afdd 
- Add documentation
- Refactor out the provider flag since there is no support for sending transactions w/ CCIP Read support
- Refactor some around the way the ``eth_call`` is made, add flag directly to ``eth_call`` and make ``durin_call()`` and internal ``_durin_call()``
fselmo added a commit to fselmo/web3.py that referenced this pull request May 20, 2022
@fselmo
Docs and refactoring from discussions on PR ethereum#2457
147c011 
- Add documentation
- Refactor out the provider flag since there is no support for sending transactions w/ CCIP Read support
- Refactor some around the way the ``eth_call`` is made, add flag directly to ``eth_call`` and make ``durin_call()`` and internal ``_durin_call()``
fselmo added a commit to fselmo/web3.py that referenced this pull request May 20, 2022
@fselmo
Docs and refactoring from discussions on PR ethereum#2457
46c1600 
- Add documentation
- Refactor out the provider flag since there is no support for sending transactions w/ CCIP Read support
- Refactor some around the way the ``eth_call`` is made, add flag directly to ``eth_call`` and make ``durin_call()`` and internal ``_durin_call()``
fselmo added a commit to fselmo/web3.py that referenced this pull request May 23, 2022
@fselmo
Refactor based on comments in PR ethereum#2457
fb620c0
fselmo added a commit to fselmo/web3.py that referenced this pull request May 23, 2022
@fselmo
Refactor based on comments in PR ethereum#2457
ab40c9f
@fselmo
CCIP Read support and some refactoring:
1d51742 
- Create the OffchainLookup exception and package the payload from the revert message appropriately. Raise the exception when the appropriate revert is caught.

- Overhaul transactions.py and async_transactions.py to handle most of the offchain lookup payload processing via the respective handle_offchain_lookup methods.

- Refactor request.py a bit to account for json responses for both POST and GET requests that are not formatted to JSON-RPC specs.

- Due to possible security risks, CCIP Read support is added as a ``ccip_read_enabled`` flag to the ``BaseProvider`` class whose default value is ``False`` but can be turned on explicitly when a user needs the functionality.

- ``ccip_read_enabled`` flags are also added as options to contract calls so the user can make this choice on a per-call basis to specific functions of the contract.

Extras:

- Simplify the way we handle different ENS resolvers. Use one main ABI but perform checks against ``supportsInterface()`` at the appropriate times when handling different cases.

- Added helper utility methods for converting between types within a new ``type_conversion_utils.py`` file.

- Fix the typing for state_override argument to eth_call due to a mypy linting error locally: create ``CallOverride`` type that maps the ``CallOverrideParams`` as values to ``ChecksumAddress`` keys.
@fselmo
Name change correction from ENSIP-10 implementation
61c1cfc
fselmo added a commit to fselmo/web3.py that referenced this pull request May 25, 2022
@fselmo
Docs and refactoring from discussions on PR ethereum#2457
39a4822 
- Add documentation
- Refactor out the provider flag since there is no support for sending transactions w/ CCIP Read support
- Refactor some around the way the ``eth_call`` is made, add flag directly to ``eth_call`` and make ``durin_call()`` and internal ``_durin_call()``
fselmo added a commit to fselmo/web3.py that referenced this pull request May 25, 2022
@fselmo
Refactor based on comments in PR ethereum#2457
f23fd94
@fselmo fselmo force-pushed the ccip-read-support branch 2 times, most recently from d74d47d to afcbb48 Compare May 25, 2022 22:33
fselmo added a commit to fselmo/web3.py that referenced this pull request May 25, 2022
@fselmo
Docs and refactoring from discussions on PR ethereum#2457
32e997f 
- Add documentation
- Refactor out the provider flag since there is no support for sending transactions w/ CCIP Read support
- Refactor some around the way the ``eth_call`` is made, add flag directly to ``eth_call`` and make ``durin_call()`` and internal ``_durin_call()``
- Refactor data back into post request after linting issues went away
- Make CCIP Read max redirects configurable and add tests
- OffchainLookup should extend from ContractLogicError
- Add some more testing and refactor some more; use web3 ValidationError instead of importing from eth_utils
- Add MultipleFailedRequests if multiple failed requests for similar / same data
kclowes
kclowes approved these changes Jun 1, 2022
View reviewed changes
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

:shipit:!

@fselmo
Docs and refactoring from discussions on PR ethereum#2457
6040b53 
- Add documentation
- Refactor out the provider flag since there is no support for sending transactions w/ CCIP Read support
- Refactor some around the way the ``eth_call`` is made, add flag directly to ``eth_call`` and make ``durin_call()`` and internal ``_durin_call()``
- Refactor data back into post request after linting issues went away
- Make CCIP Read max redirects configurable and add tests
- OffchainLookup should extend from ContractLogicError
- Add some more testing and refactor some more; use web3 ValidationError instead of importing from eth_utils
- Add MultipleFailedRequests if multiple failed requests for similar / same data
- Reconcile changes with rebased async contract
- Raise an exception if CCIP Read urls do not contain expected keyword
- Add tests for ENS offchain resolution via CCIP Read functionality
- CCIP Read support for EthereumTesterProvider / eth-tester
@fselmo
Performance changes for CI builds
f1ce16f 
- Split CI jobs into async and sync and further split into eth vs non-eth modules
- Run ``unlocked_account`` pytest fixture as ``module`` scope.
- Organize sync and async spaces in ``go_ethereum/common.py``
@fselmo fselmo merged commit 0c2ab5e into ethereum:master Jun 1, 2022
fselmo added a commit that referenced this pull request Jun 1, 2022
@fselmo
Docs and refactoring from discussions on PR #2457
9a796b9 
- Add documentation
- Refactor out the provider flag since there is no support for sending transactions w/ CCIP Read support
- Refactor some around the way the ``eth_call`` is made, add flag directly to ``eth_call`` and make ``durin_call()`` and internal ``_durin_call()``
- Refactor data back into post request after linting issues went away
- Make CCIP Read max redirects configurable and add tests
- OffchainLookup should extend from ContractLogicError
- Add some more testing and refactor some more; use web3 ValidationError instead of importing from eth_utils
- Add MultipleFailedRequests if multiple failed requests for similar / same data
- Reconcile changes with rebased async contract
- Raise an exception if CCIP Read urls do not contain expected keyword
- Add tests for ENS offchain resolution via CCIP Read functionality
- CCIP Read support for EthereumTesterProvider / eth-tester
@fselmo fselmo mentioned this pull request Jun 1, 2022
Closed
@fselmo fselmo deleted the ccip-read-support branch April 3, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pacrob pacrob pacrob left review comments

@kclowes kclowes kclowes approved these changes

@wolovim wolovim Awaiting requested review from wolovim

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fselmo @pacrob @kclowes