CCIP Read ISM

[AlexBHarley]

Description

CCIP Read ISM implementation,

• Contracts
• Relayer
• Example HTTP backend

Drive-by changes

Are there any minor or drive-by changes also included?

Related issues

• Fixes #[2211]

Backward compatibility

Are these changes backward compatible?

Yes, relayers will skip messages they can't handle.

Are there any infrastructure implications, e.g. changes that would prohibit deploying older commits using this infra tooling?

Yes, relayers won't be able to submit messages that have a CCIP Read ISM.

Testing

What kind of testing have these changes undergone?

• Manual
• Unit Tests

General approach

• Get a basic CCIP Read implementation working
• Build a mock service that serves CCIP Read compatible data
• Update the Relayer to support CCIP Read ISMs

[AlexBHarley]

After chatting with @nambrot last week I've taken a slightly different approach to the original one, interested to hear what you guys think. An implementation of a CcipReadIsm can be found here.

I have a couple open questions I'm not sure about,

• Is there any value in the AbstractCcipReadIsm living in the monorepo? I feel it's OK for consumers to implement this logic themselves, but on the other hand it's nice to have the OffchainLookup error well-typed for use in the relayer
• Do we actually want to call this CcipRead? While it's quite close to matching the spec, it doesn't exactly and that could confuse client side implementations. e.g., relayer queries for information on the destination chain, but the callback function (Mailbox.process) is not only a different contract but a different chain. Simply calling it OffchainRead could be better
• If we do keep with CCIP Read, using the custom error approach, I'm unsure how to better parse these. This section of the code is what I'm referring to. Right now I do a Regex match to pull the error out which feels messy

[nambrot]

• Is there any value in the AbstractCcipReadIsm living in the monorepo? I feel it's OK for consumers to implement this logic themselves, but on the other hand it's nice to have the OffchainLookup error well-typed for use in the relayer

For sure, IMO its critical to have it in the monorepo as its the reference for the agent implementation as well. Like i commented, it should also include instructions for the spec of this ISM, i.e. how do I override it, what interface does my offchain service have to comply with, etc. Like another comment of mine, the consumer should be forced to implement the extra data of the revert message though.

• Do we actually want to call this CcipRead? While it's quite close to matching the spec, it doesn't exactly and that could confuse client side implementations. e.g., relayer queries for information on the destination chain, but the callback function (Mailbox.process) is not only a different contract but a different chain. Simply calling it OffchainRead could be better

Hmm, i'm not sure that I'm following. The relayer would query the ISM on the destination chain for the OffchainRead revert message, to then call the Mailbox.process on that very same chain no? (You could even have the ISM call Mailbox.process if you wanted to, to be fully compliant)

• If we do keep with CCIP Read, using the custom error approach, I'm unsure how to better parse these. This section of the code is what I'm referring to. Right now I do a Regex match to pull the error out which feels messy

You should be able to look at this https://github.com/ensdomains/ethers-ccip-read/blob/main/src/middleware.rs#L243C1-L244C1

[nambrot]

Also putting a review here for the ChainlinkISM since there is no PR to look at:

• It would be nice to see a diff of the original OffchainAggregator and yours. I have terrible memory, but remind me why the Aggregator even has to be modified at all? I thought we were good with making storage writes in the verify function?
• Are you using HyperlaneConnectionClient just to have a storage variable for the mailbox? In which case it might be preferable to not have that dependency.
• Why is verify permission to onlyMailbox?
• Are etherscan keys required? IMO etherscan lets you do 1 request every 5 seconds which should maybe be fine?
• It would be nice to link to a deployment somewhere (for both the service, and the feed on an EVM chain, and show it replicating via etherscan view calls)

[nambrot]

@yorhodes called out that an alternate path for this implementation is to put the CCIP read middleware at the provider level and then have estimate gas to mailbox.process just automatically populate the metadata. However, that is probably more involved as there is no out of the box middleware that can be used for this purpose (yet). @AlexBHarley 's approach is sufficient as it enables ISM development without relayer changes, and is technically possible to be spec compliant, and we can always introduce the alternate way in the future

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (637172b) 63.56% compared to head (c4447fe) 63.56%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2398   +/-   ##
=======================================
  Coverage   63.56%   63.56%           
=======================================
  Files          89       89           
  Lines        1356     1356           
  Branches      182      182           
=======================================
  Hits          862      862           
  Misses        487      487           
  Partials        7        7           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[yorhodes]

big unlock for new ISMs 🚀