-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Ccip memory test refactor #20377
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
base: develop
Are you sure you want to change the base?
Ccip memory test refactor #20377
Conversation
7c2a091 to
236c72e
Compare
272c255 to
11dd4e9
Compare
4adb949 to
30e5c91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors CCIP memory test setup and configuration to use new environment initialization patterns. The changes standardize how test environments are created and configured across the codebase.
Key changes:
- Replaces
WithChainIDswithWithEVMChainsBySelectorsto use chain selectors instead of chain IDs - Updates environment creation to use
environment.New()instead ofmemory.NewMemoryEnvironment() - Adds new utility functions for loading Solana and TON program artifacts
- Standardizes node registration with new
RegisterNodes()helper function
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-tests/smoke/ccip/ccip_topologies_test.go | Simplified chain selection by directly assigning selectors and removed sorting logic |
| integration-tests/smoke/ccip/ccip_migration_to_v_1_6_test.go | Updated to use chain selectors instead of chain IDs |
| integration-tests/smoke/ccip/ccip_messaging_test.go | Updated to use chain selectors instead of chain IDs |
| deployment/internal/tontestutils/sha.go | Added new utility to extract TON contract SHA from go.mod |
| deployment/internal/soltestutils/preload.go | Added LoadCCIPPrograms function to load CCIP program artifacts |
| deployment/common/changeset/solana/mcms/mcms.go | Fixed bug where wrong program ID was used in waitForProgramDeployment calls |
| deployment/ccip/changeset/v1_6/cs_translate_onramp_to_feequoter_test.go | Removed unused variable and simplified chain selector handling |
| deployment/ccip/changeset/v1_6/cs_home_chain_test.go | Replaced memory environment with new environment.New() pattern |
| deployment/ccip/changeset/v1_6/cs_deploy_chain_test.go | Replaced memory environment with new environment.New() pattern |
| deployment/ccip/changeset/v1_6/cs_chain_contracts_test.go | Updated to use chain selectors instead of chain IDs |
| deployment/ccip/changeset/v1_6/cs_add_new_chain_e2e_test.go | Updated to use chain selectors instead of chain IDs |
| deployment/ccip/changeset/v1_6/accept_ownership_test.go | Removed redundant state loading |
| deployment/ccip/changeset/v1_5/e2e_test.go | Updated to use chain selectors instead of chain IDs |
| deployment/ccip/changeset/testhelpers/test_environment.go | Major refactor to use new environment loader and added RegisterNodes helper |
| deployment/ccip/changeset/solana_v0_1_1/transfer_ccip_to_mcms_with_timelock_test.go | Updated to use new environment pattern with Solana container |
| deployment/ccip/changeset/solana_v0_1_1/cs_deploy_chain_test.go | Updated to use new environment pattern with Solana container |
| deployment/ccip/changeset/aptos/cs_add_token_pool_test.go | Commented out quarantine.Flaky call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| func getModFilePath() (string, error) { | ||
| _, currentFile, _, _ := runtime.Caller(0) |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error return value from runtime.Caller(0) is ignored. If runtime.Caller fails (returns false in the 4th return value), the function should handle this error case instead of proceeding with potentially invalid values.
| _, currentFile, _, _ := runtime.Caller(0) | |
| _, currentFile, _, ok := runtime.Caller(0) | |
| if !ok { | |
| return "", errors.New("runtime.Caller failed to retrieve current file") | |
| } |
| client, ok := env.Offchain.(*simjd.MemoryJobDistributor) | ||
| require.True(t, ok) | ||
|
|
||
| nodeIDs := make([]string, 4) |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The slice nodeIDs is initialized with a fixed size of 4 on line 1710, but the function parameter numNodes is used in the loop on line 1711. This creates a mismatch when numNodes != 4, potentially causing an index out of bounds error. The slice should be initialized with make([]string, numNodes) instead.
| nodeIDs := make([]string, 4) | |
| nodeIDs := make([]string, numNodes) |
30e5c91 to
9727d91
Compare
9727d91 to
42b863d
Compare
| return nil, fmt.Errorf("access controller program not ready: %w", err) | ||
| return nil, fmt.Errorf("failed to deploy timelock program: %w", err) | ||
| } | ||
| err = waitForProgramDeployment(e.GetContext(), chain.Client, chainState.TimelockProgram, 30*time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed waiting on incorrect program
| return nil, fmt.Errorf("access controller program not ready: %w", err) | ||
| return nil, fmt.Errorf("failed to deploy mcm program: %w", err) | ||
| } | ||
| err = waitForProgramDeployment(e.GetContext(), chain.Client, chainState.McmProgram, 30*time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed waiting on incorrect program
|
|
||
| allChains := tenv.BlockChains.ListChainSelectors( | ||
| cldf_chain.WithFamily(chain_selectors.FamilyEVM), | ||
| cldf_chain.WithChainSelectorsExclusion([]uint64{chain_selectors.GETH_TESTNET.Selector}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded call to List since we already have the selectors
| testhelpers.WithNumOfChains(3), | ||
| testhelpers.WithChainIDs([]uint64{chainselectors.GETH_TESTNET.EvmChainID})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chaanged to directly specify the chains instead of using a combination of N chains and direct selectors
| t.Parallel() | ||
| e, _ := testhelpers.NewMemoryEnvironment(t) | ||
| state, err := stateview.LoadOnchainState(e.Env) | ||
| _, err := stateview.LoadOnchainState(e.Env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state var was unused
| // testhelpers.WithNumOfChains(3), | ||
| testhelpers.WithEVMChainsBySelectors([]uint64{ | ||
| chainselectors.GETH_TESTNET.Selector, | ||
| chainselectors.TEST_90000001.Selector, | ||
| chainselectors.TEST_90000002.Selector, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chaanged to directly specify the chains instead of using a combination of N chains and direct selectors
9ed54f5 to
938f93b
Compare
938f93b to
c9b759c
Compare
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
| TonChains int // only used in memory mode, for docker mode, this is determined by the integration-test config toml input | ||
| ChainIDs []uint64 // only used in memory mode, for docker mode, this is determined by the integration-test config toml input | ||
| NumOfUsersPerChain int // only used in memory mode, for docker mode, this is determined by the integration-test config toml input | ||
| EVMChainsBySelectors []uint64 // only used in memory mode, for docker mode, this is determined by the integration-test config toml input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to make it clearer what it is doing
| ChainIDs []uint64 // only used in memory mode, for docker mode, this is determined by the integration-test config toml input | ||
| NumOfUsersPerChain int // only used in memory mode, for docker mode, this is determined by the integration-test config toml input | ||
| EVMChainsBySelectors []uint64 // only used in memory mode, for docker mode, this is determined by the integration-test config toml input | ||
| NumOfUsersPerChain uint // only used in memory mode, for docker mode, this is determined by the integration-test config toml input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type change because this can not be less than 0 and it matches the expected value to the EVMContainer config
| if tc.Chains > len(tc.ChainIDs) { | ||
| additionalChains := cldf_chain.NewBlockChainsFromSlice( | ||
| memory.NewMemoryChainsEVM(t, tc.Chains-len(tc.ChainIDs), tc.NumOfUsersPerChain), | ||
| ) | ||
|
|
||
| maps.Copy(chains, additionalChains.EVMChains()) | ||
|
|
||
| additionalUsers := usersMap(t, chains) | ||
| maps.Copy(users, additionalUsers) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic has been intentionally removed in favour of the caller explicitly providing the chain selectors they want to use.
The existing logic was confusing to use
| nodeIDs := make([]string, 0, len(nodes)) | ||
| for id, node := range nodes { | ||
| require.NoError(t, node.App.Start(ctx)) | ||
| t.Cleanup(func() { | ||
| require.NoError(t, node.App.Stop()) | ||
| }) | ||
| nodeIDs = append(nodeIDs, id) | ||
| } | ||
| m.nodes = nodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This had to be moved here to remove usage of NewMemoryEnvironmentFromChainsNodes
c9b759c to
e9661fa
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Test Results: SUI Chain Funding Regression
Affected failures:
What Broke
The refactoring of the SUI chain initialization in 'deployment/ccip/changeset/testhelpers/test_environment.go' introduced a regression that prevents the SUI chain from properly processing or reporting cross-chain commits, leading to a timeout.
Proposed Fixes
Add a funding mechanism for SUI chains in the test environment to ensure they have sufficient funds for operations, addressing a timeout issue related to commit report processing.
var (
tonDeployerFundAmount = tlb.MustFromTON("1000")
+ suiDeployerFundAmount = sui_deployment.MustNewSuiCoin("1000")
)
utils.FundWallets(t, tonChain.Client, []*address.Address{tonChain.WalletAddress}, []tlb.Coins{tonDeployerFundAmount})
}
}
+
+ // Sui chains must be funded if they exist
+ if len(env.BlockChains.SuiChains()) > 0 {
+ for _, suiChain := range env.BlockChains.SuiChains() {
+ sui_deployment.FundWallets(t, suiChain.Client, []*sui.Address{suiChain.WalletAddress}, []sui_deployment.Coins{suiDeployerFundAmount})
+ }
+ }
m.DeployedEnv = DeployedEnv{
Env: *env,Autofix Options
You can apply the proposed fixes directly to your branch. Try the following:
- Comment
/trunk stack-fix fnBEbPouto generate a stacked PR with the proposed fixes. - Use MCP in your IDE to fix the issue. Try
Help me fix CI failures from fnBEbPouto get started.
Tip
Get Better Results: This CI job is not uploading test reports. Adding structured test reports enables more precise, test-level analysis with better root cause identification and more targeted fix recommendations.
👉🏻 Learn how to upload test results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Test Results: Unrelated Failure
Affected failures:
- Workflow Run: Integration Tests
What Broke
Solana programs (Access Controller, MCM, Timelock) are not deploying or becoming ready within the expected timeframe in the new test environment setup. This issue is now correctly reported due to improved error handling. The PR refactors the test environment setup, migrating from a custom in-memory environment to a more generic framework. The improved error handling for Solana program deployments in deployment/common/changeset/solana/mcms/mcms.go likely exposed a pre-existing or newly introduced issue with Solana program readiness in the refactored environment.
Autofix Options
You can use our MCP server to get AI assistance with debugging and fixing these failures.
- Use MCP in your IDE to debug the issue. Try
Help me fix CI failures from tSVopD0Dto get started.




wip