Conversation
There was a problem hiding this comment.
Pull request overview
Updates devenv to use a chain-agnostic “TokenExpansion v2” flow for deploying tokens/pools and configuring cross-chain token transfers, moving token setup responsibilities into per-chain implementations.
Changes:
- Bumps
chainlink-ccip/chainlink-ccip/ccv/chains/evmdependencies to newer commits. - Replaces the old EVM-specific token transfer config builder with chain-agnostic deployment (
DeployTokensAndPools) + configuration (ConfigureAllTokenTransfers) driven by new chain-impl interfaces. - Refactors token-combination modeling (local/remote pools) and updates E2E token-transfer test case generation APIs.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updates chainlink-ccip/ccv/chains/evm pseudo-version. |
| go.sum | Updates module checksums for the bumped dependency. |
| build/devenv/go.mod | Updates chainlink-ccip module versions used by devenv module. |
| build/devenv/go.sum | Updates checksums for bumped chainlink-ccip/deployment deps. |
| build/devenv/tokenconfig/config.go | Removes prior token transfer config builder (EVM-specific). |
| build/devenv/cciptestinterfaces/interface.go | Extends on-chain configuration interface with token/pool deployment + transfer-config provider methods. |
| build/devenv/common/common.go | Introduces chain-agnostic pool capability + token-combination computation, and refactors combination API to local/remote naming. |
| build/devenv/implcommon.go | Adds chain-agnostic helpers to deploy tokens/pools and configure transfers across chains. |
| build/devenv/environment.go | Computes valid combinations from chain capabilities; deploys generic tokens/pools per chain; calls new transfer configuration path. |
| build/devenv/evm/impl.go | Moves generic token/pool deployment out of DeployContractsForSelector; implements new token expansion + transfer config provider methods for EVM. |
| build/devenv/tests/e2e/tcapi/token_transfer/v3.go | Updates test case generation to accept combos and uses local/remote pool refs for naming/lookup. |
| build/devenv/tests/e2e/smoke_test.go | Updates smoke test to pass token combinations into token-transfer test case generators. |
Comments suppressed due to low confidence (1)
build/devenv/tests/e2e/tcapi/token_transfer/v3.go:242
- All17 is described as “2.0.0-only token combinations”, but the generated test names still say “token transfer 1.7.0 …”. This makes test output misleading when debugging failures. Rename the test case strings to match the intended version terminology (either 2.0.0 or 1.7.0, but be consistent with the comment and filtering logic).
// All17 returns test cases for 2.0.0-only token combinations: EOA and mock receiver with default finality (0).
func All17(src, dest cciptestinterfaces.CCIP17, combos []common.TokenCombination) []tcapi.TestCase {
var filtered []common.TokenCombination
for _, tc := range combos {
if common.Is17Combination(tc) {
filtered = append(filtered, tc)
}
}
out := make([]tcapi.TestCase, 0, len(filtered)*2)
for _, combo := range filtered {
qual := combo.LocalPoolAddressRef().Qualifier
out = append(out,
tokenTransferCase(src, dest, combo, 0, true, fmt.Sprintf("token transfer 1.7.0 EOA default finality (%s)", qual)),
tokenTransferCase(src, dest, combo, 0, false, fmt.Sprintf("token transfer 1.7.0 mock receiver default finality (%s)", qual)),
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // up-to-date with the full state so that each TokenExpansion call can | ||
| // resolve previously deployed contracts. | ||
| func DeployTokensAndPools( | ||
| impl cciptestinterfaces.CCIP17Configuration, |
There was a problem hiding this comment.
Small nit: do we want to use the narrower TokenConfigProvider interface instead here, since we're only using those methods on impl?
There was a problem hiding this comment.
Yeah I think that is probably cleaner
| } | ||
|
|
||
| if err := impl.PostTokenDeploy(env, selector, deployedRefs); err != nil { | ||
| return fmt.Errorf("post-token-deploy for selector %d: %w", selector, err) |
There was a problem hiding this comment.
nit/
| return fmt.Errorf("post-token-deploy for selector %d: %w", selector, err) | |
| return fmt.Errorf("PostTokenDeploy for selector %d: %w", selector, err) |
| if cfg.TokenPoolRef.Version != nil { | ||
| v = cfg.TokenPoolRef.Version.String() | ||
| } | ||
| return string(cfg.TokenPoolRef.Type) + "\x00" + v + "\x00" + cfg.TokenPoolRef.Qualifier |
There was a problem hiding this comment.
Q: is the "\x00" necessary or would any symbol (e.g. "+") do to create a unique key?
There was a problem hiding this comment.
The "+" is fine and more intuitive. Switching that out
| func ComputeTokenCombinations( | ||
| capabilities map[uint64][]PoolCapability, | ||
| topology *offchain.EnvironmentTopology, | ||
| ) []TokenCombination { |
There was a problem hiding this comment.
This is a pretty hefty method - should we have some unit tests?
There was a problem hiding this comment.
I actually don't think such unit tests are being run in CI now actually 😬 . Maybe this can be a follow up, we should track it.
There was a problem hiding this comment.
yeah this is only covered insofar as the existing e2e tests call it. Opened https://smartcontract-it.atlassian.net/browse/CCIP-10703 to track
| // OnChainConfigurable defines methods that allows devenv to | ||
| // deploy, configure Chainlink product and connect on-chain part with other chains. | ||
| type OnChainConfigurable interface { | ||
| TokenConfigProvider |
There was a problem hiding this comment.
This ties into a discussion we had in standup - could we have these somehow as "optional" (a good example would be, imagine we just wanted to deploy an env w/out token pools, for pure messaging testing, in the event token pools have not been developed for a particular AltVM)?
Doesn't have to be solved in this PR, but I think its something we should think about for both devenv usage and for tests.
There was a problem hiding this comment.
I do think pulling this out and having it as a standalone interface is the best play. I can give it a try here just to see what it would look like
|
Code coverage report:
|
No description provided.