Conversation
🦋 Changeset detectedLatest commit: b30b3af The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds a simple RPC provider implementation for the Stellar blockchain network within the chainlink-deployments-framework. It introduces the foundational types and structures needed to support Stellar chain operations.
Changes:
- Introduces
ChainandChainMetadatatypes for Stellar network representation - Implements
RPCChainProviderwith configuration and initialization logic - Adds validation for required Stellar network parameters (NetworkPassphrase, FriendbotURL, SorobanRPCURL)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| chain/stellar/stellar_chain.go | Defines the basic Chain struct and ChainMetadata type alias for Stellar networks |
| chain/stellar/provider/rpc_provider.go | Implements RPCChainProvider with configuration validation and chain initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (p *RPCChainProvider) Initialize(_ context.Context) (chain.BlockChain, error) { | ||
| if p.chain != nil { | ||
| return p.chain, nil // already initialized | ||
| } | ||
|
|
||
| if err := p.config.validate(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| p.chain = &stellar.Chain{ | ||
| ChainMetadata: stellar.ChainMetadata{Selector: p.selector}, | ||
| } | ||
|
|
||
| return *p.chain, nil | ||
| } |
There was a problem hiding this comment.
Initialize() performs a check-then-set on p.chain without synchronization. If Initialize() can be called concurrently, this can lead to data races and double-initialization. Consider guarding initialization with sync.Once or a mutex (and similarly protect reads in BlockChain() if needed).
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| c, err := stellarprov.NewRPCChainProvider(selector, | ||
| stellarprov.RPCChainProviderConfig{ | ||
| NetworkPassphrase: network.Metadata.(map[string]any)["network_passphrase"].(string), | ||
| FriendbotURL: network.RPCs[1].HTTPURL, | ||
| SorobanRPCURL: network.RPCs[0].HTTPURL, | ||
| }, | ||
| ).Initialize(ctx) |
There was a problem hiding this comment.
network.Metadata.(map[string]any)["network_passphrase"].(string)andnetwork.RPCs[0/1]can panic at runtime if metadata is missing/typed unexpectedly or if RPC endpoints are not present/ordered as assumed. Prefer explicit extraction withokchecks (and returning a descriptive error) and avoid hard-coded indices by selecting RPC URLs by role/name (or at least validatinglen(network.RPCs)` before indexing).
| if cfg.Stellar.DeployerKey != "" { | ||
| loaders[chainsel.FamilyStellar] = newChainLoaderStellar(networks, cfg) | ||
| } else { | ||
| lggr.Info("Skipping Stellar chains, no private key found in secrets") | ||
| } |
There was a problem hiding this comment.
chainLoaderStellar/RPCChainProviderdon’t usecfg.Stellar.DeployerKey`, but chain loading is gated on it being set. This will unexpectedly skip Stellar chain initialization even when the RPC config is present/valid. Either (a) remove this gate for Stellar and gate on the actual required inputs (passphrase + RPC URLs), or (b) plumb the deployer key into the Stellar provider/loader if it’s required for intended operations.
| if cfg.Stellar.DeployerKey != "" { | |
| loaders[chainsel.FamilyStellar] = newChainLoaderStellar(networks, cfg) | |
| } else { | |
| lggr.Info("Skipping Stellar chains, no private key found in secrets") | |
| } | |
| loaders[chainsel.FamilyStellar] = newChainLoaderStellar(networks, cfg) |
| // StellarConfig is the configuration for the Stellar Chains. | ||
| // | ||
| // WARNING: This data type contains sensitive fields and should not be logged or set in file | ||
| // configuration. | ||
| type StellarConfig struct { | ||
| DeployerKey string `mapstructure:"deployer_key" yaml:"deployer_key"` // Secret: The private key of the deployer account. | ||
| } |
There was a problem hiding this comment.
The comment says the deployer key “should not be ... set in file configuration”, but the struct is explicitly tagged for YAML file config (yaml:"deployer_key") and this PR adds it to engine/cld/config/env/testdata/config.yml. Consider clarifying the warning (e.g., “do not commit real secrets; prefer env vars for production”) or adjusting test/config patterns to align with the guidance.
| func (c RPCChainProviderConfig) validate() error { | ||
| if c.NetworkPassphrase == "" { | ||
| return errors.New("network passphrase is required") | ||
| } | ||
| if c.FriendbotURL == "" { | ||
| return errors.New("friendbot URL is required") | ||
| } | ||
| if c.SorobanRPCURL == "" { | ||
| return errors.New("soroban RPC URL is required") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
This new config validation (and the provider initialization/caching behavior) doesn’t appear to have unit coverage. Adding tests for each missing-field case and for Initialize() returning the same chain on subsequent calls will help prevent regressions as Stellar support expands.
2 participants
No description provided.