Skip to content

Conversation

@kylesmartin
Copy link
Contributor

@kylesmartin kylesmartin commented Oct 26, 2025

@kylesmartin kylesmartin marked this pull request as ready for review October 27, 2025 01:54
@kylesmartin kylesmartin requested a review from a team as a code owner October 27, 2025 01:54

for (uint256 i = 0; i < ccvsToQuery.length; ++i) {
ICrossChainVerifierV1(ccvsToQuery[i]).verifyMessage({
address ccvAddress = ccvsToQuery[i];
Copy link
Member

@0xsuryansh 0xsuryansh Oct 27, 2025

Choose a reason for hiding this comment

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

The resolver requires the first 4 bytes of ccvData to encode a version, but the blobs we emit are whatever the underlying verifier returns (e.g. the
current committee verifier returns ""). When we attempt to route verification through the resolver will revert with InvalidCCVDataLength right ?

I think if we are going with this "versioning" approach then CommitVerifier should prepend the version in verifierReturnData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, meant to add this in.

Copy link
Collaborator

@RensR RensR Oct 27, 2025

Choose a reason for hiding this comment

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

The source CCVData is not the same as what is sent on dest. Offchain is responsible for getting the dest data and consuming source. There is no requirement that the version comes from source

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the DD I posed a couple options we can take for the CommitteeVerifier specifically:

  • CommitteeVerifier returns version as part of forwardToVerifier return data. We will use bytes4 to represent the version, as we’ve established precedent here with USDC. This information is then included in CCIPMessageSent.receiptBlobs and subsequently parsed by each verifier node. The aggregator would then need to validate that each verifier node pulled the same version before it can prepend the version to the signatures and write to storage.
  • The aggregator service stores the verifier version assigned to each destination chain and prepends it to the ccvData for a particular VerifierResult.

I prefer the first because option 2 creates duplicated config. I've got offchain sign off on it as well.

@0xsuryansh
Copy link
Member

0xsuryansh commented Oct 27, 2025

Regarding OnRamp._parseExtraArgsWithDefaults
Consider the following scenario..

  • Lane defaults point at ProxyA (just a plain proxy that resolves to VerifierImpl).
  • A user supplies ProxyB which implements the resolver interface and also returns VerifierImpl.

Pre-resolution ProxyA != ProxyB, so both survive the merge. Post-resolution both map to VerifierImpl, so we’ll call forwardToVerifier twice and sum its fee twice. Until we either (a) dedupe after resolution or (b) guarantee that no two proxies/resolvers can collapse to the same implementation..

Thoughts on this edge case ?

cc : @RensR

@kylesmartin
Copy link
Contributor Author

kylesmartin commented Oct 27, 2025

@0xsuryansh good call-out in your top level comment, resolution generally needs to happen as immediately as possible.

@kylesmartin kylesmartin requested a review from a team as a code owner October 29, 2025 15:33
@kylesmartin
Copy link
Contributor Author

kylesmartin commented Oct 29, 2025

Pre-resolution ProxyA != ProxyB, so both survive the merge. Post-resolution both map to VerifierImpl, so we’ll call forwardToVerifier twice and sum its fee twice. Until we either (a) dedupe after resolution or (b) guarantee that no two proxies/resolvers can collapse to the same implementation..

Thinking about it more, I actually don't think this is an issue. Different verifier addresses are effectively different entities. If they resolve to the same implementation, it shouldn't matter. They just happen to be using the same implementation. They should each still get their fee.

@kylesmartin kylesmartin requested a review from RensR November 2, 2025 00:04
@github-actions
Copy link

github-actions bot commented Nov 2, 2025

Metric CCIP-7723 develop
Coverage 69.4% 68.7%

@kylesmartin kylesmartin merged commit a2ec6a1 into develop Nov 3, 2025
26 checks passed
@kylesmartin kylesmartin deleted the CCIP-7723 branch November 3, 2025 12:26
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.

3 participants