Consolidate CS to enable lane between an existing EVM chain and solana #17607
Conversation
| updateEVMOnRampReport, err := operations.ExecuteOperation(b, operations.NewOperation( | ||
| "updateEVMOnRamp", | ||
| semver.MustParse("1.0.0"), | ||
| "Updates EVM OnRamps with Destination Chain Configs for Solana", | ||
| func(b operations.Bundle, deps Dependencies, input v1_6.UpdateOnRampDestsConfig) ([]mcmslib.TimelockProposal, error) { | ||
| output, err := v1_6.UpdateOnRampsDestsChangeset(deps.Env, input) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return output.MCMSTimelockProposals, nil | ||
| }, | ||
| ), deps, deps.changesetInput.evmOnRampInput) |
There was a problem hiding this comment.
It would be better to declare each operation outside of the ExecuteOperation function for reusability.
| semver.MustParse("1.0.0"), | ||
| "Updates EVM OnRamps with Destination Chain Configs for Solana", | ||
| func(b operations.Bundle, deps Dependencies, input v1_6.UpdateOnRampDestsConfig) ([]mcmslib.TimelockProposal, error) { | ||
| output, err := v1_6.UpdateOnRampsDestsChangeset(deps.Env, input) |
There was a problem hiding this comment.
An Operation should have a single side effect, but this changeset does alot of things, which makes it hard to see where something goes wrong. Consider splitting up this changeset into modular operations, and then create a sequence. Sequences can be nested within each other as well, so this gives you the composability to share sequences
There was a problem hiding this comment.
Discussed offline , creating a follow up ticket to wrap all changesets with corresponding atomic operation instead of directly calling changesets from Operation
deployment/ccip/changeset/crossfamily/v1_6/cs_add_evm_solana_lane.go
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
I think it will become really hard to grok these changesets if ops & sequences & changesets are all defined in a single file. I'm thinking we should create three different packages:
- ccip/operation
- ccip/sequence
- ccip/changeset
And have each entity properly versioned within a subfolder. That way, the sequence package imports operations and the changeset package imports sequences and we are protected from cyclic imports. There is also a clear indication of what imports what. Wdyt?
There was a problem hiding this comment.
No comments outside of what I've left above, which will be addressed in later PR: https://smartcontract-it.atlassian.net/browse/CCIP-5847
Agreed. I will try to give it this structure as part of the follow up work. |
This assumes that EVM chain and solana chain is already added to CCIP Home and the ocr3 configs are set. It's just enabling wiring between existing EVM and Solana chain
There are subsequent work after this PR. Follow up work - Need to change all granular changeset to convert into Operation so that the Operation can be reusable from multiple changesets directly or as part of sequence.