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

CCIP Read implementation #41

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

pikonha
Copy link

@pikonha pikonha commented Mar 19, 2024

CCIP Read implementation

Description

This pull request adds the CCIP-Read support to the Go-ENS library. This function enables users to read data from offchain data sources that follow the EIP-3668 standard.

These additions enhance the versatility and utility of the Go-ENS library by enabling seamless interaction with contracts and providing efficient domain resolution capabilities. The implementation aligns with the library's objective of comprehensive support for Ethereum Name Service (ENS) operations.

How it works

  1. Go-ENS calls the Universal Resolver ENS' contract with the intent of forward resolving a domain such as jesse.cd.id (externally handled by Coinbase's Offchain Resolver)
  2. The Universal Resolver returns the address of the offchain resolver
  3. Go-ENS calls the addr(namehash) or resolve(labelhash, encodedArgs) function on the contract which reverts with OffchainLookup as described on the EIP-3668
  4. The client then parses the given error in order to get the following parameters:
    • sender: contract's address
    • urls: offchain API URL
    • callData: encoded function call that should be redirected to the API
    • callbackFunction: validation function that should be called with the API's response
    • extraData: encoded function call with arbitrary arguments
  5. A request to the given URL is made with sender and calldata as arguments following the Gateway Interface
  6. The callbackFunction is then called with the API's response and the extraData to validate the authenticity of the data
  7. The Offchain Resolver's callback function response is then parsed and returned by the caller
┌──────┐                                          ┌────────┐ ┌─────────────┐
│GO-ENS│                                          │Contract│ │   Gateway   │
└──┬───┘                                          └───┬────┘ └──────┬──────┘
   │                                                  │             │
   │ somefunc(...)                                    │             │
   ├─────────────────────────────────────────────────►│             │
   │                                                  │             │
   │ revert OffchainLookup(sender, urls, callData,    │             │
   │                     callbackFunction, extraData) │             │
   │◄─────────────────────────────────────────────────┤             │
   │                                                  │             │
   │ HTTP request (sender, callData)                  │             │
   ├──────────────────────────────────────────────────┼────────────►│
   │                                                  │             │
   │ Response (result)                                │             │
   │◄─────────────────────────────────────────────────┼─────────────┤
   │                                                  │             │
   │ callbackFunction(result, extraData)              │             │
   ├─────────────────────────────────────────────────►│             │
   │                                                  │             │
   │ answer                                           │             │
   │◄─────────────────────────────────────────────────┤             │
   │                                                  │             │

Related Issue

Issue #38
PR #40

Changes

  • New feature implementation
  • Bug fix
  • Code refactoring
  • Documentation update
  • Other (please specify)

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

Additional Notes

I'm working on the CCIP Read implementation for the other features such as reading text, multicoin addresses, and contenthash.

The Subdomain resolution PR was used as a starting point for this implementation, which enables this PR to handle both, CCIP-Read and subdomain resolution. They have been decoupled to make the review process easier.

Cheers from the Blockful team 👋🏼

@pikonha
Copy link
Author

pikonha commented Mar 26, 2024

This is now fully working:

  • resolving off-chain addresses
  • getting off-chain text

feedback on the implementation is more than welcome

@mdtanrikulu
Copy link

mdtanrikulu commented Apr 2, 2024

lgtm! works fine with real examples. only small problem is that the following test fails, probably because of some custom error change;

TestResolveEthereum > resolver_test.go:74

Expected nil, but got: &errors.errorString{s:"unregistered name"}

@pikonha
Copy link
Author

pikonha commented Apr 2, 2024

lgtm! works fine with real examples. only small problem is that the following test fails, probably because of some custom error change;

TestResolveEthereum > resolver_test.go:74

Expected nil, but got: &errors.errorString{s:"unregistered name"}

thanks for reviewing it, mate. this test is failing on the main branch as well, I'll fix it with a similar scenario.

@pikonha
Copy link
Author

pikonha commented Apr 5, 2024

lgtm! works fine with real examples. only small problem is that the following test fails, probably because of some custom error change;

TestResolveEthereum > resolver_test.go:74

Expected nil, but got: &errors.errorString{s:"unregistered name"}

the ethereum.eth test is failing because no address is assigned to this domain on mainnet as you can see in here. a similar scenario is being tested on the TestResolveResolverEth test.

image

@pikonha
Copy link
Author

pikonha commented Apr 23, 2024

