Skip to content

[CCIP-7331] Create x-family ConnectChains changeset for 1.7.0 #1338

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
Merged
merged 13 commits into from
Nov 3, 2025
Merged

[CCIP-7331] Create x-family ConnectChains changeset for 1.7.0 #1338

merged 13 commits into from
Nov 3, 2025

Conversation

@kylesmartin
Copy link
Contributor

@kylesmartin kylesmartin commented Oct 29, 2025 •
edited
Loading

@kylesmartin kylesmartin marked this pull request as ready for review October 31, 2025 00:50
@kylesmartin kylesmartin requested review from a team as code owners October 31, 2025 00:50
RensR
RensR reviewed Oct 31, 2025
View reviewed changes
ccv/chains/evm/deployment/v1_7_0/sequences/configure_chain_for_lanes_test.go
var offRamp common.Address
var committeeVerifier common.Address
var Executor common.Address
var routerAddress string
Copy link
Collaborator

@RensR RensR Oct 31, 2025

Choose a reason for hiding this comment

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

quick q: why do we choose string over bytes? Isn't that the most logical, flexible solution?

Copy link
Contributor Author

@kylesmartin kylesmartin Oct 31, 2025

Choose a reason for hiding this comment

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

String is just what datastore returns for addresses, so we just provide that to the sequence and let the sequence do what it wants with it. Unless the address is from a remote chain. In that case we need to convert to bytes at the changeset level.

Copy link
Contributor Author

@kylesmartin kylesmartin Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Subject to change though. I could see it making sense to just convert everything to bytes at the changeset level. But I'd want to change that in all of our changesets in a separate PR.

Copy link
Collaborator

@RensR RensR Oct 31, 2025

Choose a reason for hiding this comment

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

OK so a Core CLD thing, not our usage of it

@kylesmartin kylesmartin enabled auto-merge (squash) November 3, 2025 16:37
@kylesmartin kylesmartin requested a review from RensR November 3, 2025 16:37
RensR
RensR approved these changes Nov 3, 2025
View reviewed changes
Copy link
Collaborator

@RensR RensR left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Has all of this been tested by actually deploying?

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Metric CCIP-7331 develop
Coverage 69.4% 69.1%

@kylesmartin
Copy link
Contributor Author

kylesmartin commented Nov 3, 2025 •
edited
Loading

Has all of this been tested by actually deploying?

In ccv/chains/evm/deployment yes, we test the adapter against a geth node by running this changeset. But we haven't tested on a real network.

@kylesmartin
Copy link
Contributor Author

kylesmartin commented Nov 3, 2025

When we determine which params are more EVM specific, can make a follow up PR to move them to an ExtraConfigs struct

@kylesmartin kylesmartin merged commit 9f70bcf into develop Nov 3, 2025
26 checks passed
@kylesmartin kylesmartin deleted the CCIP-7331 branch November 3, 2025 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RensR RensR RensR approved these changes

@rodrigombsoares rodrigombsoares rodrigombsoares approved these changes

@0xsuryansh 0xsuryansh 0xsuryansh approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kylesmartin @RensR @rodrigombsoares @0xsuryansh