Sish/v2 pre upgrade - #824
Open
stackman27 wants to merge 3 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Canton deployment adapter defaults ahead of a v2 upgrade, adjusting token price assumptions and lane configuration defaults (including different defaults depending on whether the remote chain is EVM).
Changes:
- Update default Amulet/USD spot price used when no FamilyExtras price is provided.
- Introduce EVM-vs-non-EVM branching for Canton fee-quoter and remote-chain default configs.
- Harden default inbound CCV selection by forcing a fail-closed “invalid-ccv” default.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| deployment/adapters/configure_lanes_token_prices.go | Updates the default native token USD price used when per-remote prices aren’t provided. |
| deployment/adapters/chain_family_adapter.go | Adjusts Canton lane defaults (fee quoter + remote defaults), adds EVM-remote detection, and changes default inbound CCV resolution behavior. |
Comments suppressed due to low confidence (4)
deployment/adapters/chain_family_adapter.go:496
- The non‑EVM default in GetDefaultRemoteChainConfig no longer sets ExecutorDestChainConfig. This leaves ExecutorDestChainConfig.Enabled at the zero value (false), which will disable executor dest-chain config updates for non‑EVM remotes.
return ccipadapters.RemoteChainDefaults{
AllowTrafficFrom: true,
BaseExecutionGasCost: 50_000,
TokenReceiverAllowed: true,
MessageNetworkFeeUSDCents: 10,
deployment/adapters/chain_family_adapter.go:73
- The unexported helper name defaultCantonFeeQuoterDestChainConfig is extremely close to DefaultCantonFeeQuoterDestChainConfig and doesn't convey that it's the non‑EVM/legacy defaults. Renaming it would reduce the chance of calling the wrong function.
func defaultCantonFeeQuoterDestChainConfig() lanes.FeeQuoterDestChainConfig {
deployment/adapters/chain_family_adapter.go:443
- If you rename the non‑EVM defaults helper, update this call site accordingly to avoid accidentally selecting the wrong defaults.
if !isEVMRemote(remoteChainSelector) {
defaults = defaultCantonFeeQuoterDestChainConfig()
}
deployment/adapters/chain_family_adapter.go:555
- resolveDefaultInboundCCVs hard-codes the qualifier "invalid-ccv" and will now fail lane configuration unless that specific committee verifier ref exists in the datastore. At minimum, consider making the qualifier a named constant and including chainSelector/qualifier in the error to make operational failures easier to diagnose.
// Hardening: default inbound CCV is always invalid-ccv (fail-closed). Real traffic
// should rely on lane-mandated CCVs; the default must not implicitly trust a live CCV.
ref, err := findContractRef(
ds,
chainSelector,
datastore.ContractType(committeeverifierop.ContractType),
committeeverifierop.Version,
"invalid-ccv",
)
if err != nil {
return nil, fmt.Errorf("resolve default invalid-ccv inbound qualifier: %w", err)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| defaultInboundCCVs, err := resolveDefaultInboundCCVs(ds, input.ChainSelector) | ||
| if err != nil { | ||
| return out, fmt.Errorf("resolve default inbound ccvs for remote chain %d: %w", remoteSelector, err) |
Comment on lines
52
to
+70
| func DefaultCantonFeeQuoterDestChainConfig() lanes.FeeQuoterDestChainConfig { | ||
| return lanes.FeeQuoterDestChainConfig{ | ||
| OverrideExistingConfig: false, | ||
| IsEnabled: true, | ||
| MaxDataBytes: 30_000, | ||
| MaxPerMsgGasLimit: 3_000_000, | ||
| DestGasOverhead: 300_000, | ||
| DestGasPerPayloadByteBase: 16, | ||
| MaxDataBytes: 32_000, | ||
| MaxPerMsgGasLimit: 15_000_000, | ||
| DestGasOverhead: 0, | ||
| DestGasPerPayloadByteBase: 20, | ||
| ChainFamilySelector: binary.BigEndian.Uint32(CantonFamilySelector[:]), | ||
| DefaultTokenFeeUSDCents: 0, | ||
| DefaultTokenDestGasOverhead: 90_000, | ||
| DefaultTxGasLimit: 200_000, | ||
| NetworkFeeUSDCents: 50, | ||
| V1Params: nil, | ||
| V2Params: &lanes.FeeQuoterV2Params{ | ||
| LinkFeeMultiplierPercent: 90, | ||
| USDPerUnitGas: big.NewInt(38), | ||
| }, | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
deployment/adapters/chain_family_adapter.go:220
remoteCfg.DefaultInboundCCVsis no longer used here; the code always setsDefaultInboundCCVsto the invalid sentinel. If this is intentional (fail-closed), add an inline comment so operators don’t assumeDefaultInboundCCVsin topology/config will take effect.
defaultInboundCCVs := []datastore.AddressRef{invalidCCVAddressRef(input.ChainSelector)}
deployment/adapters/chain_family_adapter.go:493
- The non-EVM
RemoteChainDefaultsno longer setsExecutorDestChainConfig, which previously defaulted to enabled. IfExecutorDestChainConfig.Enableddefaults to false/zero, this will unintentionally disable the executor config for non-EVM remotes.
return ccipadapters.RemoteChainDefaults{
AllowTrafficFrom: true,
BaseExecutionGasCost: 50_000,
TokenReceiverAllowed: true,
MessageNetworkFeeUSDCents: 10,
deployment/adapters/chain_family_adapter.go:553
invalidCCVRawInstanceAddressis intended to be a valid raw instance address, but this helper bypasses validation via a plain type conversion. Since downstream code parses the label viaRawInstanceAddressFromString, validate once here (and fail loudly) so a future constant edit can’t cause a confusing runtime error mid-sequence.
func invalidCCVAddressRef(chainSelector uint64) datastore.AddressRef {
raw := contracts.RawInstanceAddress(invalidCCVRawInstanceAddress)
return datastore.AddressRef{
Address: raw.InstanceAddress().Hex(),
ChainSelector: chainSelector,
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.