Conversation
tt-cll
requested review from
AmrMohamedRezk,
KodeyThomas,
agusaldasoro,
asoliman92,
Copilot,
makramkd,
matYang and
winder
There was a problem hiding this comment.
Pull request overview
Adds first-class support for in-place Solana program upgrades to the unified deployments framework, including a Solana adapter upgrade sequence and a local artifact build pipeline used by integration tests.
Changes:
- Introduces an
UpgradeContractschangeset with anUpgraderRegistryfor chain families that support in-place upgrades. - Adds an optional
ArtifactPreparerhook to deployment/upgrade flows and implements it for the Solana adapter. - Adds Solana local-build + upgrade integration tests plus supporting build/upgrade utilities.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-tests/deployment/upgrade_chain_contracts_test.go | New integration tests for local-build deploy + in-place upgrade flow |
| deployment/deploy/upgrade.go | New UpgradeContracts changeset + upgrader registry plumbing |
| deployment/deploy/product.go | Adds optional ArtifactPreparer interface for deployer adapters |
| deployment/deploy/contracts.go | Invokes PrepareArtifacts during deploy when supported |
| chains/solana/deployment/v1_6_0/sequences/upgrade_chain_contracts.go | New Solana upgrade sequence using BPF Loader Upgradeable + optional MCMS batching |
| chains/solana/deployment/v1_6_0/sequences/adapter.go | Registers Solana upgrader + implements PrepareArtifacts |
| chains/solana/deployment/utils/upgrade.go | New Solana upgrade instruction helpers (set authority/extend/upgrade/close, buffer deploy) |
| chains/solana/deployment/utils/build.go | New Solana artifact builder (download or local clone/key replacement/anchor build) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const ( | ||
| repoURL = "https://github.com/smartcontractkit/chainlink-ccip.git" | ||
| cloneDir = "./temp-repo" | ||
| anchorDir = "chains/solana/contracts" | ||
| deployDir = "chains/solana/contracts/target/deploy" | ||
| ) |
There was a problem hiding this comment.
cloneDir is a fixed relative path ("./temp-repo"). Because the integration tests run with t.Parallel() and BuildSolana may be invoked concurrently, parallel runs can clobber each other (e.g., CleanGitDir/git reset/key replacement) and produce flaky or incorrect builds. Make the clone directory unique per build (e.g., os.MkdirTemp + pass via config) or add a process-wide lock to serialize local builds.
| func copyFile(src, dst string) error { | ||
| data, err := os.ReadFile(src) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(dst, data, 0600) | ||
| } |
There was a problem hiding this comment.
copyFile reads the entire artifact into memory and writes it with mode 0600. The restrictive permissions can break scenarios where the Solana container/validator runs under a different user and needs read access, and it’s inconsistent with the download path (which uses os.Create). Prefer streaming copy (io.Copy) and use 0644 (or preserve source file mode) for .so/JSON artifacts.
| output, err := RunCommand("solana-keygen", []string{"grind", "--starts-with", prefix + ":1"}, "./") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate vanity key for %s: %w", program, err) | ||
| } | ||
|
|
||
| scanner := bufio.NewScanner(strings.NewReader(output)) | ||
| var jsonFilePath string | ||
| for scanner.Scan() { | ||
| if matches := jsonFilePattern.FindStringSubmatch(scanner.Text()); len(matches) > 1 { | ||
| jsonFilePath = matches[1] | ||
| break | ||
| } | ||
| } | ||
| if jsonFilePath == "" { | ||
| return fmt.Errorf("failed to parse vanity key output for %s", program) | ||
| } | ||
|
|
||
| absPath, err := filepath.Abs(jsonFilePath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get absolute path for vanity key: %w", err) | ||
| } | ||
|
|
||
| fileName := filepath.Base(absPath) | ||
| keys[program] = strings.TrimSuffix(fileName, ".json") | ||
|
|
||
| destination := filepath.Join(cloneDir, deployDir, programTypeToDeployName(program)+"-keypair.json") | ||
| if err := os.Rename(absPath, destination); err != nil { | ||
| return fmt.Errorf("failed to move vanity key from %s to %s: %w", absPath, destination, err) | ||
| } |
There was a problem hiding this comment.
solana-keygen grind is executed with workDir "./" and then the generated JSON is moved into the cloned repo. When multiple builds/tests run concurrently, keygen output files in the shared working directory can collide. Run keygen in a per-build temp directory (or inside the clone dir) to avoid cross-test interference.
| semver.MustParse("1.6.0"), | ||
| "Upgrades deployed Solana CCIP programs in place via BPF Loader Upgradeable", | ||
| func(b cldf_ops.Bundle, chains cldf_chain.BlockChains, input deployapi.ContractUpgradeConfigWithAddress) (sequences.OnChainOutput, error) { | ||
| chain := chains.SolanaChains()[input.ChainSelector] |
There was a problem hiding this comment.
The chain is fetched via chains.SolanaChains()[input.ChainSelector] without checking existence. If the selector is missing from the environment this will use a zero-value chain and likely panic later (nil client/key). Add an ok check and return a clear error when the chain selector is not present.
| chain := chains.SolanaChains()[input.ChainSelector] | |
| chain, ok := chains.SolanaChains()[input.ChainSelector] | |
| if !ok { | |
| return sequences.OnChainOutput{}, fmt.Errorf("solana chain selector %d not found in environment", input.ChainSelector) | |
| } |
| timelockSignerPDA := utils.GetTimelockSignerPDA( | ||
| input.ExistingAddresses, | ||
| input.ChainSelector, | ||
| "cll", |
There was a problem hiding this comment.
The timelock qualifier is hardcoded as "cll" here. Elsewhere the codebase uses common_utils.CLLQualifier (value "CLLCCIP") when looking up the timelock seed/address, so this will fail to find the correct timelock signer PDA and will break the MCMS-wrapped upgrade path. Use the shared qualifier constant instead of a literal.
| "cll", | |
| common_utils.CLLQualifier, |
| // Execute directly if deployer owns it, otherwise wrap in MCMS proposal | ||
| if upgradeAuthority != timelockSignerPDA { | ||
| if err := chain.Confirm([]solana.Instruction{upgradeIxn, closeIxn}); err != nil { | ||
| return nil, fmt.Errorf("failed to confirm upgrade: %w", err) | ||
| } | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Direct execution is gated on upgradeAuthority != timelockSignerPDA, but chain.Confirm will only be signed by chain.DeployerKey. If the upgrade authority is neither the deployer key nor the timelock signer PDA, this will reliably fail at runtime. Consider mirroring the established pattern used elsewhere (execute directly only when upgradeAuthority == chain.DeployerKey.PublicKey(), otherwise emit an MCMS batch op or return an explicit error).
| // Execute directly if deployer owns it, otherwise wrap in MCMS proposal | |
| if upgradeAuthority != timelockSignerPDA { | |
| if err := chain.Confirm([]solana.Instruction{upgradeIxn, closeIxn}); err != nil { | |
| return nil, fmt.Errorf("failed to confirm upgrade: %w", err) | |
| } | |
| return nil, nil | |
| } | |
| // Execute directly only when the deployer is the upgrade authority; otherwise | |
| // require the timelock signer PDA so the upgrade can be proposed through MCMS. | |
| if upgradeAuthority == chain.DeployerKey.PublicKey() { | |
| if err := chain.Confirm([]solana.Instruction{upgradeIxn, closeIxn}); err != nil { | |
| return nil, fmt.Errorf("failed to confirm upgrade: %w", err) | |
| } | |
| return nil, nil | |
| } | |
| if upgradeAuthority != timelockSignerPDA { | |
| return nil, fmt.Errorf( | |
| "unsupported upgrade authority %s: expected deployer %s for direct execution or timelock signer PDA %s for MCMS", | |
| upgradeAuthority.String(), | |
| chain.DeployerKey.PublicKey().String(), | |
| timelockSignerPDA.String(), | |
| ) | |
| } |
| func TestDeployWithLocalBuild(t *testing.T) { | ||
| t.Parallel() | ||
| skipInCI(t) | ||
|
|
||
| solSelector := chain_selectors.SOLANA_MAINNET.Selector | ||
| programsPath := t.TempDir() | ||
|
|
There was a problem hiding this comment.
These tests run with t.Parallel() but the local-build pipeline uses a shared clone directory (currently hardcoded as ./temp-repo). Running TestDeployWithLocalBuild and TestUpgradeChainContracts concurrently can race and clobber the working tree / generated keys. Either remove t.Parallel() for tests that invoke local builds, or ensure the build pipeline uses a per-test temp clone dir.
| for _, addr := range postUpgradeAddresses { | ||
| switch cldf.ContractType(addr.Type) { | ||
| case routerops.ContractType: | ||
| routerCount++ | ||
| require.Equal(t, routerAddr, addr.Address, "Router should be upgraded in place") | ||
| case fqops.ContractType: | ||
| fqCount++ | ||
| require.Equal(t, fqAddr, addr.Address, "FeeQuoter should be upgraded in place") | ||
| case offrampops.ContractType: | ||
| offRampCount++ | ||
| } | ||
| } | ||
| require.Equal(t, 1, routerCount, "Should have exactly one Router") | ||
| require.Equal(t, 1, fqCount, "Should have exactly one FeeQuoter") | ||
| require.GreaterOrEqual(t, offRampCount, 1, "Should have at least one OffRamp") | ||
| } |
There was a problem hiding this comment.
The post-upgrade verification increments offRampCount but never asserts that the OffRamp address remains the same as offRampAddr, unlike Router/FeeQuoter. This can allow regressions where OffRamp is redeployed or upgraded at a different address without failing the test. Add an address equality assertion for the OffRamp case (and ideally assert exactly one OffRamp, if that’s the invariant).
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
No description provided.