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

Add changeset to add bidirectional lanes #17015

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Conversation

kylesmartin
Copy link
Contributor

@kylesmartin kylesmartin commented Mar 28, 2025

@kylesmartin kylesmartin marked this pull request as ready for review March 28, 2025 18:40
@kylesmartin kylesmartin requested review from a team as code owners March 28, 2025 18:40
Copy link
Contributor

Flakeguard Summary

Ran new or updated tests between develop and 2c7392e (CCIP-5502).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

1 Results
Name Pass Ratio Panicked? Timed Out? Race? Runs Successes Failures Skips Package Package Panicked? Avg Duration Code Owners
TestAddBidirectionalLanesChangeset 0% true false false 3 0 3 0 github.com/smartcontractkit/chainlink/deployment/ccip/changeset/v1_6 true 0s @smartcontractkit/ccip-tooling, @smartcontractkit/ccip-offchain, @smartcontractkit/core, @smartcontractkit/deployment-automation, @smartcontractkit/cld-team

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

UpdateRouterRampsConfig UpdateRouterRampsConfig
}

func (c AddBidirectionalLanesConfig) buildConfigs() addBidirectionalLanesChangesetConfigs {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add an unit test for this func

onRampUpdatesByChain[laneDef.Source.Selector] = make(map[uint64]OnRampDestinationUpdate)
}
onRampUpdatesByChain[laneDef.Source.Selector][laneDef.Dest.Selector] = OnRampDestinationUpdate{
IsEnabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion : Can you take this as input? It might help disabling lane with same changeset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will add an IsDisabled option though because enabling a lane should be default action.

Comment on lines +112 to +119
feeQuoterPriceUpdatesOnSource := feeQuoterPriceUpdatesByChain[laneDef.Source.Selector]
if feeQuoterPriceUpdatesOnSource.GasPrices == nil {
feeQuoterPriceUpdatesOnSource.GasPrices = make(map[uint64]*big.Int)
}
feeQuoterPriceUpdatesOnSource.GasPrices[laneDef.Dest.Selector] = laneDef.Dest.GasPrice
feeQuoterPriceUpdatesByChain[laneDef.Source.Selector] = feeQuoterPriceUpdatesOnSource
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

are you missing the token prices here?

Copy link
Contributor Author

@kylesmartin kylesmartin Mar 31, 2025

Choose a reason for hiding this comment

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

Token prices are not tied to a dest chain selector. They are tied to token addresses on the source chain. That is to say that when you add a lane, you shouldn't be adding a new token price. These should already exist.

If you need to add a new token price to a chain, that is a separate process and should not go through "add lane".

return nil
}

func addBidirectionalLanesLogic(e deployment.Environment, c AddBidirectionalLanesConfig) (deployment.ChangesetOutput, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: log statements after every success operation. Ignore if there are already log statements available in individual changesets

Copy link
Contributor

Flakeguard Summary

Ran new or updated tests between develop and 110da4a (CCIP-5502).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

2 Results
Name Pass Ratio Panicked? Timed Out? Race? Runs Successes Failures Skips Package Package Panicked? Avg Duration Code Owners
TestAddAndPromoteCandidatesForNewChain 0% true false false 1 0 1 0 github.com/smartcontractkit/chainlink/deployment/ccip/changeset/v1_6 true 0s @smartcontractkit/ccip-tooling, @smartcontractkit/ccip-offchain, @smartcontractkit/core, @smartcontractkit/deployment-automation, @smartcontractkit/cld-team
TestConnectNewChain 0% true false false 1 0 1 0 github.com/smartcontractkit/chainlink/deployment/ccip/changeset/v1_6 true 0s @smartcontractkit/ccip-tooling, @smartcontractkit/ccip-offchain, @smartcontractkit/core, @smartcontractkit/deployment-automation, @smartcontractkit/cld-team

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

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.

2 participants