Conversation
0xsuryansh
requested review from
AmrMohamedRezk,
KodeyThomas,
agusaldasoro,
Copilot,
makramkd,
matYang,
ogtownsend and
winder
|
👋 0xsuryansh, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
0xsuryansh
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a read-only changeset and basic tests to validate the post-migration on-chain state for the USDC CLD rollout across EVM chains (token admin registry active pool, USDCTokenPoolProxy routing/config, remote lane config, and Ethereum home-chain liquidity migration invariants).
Changes:
- Introduces
ValidateUSDCCLDRolloutchangeset with config schema, precondition validation, and on-chain reads/assertions. - Adds home-chain liquidity migration checks (hybrid lock-release mode, lockbox auth, locked/balance derived expectations).
- Adds unit tests covering key
VerifyPreconditionsfailures and anApplymissing-chain failure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| ccv/chains/evm/deployment/v1_7_0/changesets/validate_usdc_cld_rollout.go | New post-validation changeset implementing config validation + on-chain assertions for USDC CLD rollout. |
| ccv/chains/evm/deployment/v1_7_0/changesets/validate_usdc_cld_rollout_test.go | New tests for precondition validation and basic apply error behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| verifyHexAddress(v, fmt.Sprintf("chains[%d].TokenAdminRegistry", chainSelector), chainCfg.TokenAdminRegistry, true) | ||
| verifyHexAddress(v, fmt.Sprintf("chains[%d].ExpectedTokenPool", chainSelector), chainCfg.ExpectedTokenPool, false) | ||
| verifyHexAddress(v, fmt.Sprintf("chains[%d].USDCTokenPoolProxy", chainSelector), chainCfg.USDCTokenPoolProxy, false) | ||
| verifyProxyPools(v, fmt.Sprintf("chains[%d].ExpectedProxyPools", chainSelector), chainCfg.ExpectedProxyPools) |
There was a problem hiding this comment.
VerifyPreconditions allows RemoteChains / ExpectedProxyPools to be configured even when USDCTokenPoolProxy is empty. In that case Apply will silently skip all proxy/remote validations (it returns early when USDCTokenPoolProxy == ""). Consider enforcing that USDCTokenPoolProxy is required whenever RemoteChains is non-empty or ExpectedProxyPools is non-nil (or alternatively error if those fields are set while proxy is empty).
| verifyProxyPools(v, fmt.Sprintf("chains[%d].ExpectedProxyPools", chainSelector), chainCfg.ExpectedProxyPools) | |
| verifyProxyPools(v, fmt.Sprintf("chains[%d].ExpectedProxyPools", chainSelector), chainCfg.ExpectedProxyPools) | |
| if chainCfg.USDCTokenPoolProxy == "" { | |
| if chainCfg.ExpectedProxyPools != nil { | |
| v.addf("chains[%d].USDCTokenPoolProxy: required when ExpectedProxyPools is configured", chainSelector) | |
| } | |
| if len(chainCfg.RemoteChains) > 0 { | |
| v.addf("chains[%d].USDCTokenPoolProxy: required when RemoteChains is configured", chainSelector) | |
| } | |
| } |
| ) | ||
| } | ||
|
|
||
| if cfg.USDCTokenPoolProxy == "" { |
There was a problem hiding this comment.
validateChainState returns immediately when USDCTokenPoolProxy is empty, which can lead to false-green runs when the caller provided ExpectedProxyPools and/or RemoteChains expectations but forgot to set the proxy address. Instead of returning silently, consider adding a validation error when proxy-dependent checks are configured (or move this enforcement into VerifyPreconditions).
| if cfg.USDCTokenPoolProxy == "" { | |
| if cfg.USDCTokenPoolProxy == "" { | |
| if len(cfg.ExpectedProxyPools) > 0 || len(cfg.RemoteChains) > 0 { | |
| v.addf( | |
| "chains[%d]: USDCTokenPoolProxy is required when ExpectedProxyPools or RemoteChains are configured", | |
| chain.Selector, | |
| ) | |
| } |
| cfg ValidateUSDCCLDRolloutRemoteChainConfig, | ||
| v *validationCollector, | ||
| ) { | ||
| if cfg.ExpectedMechanism == "" { |
There was a problem hiding this comment.
validateRemoteChainState bails out when ExpectedMechanism is empty, which also skips validating ExpectedRemoteToken / ExpectedRemotePools even if those fields are set in config. Either (a) validate those fields independently of ExpectedMechanism, or (b) add a precondition that ExpectedRemoteToken/ExpectedRemotePools must be empty unless ExpectedMechanism is set, to avoid silently ignoring configured expectations.
| if cfg.ExpectedMechanism == "" { | |
| if cfg.ExpectedMechanism == "" { | |
| if cfg.ExpectedRemoteToken != (common.Address{}) { | |
| v.addf("chains[%d].remoteChains[%d]: expectedRemoteToken requires expectedMechanism to be set", chainSelector, remoteSelector) | |
| } | |
| if len(cfg.ExpectedRemotePools) > 0 { | |
| v.addf("chains[%d].remoteChains[%d]: expectedRemotePools requires expectedMechanism to be set", chainSelector, remoteSelector) | |
| } |
| v.addf("chains[%d].remoteChains[%d]: failed to read proxy mechanism: %v", chainSelector, remoteSelector, err) | ||
| return | ||
| } | ||
|
|
||
| expectedMechanism, err := mechanismUint8(cfg.ExpectedMechanism) | ||
| if err != nil { | ||
| v.addf("chains[%d].remoteChains[%d]: %v", chainSelector, remoteSelector, err) | ||
| return | ||
| } | ||
| if mechanism != expectedMechanism { | ||
| v.addf("chains[%d].remoteChains[%d]: mechanism mismatch, expected %s got %d", chainSelector, remoteSelector, cfg.ExpectedMechanism, mechanism) | ||
| } | ||
|
|
||
| if supported, err := proxy.IsSupportedChain(callOpts, remoteSelector); err != nil { | ||
| v.addf("chains[%d].remoteChains[%d]: failed to check proxy supported chain: %v", chainSelector, remoteSelector, err) | ||
| } else if !supported { | ||
| v.addf("chains[%d].remoteChains[%d]: proxy does not report the chain as supported", chainSelector, remoteSelector) | ||
| } | ||
|
|
||
| backingPool := backingPoolForMechanism(pools, cfg.ExpectedMechanism) | ||
| if backingPool == (common.Address{}) { | ||
| v.addf("chains[%d].remoteChains[%d]: proxy backing pool for mechanism %s is not configured", chainSelector, remoteSelector, cfg.ExpectedMechanism) |
There was a problem hiding this comment.
Error paths in remote-chain validation use chains[%d].remoteChains[%d] (lowercase remoteChains), while VerifyPreconditions uses chains[%d].RemoteChains[...]. Similarly, home-chain liquidity errors use homeChainLiquidity.checks[...] in some places. Aligning these field names/casing across all validation errors will make failures easier to map back to config keys.
| v.addf("chains[%d].remoteChains[%d]: failed to read proxy mechanism: %v", chainSelector, remoteSelector, err) | |
| return | |
| } | |
| expectedMechanism, err := mechanismUint8(cfg.ExpectedMechanism) | |
| if err != nil { | |
| v.addf("chains[%d].remoteChains[%d]: %v", chainSelector, remoteSelector, err) | |
| return | |
| } | |
| if mechanism != expectedMechanism { | |
| v.addf("chains[%d].remoteChains[%d]: mechanism mismatch, expected %s got %d", chainSelector, remoteSelector, cfg.ExpectedMechanism, mechanism) | |
| } | |
| if supported, err := proxy.IsSupportedChain(callOpts, remoteSelector); err != nil { | |
| v.addf("chains[%d].remoteChains[%d]: failed to check proxy supported chain: %v", chainSelector, remoteSelector, err) | |
| } else if !supported { | |
| v.addf("chains[%d].remoteChains[%d]: proxy does not report the chain as supported", chainSelector, remoteSelector) | |
| } | |
| backingPool := backingPoolForMechanism(pools, cfg.ExpectedMechanism) | |
| if backingPool == (common.Address{}) { | |
| v.addf("chains[%d].remoteChains[%d]: proxy backing pool for mechanism %s is not configured", chainSelector, remoteSelector, cfg.ExpectedMechanism) | |
| v.addf("chains[%d].RemoteChains[%d]: failed to read proxy mechanism: %v", chainSelector, remoteSelector, err) | |
| return | |
| } | |
| expectedMechanism, err := mechanismUint8(cfg.ExpectedMechanism) | |
| if err != nil { | |
| v.addf("chains[%d].RemoteChains[%d]: %v", chainSelector, remoteSelector, err) | |
| return | |
| } | |
| if mechanism != expectedMechanism { | |
| v.addf("chains[%d].RemoteChains[%d]: mechanism mismatch, expected %s got %d", chainSelector, remoteSelector, cfg.ExpectedMechanism, mechanism) | |
| } | |
| if supported, err := proxy.IsSupportedChain(callOpts, remoteSelector); err != nil { | |
| v.addf("chains[%d].RemoteChains[%d]: failed to check proxy supported chain: %v", chainSelector, remoteSelector, err) | |
| } else if !supported { | |
| v.addf("chains[%d].RemoteChains[%d]: proxy does not report the chain as supported", chainSelector, remoteSelector) | |
| } | |
| backingPool := backingPoolForMechanism(pools, cfg.ExpectedMechanism) | |
| if backingPool == (common.Address{}) { | |
| v.addf("chains[%d].RemoteChains[%d]: proxy backing pool for mechanism %s is not configured", chainSelector, remoteSelector, cfg.ExpectedMechanism) |
| } | ||
|
|
||
| for chainSelector, chainCfg := range cfg.Chains { | ||
| verifyEVMChainSelector(v, chainSelector, "chains") |
There was a problem hiding this comment.
verifyEVMChainSelector is called with field name "chains" for the top-level chain selector (no per-chain context). If multiple chains are provided, an error like chains: invalid chain selector ... is ambiguous. Consider passing a selector-specific field name (e.g., chains[<selector>]) for clearer error messages.
| verifyEVMChainSelector(v, chainSelector, "chains") | |
| verifyEVMChainSelector(v, chainSelector, fmt.Sprintf("chains[%d]", chainSelector)) |
| func deriveExpectedHybridLocked(check ValidateUSDCCLDRolloutLiquidityLaneCheck) (*big.Int, error) { | ||
| if check.ExpectedHybridLocked != "" { | ||
| return parseBigInt(check.ExpectedHybridLocked) | ||
| } | ||
| if check.PreHybridLocked == "" || check.ExpectedWithdrawAmount == "" { | ||
| return nil, nil | ||
| } | ||
| pre, err := parseBigInt(check.PreHybridLocked) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid PreHybridLocked %q: %w", check.PreHybridLocked, err) | ||
| } | ||
| withdraw, err := parseBigInt(check.ExpectedWithdrawAmount) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid ExpectedWithdrawAmount %q: %w", check.ExpectedWithdrawAmount, err) | ||
| } | ||
| if pre.Cmp(withdraw) < 0 { | ||
| return nil, fmt.Errorf("pre hybrid locked amount %s is smaller than expected withdraw amount %s", pre.String(), withdraw.String()) | ||
| } | ||
| return new(big.Int).Sub(pre, withdraw), nil | ||
| } | ||
|
|
||
| func deriveExpectedLockBoxBalance(check ValidateUSDCCLDRolloutLiquidityLaneCheck) (*big.Int, error) { | ||
| if check.ExpectedLockBoxBalance != "" { | ||
| return parseBigInt(check.ExpectedLockBoxBalance) | ||
| } | ||
| if check.PreLockBoxBalance == "" || check.ExpectedWithdrawAmount == "" { | ||
| return nil, nil | ||
| } | ||
| pre, err := parseBigInt(check.PreLockBoxBalance) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid PreLockBoxBalance %q: %w", check.PreLockBoxBalance, err) | ||
| } | ||
| withdraw, err := parseBigInt(check.ExpectedWithdrawAmount) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid ExpectedWithdrawAmount %q: %w", check.ExpectedWithdrawAmount, err) | ||
| } | ||
| return new(big.Int).Add(pre, withdraw), nil | ||
| } |
There was a problem hiding this comment.
The arithmetic helpers deriveExpectedHybridLocked / deriveExpectedLockBoxBalance introduce non-trivial validation logic (explicit expected value vs derived value, and erroring when withdraw > pre). Adding focused unit tests for these functions (including boundary cases like equal amounts and withdraw greater than pre) would reduce the risk of incorrect post-validation results.
No description provided.