Repository navigation
Add Stellar support to cre workflow simulate - #612
karen-stepanyan wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Stellar services are not started, and the core broadcast/dry-run registration path lacks coverage.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds Stellar read/write support to workflow simulation, including dry-run and broadcast modes.
Changes:
- Registers Stellar clients, mock forwarders, keys, health checks, and limits.
- Migrates capability registries and updates telemetry compatibility.
- Upgrades Go, Chainlink, Stellar, and related dependencies.
| File | Description |
|---|---|
.github/workflows/pull-request-main.yml |
Updates golangci-lint. |
cmd/workflow/simulate/capabilities.go |
Migrates capability registry types. |
cmd/workflow/simulate/chain/evm/capabilities.go |
Migrates EVM registry types. |
cmd/workflow/simulate/chain/evm/chaintype_test.go |
Updates EVM registry tests. |
cmd/workflow/simulate/chain/solana/capabilities.go |
Migrates Solana registry types. |
cmd/workflow/simulate/chain/stellar/chaintype.go |
Implements Stellar simulation support. |
cmd/workflow/simulate/chain/stellar/chaintype_test.go |
Tests Stellar configuration and keys. |
cmd/workflow/simulate/chain/stellar/health.go |
Adds Stellar RPC health checks. |
cmd/workflow/simulate/chain/stellar/limited_capabilities.go |
Enforces Stellar report limits. |
cmd/workflow/simulate/chain/stellar/supported_chains.go |
Defines default Stellar networks and forwarders. |
cmd/workflow/simulate/chain/types.go |
Migrates shared registry configuration. |
cmd/workflow/simulate/simulate.go |
Registers Stellar and the new registry implementation. |
cmd/workflow/simulate/telemetry_writer.go |
Supports OTel byte-slice bodies. |
go.mod |
Upgrades Go and dependencies. |
go.sum |
Refreshes dependency checksums. |
internal/settings/settings.go |
Adds Stellar private-key settings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Transmitter: transmitter, | ||
| DryRun: !cfg.Broadcast, |
| ct.chains[sel] = dc | ||
| out = append(out, dc) |
anirudhwarrier
left a comment
anirudhwarrier
left a comment
There was a problem hiding this comment.
Requesting changes for a resource leak in StellarChainType.RegisterCapabilities: started fake chains aren't closed on error paths. Details inline.
| if err != nil { | ||
| return nil, fmt.Errorf("stellar: selector %d: %w", sel, err) | ||
| } | ||
| if err := dc.Start(ctx); err != nil { |
There was a problem hiding this comment.
Chains started in earlier iterations leak if a later iteration fails. Every error return here (and the resolvePassphrase / NewFakeStellarChain errors above) returns nil, err, so out is discarded. simulate.go only keeps the services when there's no error, so simulatorCleanup never calls Close on the chains that were already started.
Example: testnet plus an experimental selector. Testnet starts first (depending on map order), then GetNetwork for the experimental selector fails. The testnet chain's goroutines and RPC polling keep running.
Suggestion: on any error, close everything already in out before returning, e.g. a deferred cleanup guarded by a named err:
defer func() {
if err != nil {
for _, s := range out {
_ = s.Close()
}
}
}()There was a problem hiding this comment.
thanks, good catch. There are no background goroutines or polling, Start and Close both are no-ops and just return nil now, so nothing actually leaked, but this no longer relies on that.
| return nil, fmt.Errorf("stellar: failed to start chain for selector %d: %w", sel, err) | ||
| } | ||
| server := stellarserver.NewClientServer(NewLimitedStellarChain(dc, lim)) | ||
| if err := cfg.Registry.Add(ctx, server); err != nil { |
There was a problem hiding this comment.
dc.Start runs before cfg.Registry.Add. If Add fails (e.g. a duplicate capability ID), dc is already started but never appended to out and never closed.
Solana builds and registers everything first and starts afterwards (solana/chaintype.go:253). Could we do the same here: register first, then start, or at least dc.Close() when Add fails? Combined with the cleanup suggested on L186, that covers every error path.
There was a problem hiding this comment.
same as comment above, applied fix to not rely on no-ops
Adds Stellar to
cre workflow simulate, built onFakeStellarChainfromchainlink-stellar/capabilities.stellarchain type: reads (ReadContract,GetLatestLedger) go to the configured Stellar RPC; writes go through the CRE mock forwarder, which skips DON signature checks.simulateTransaction, nothing submitted).--broadcast: signed and submitted; requiresCRE_STELLAR_PRIVATE_KEY(S… seed of a funded account).Default mock forwarders
CC5OVNNU32FQAWHDWIGFUWYRLMPVERESM52S4IYC3MMETI2YDX3N674XCBBNYOUOFBQZDJFN4GVFODGOPDY6DRRWZBWGXFGJQZBRFXSFAQPX3L6TOverride per target with an
experimental-chainsentry (chain-type: stellar).Dependency bump
chainlink/v2+chainlink/deployment→edb5ba66(includes thestellarencoder in the fake consensus), Go 1.27.1.capreg.Registry); telemetry writer accepts the OTel v0.22BYTESLICEbody type, otherwise simulations silently dropped all workflow logs.