chore: use remote chains selectors#941
Conversation
🦋 Changeset detectedLatest commit: ade774d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
gustavogama-cll
force-pushed
the
ggama/feat/use-remote-chain-selectors
branch
from
f525925 to
39adb31
Compare
gustavogama-cll
changed the title
gustavogama-cll
force-pushed
the
ggama/feat/use-remote-chain-selectors
branch
from
39adb31 to
2cd7d57
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates parts of CLDF chain loading and EVM RPC providers to resolve chain metadata via github.com/smartcontractkit/chain-selectors/remote, aiming to support newly-added EVM chains without bumping the chain-selectors dependency.
Changes:
- Switch chain-family resolution in
engine/cld/chainsto use remote chain details. - Update EVM RPC provider(s) to derive chain ID from remote chain details.
- Update
rpcclient.MultiClientto use remote chain details (and update call sites/tests to pass context).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| engine/cld/chains/chains.go | Uses remote chain details to determine chain family before selecting loaders. |
| chain/evm/provider/rpc_provider.go | Uses remote chain details to derive ChainID; passes ctx into MultiClient. |
| chain/evm/provider/zksync_rpc_provider.go | Updates MultiClient call signature to include ctx. |
| chain/evm/provider/ctf_geth_provider.go | Updates MultiClient call signature to include ctx. |
| chain/evm/provider/ctf_anvil_provider.go | Updates MultiClient call signature to include ctx. |
| chain/evm/provider/rpcclient/multiclient.go | Switches selector->chain lookup to remote; adds ctx param to constructor; uses ctx for health checks. |
| chain/evm/provider/rpcclient/multiclient_test.go | Updates tests for new MultiClient signature. |
| chain/evm/provider/rpc_provider_test.go | Updates expected error substring for remote chain-details lookup failure. |
| .changeset/easy-waves-hunt.md | Adds a release note / version bump entry for the change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NewMultiClient creates a new MultiClient with failover capabilities. | ||
| func NewMultiClient(lggr logger.Logger, rpcsCfg RPCConfig, opts ...func(client *MultiClient)) (*MultiClient, error) { | ||
| func NewMultiClient( | ||
| ctx context.Context, lggr logger.Logger, rpcsCfg RPCConfig, opts ...func(client *MultiClient), | ||
| ) (*MultiClient, error) { |
There was a problem hiding this comment.
NewMultiClient is an exported function in a non-internal package; adding a new leading context.Context parameter is a breaking API change for downstream callers. Consider preserving backwards compatibility by keeping the old signature as a wrapper (calling the new one with context.Background()/TODO) or by introducing a new function name, and align the version bump accordingly.
There was a problem hiding this comment.
this is indeed a breaking change, but the only external usages I found are:
- https://github.com/smartcontractkit/chainlink/blob/df2100a728542fd6083b273b990ee7df552b9726/deployment/environment/devenv/chain.go#L178
- https://github.com/smartcontractkit/por-workflow-hub-test/blob/6d373ea2280967ef189f83982a46774608948ce1/src/internal/environment/common.go#L220
Maybe if we proactively patch them we can get away with it?
There was a problem hiding this comment.
sounds good to me!
gustavogama-cll
force-pushed
the
ggama/feat/use-remote-chain-selectors
branch
from
2cd7d57 to
5c6e5bf
Compare
gustavogama-cll
force-pushed
the
ggama/feat/use-remote-chain-selectors
branch
from
5c6e5bf to
ade774d
Compare
|
graham-chainlink
left a comment
graham-chainlink
left a comment
There was a problem hiding this comment.
Looks good to me, running this in CLD should be no issue as the remote github url is public.
i guess one of the side effect is that running the test will hit the github url everytime test is run, i wonder if we should introduce a flag to only use local in test to avoid ddosing github? thoughts?
ecPablo
left a comment
ecPablo
left a comment
There was a problem hiding this comment.
LGTM, just following what @graham-chainlink said on rate limits for unit tests. I believe they have 5,000 requests per hour per IP address. But this should not be a problem as long as we have the chain selectors used in unit tests existing locally, which I think is the case right now.
yeah true, since it checks for local first then only fetch remote if it doesnt exist. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| chainDetails, err := chainsel.GetChainDetailsBySelector(ctx, p.selector) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get chain id from selector: %w", err) | ||
| return nil, fmt.Errorf("failed to get chain details for selector: %w", err) |
There was a problem hiding this comment.
This error omits the selector value (unlike other providers in this PR) which makes failures harder to diagnose. Include p.selector in the message for consistency and easier debugging (e.g., "... for selector %d").
| return nil, fmt.Errorf("failed to get chain details for selector: %w", err) | |
| return nil, fmt.Errorf("failed to get chain details for selector %d: %w", p.selector, err) |
This pull request updates the
/chain/evm/providerand/engine/cld/chainspackages to use remote chain selectors from thegithub.com/smartcontractkit/chain-selectors/remotepackage. It will enable CLDF clients to load new EVM chains without the need to update the "chain-selectors" library version.