any updated on this?

@mdtanrikulu
Copy link

any updated on this?

I think in that case this should be handled in the original go-ens repo. It's surely unrelated with the current PR. All good for me.

@dneilroth
Copy link

@pikonha Thanks for putting this together, was looking for an implementation just like this to help with x-chain reads for a project of mine. Anything I can do to help get it merged?

@pikonha
Copy link
Author

pikonha commented May 8, 2024

@dneilroth it's pretty much done from my end tbh, just waiting for the review and approval.

@dneilroth
Copy link

dneilroth commented Jul 8, 2024

@mcdee is it possible to get this one merged? cc @mdtanrikulu

Copy link
Collaborator

@mcdee mcdee left a comment

Choose a reason for hiding this comment

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

Various comments on the PR.

Note that I have not yet looked closely at the changes in resolver.go, I will attempt them at a later date.


// NewUniversalResolver obtains the ENS UniversalResolver.
func NewUniversalResolver(backend bind.ContractBackend) (*UniversalResolver, error) {
address, err := UniversalResolverContractAddress(backend, "mainnet")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will fail for non-mainnet instances. The backend should be queried for the chain ID and a suitable address returned.

Copy link
Author

Choose a reason for hiding this comment

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

I know it is possible to fetch the chainID from the ethClient injected into the Resolver, however, I haven't found a way to fetch it from the bind.ContractBackend interface. any idea on how to solve this without changing the Resolver signature?

universal_resolver.go Outdated Show resolved Hide resolved
resolver.go Outdated
@@ -214,21 +240,11 @@ func resolveName(backend bind.ContractBackend, input string) (common.Address, er
}

func resolveHash(backend bind.ContractBackend, domain string) (common.Address, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the purpose of the changes here. Please don't use shortened names, it doesn't add anything (especially in a world where we have registry, registrar and resolver; r is not a useful letter). I also don't understand why the returned error has been changed.

Copy link
Author

Choose a reason for hiding this comment

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

renaming all the aliased variables to a more meaningful version.

the resolver.go had to be changed to support offchain domains the same way it does for the on-chain ones. the main goal is to maintain the syntax for both types of domain:

r, _ := NewResolver(client, "onchain.eth")
actual, err := r.Text("com.twitter")

r, _ := NewResolver(client, "offchain.eth")
actual, err := r.Text("com.twitter")

the implementation was inspired by the Viem's implementation of the CCIP-Read which relies on the Universal Resolver instead of directly on the Resolver itself.

resolver.go Outdated Show resolved Hide resolved
ccip.go Outdated Show resolved Hide resolved
return nil, err
}

sender := errArgs[0].(common.Address)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a lot of casting and array access here without checks, please add checks to avoid potential panics.

Copy link
Author

Choose a reason for hiding this comment

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

for the returned error to have the OffchainLookup signature (0x556f1830), it must have the following arguments:

  • sender
  • urls
  • calldata
  • callback
  • extraData

so I assume those variables will be on the errArgs variable and so they can be accessed directly by the index. otherwise, it would have failed on the (len(errData) >= 10 && errData[:10] == offchainLookupSignature) condition

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is easy for an error to be returned with the same four bytes but representing a different function (see 4byte.info for an example of clashes), and if done maliciously then it would cause a panic.

Copy link
Author

@pikonha pikonha Jul 10, 2024

Choose a reason for hiding this comment

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

I understand the selector conflict and I did some testing to ensure it would break before an unexpected panic. in every scenario I've tested these conditions would return an error in case of an unexpected error since it wouldn't be present on the Universal Resolver's ABI

var sig [4]byte
copy(sig[:], hexBytes[:4])
abiErr, err := uAbi.ErrorByID(sig)
if err != nil {
  return nil, err
}
var errorData []byte
copy(errorData[:], hexBytes[4:])
errArgs, err := abiErr.Inputs.Unpack(errorData)
if err != nil {
  return nil, err
}

ccip.go Outdated Show resolved Hide resolved
ccip.go Outdated Show resolved Hide resolved
ccip.go Outdated Show resolved Hide resolved
resolver.go Outdated Show resolved Hide resolved
@pikonha
Copy link
Author

pikonha commented Jul 9, 2024

I'll go through the comments ASAP, thanks for reviewing it, mate.

ccip.go Outdated Show resolved Hide resolved
@pikonha pikonha requested a review from mcdee July 10, 2024 13:07
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.

4 participants