refactor: make simulator chain-agnostic with pluggable chain families#373
Draft
refactor: make simulator chain-agnostic with pluggable chain families#373
Conversation
|
| Version | Value |
|---|---|
| Current Fork | v1.17.0 |
| Latest Upstream | null |
Action Required
- Review abigen changes in upstream (only the
accounts/abi/binddirectory matters) - Compare with our fork in
cmd/generate-bindings/bindings/abigen/ - If relevant changes exist, sync them and update
FORK_METADATA.md - If no abigen changes, just update the version in
FORK_METADATA.mdtonull
Files to Review
cmd/generate-bindings/bindings/abigen/bind.gocmd/generate-bindings/bindings/abigen/bindv2.gocmd/generate-bindings/bindings/abigen/template.go
cc @smartcontractkit/bix-framework
…rchitecture Introduce ChainFamily interface and package-level registry enabling future Aptos/Solana chain support. Extract all EVM-specific simulation code into cmd/workflow/simulate/chain/evm/ package with self-registration via init(). - Add ChainFamily interface with 7 methods (Name, ResolveClients, RegisterCapabilities, ExecuteTrigger, ParseTriggerChainSelector, RunHealthCheck, SupportedChains) - Add thread-safe chain family registry with Register/Get/All/Names - Move EVM health checks, trigger parsing, supported chains list, and limited capabilities into chain/evm package - Refactor simulate.go to iterate registered families instead of hardcoding EVM logic - Fix EVM chains not being Start()ed after registration - Remove redundant blank import (named import triggers init()) - Rename test stubs for clarity
Fletch153
force-pushed
the
plex-2751-modularise-chain-families
branch
from
b13b64c to
1d3000b
Compare
- Restore missing debug/error log statements in EVM ResolveClients - Restore ExecuteTrigger nil guards (f.evmChains, EVMChains[selector]) - Restore ui.Success for EVM log discovery in fetchAndConvertLog - Restore comments: Use Opaque, runRPCHealthCheck semantics, regex example - Track experimentalSelectors on EVMFamily, pass to RunHealthCheck - Factory-pattern family registration with logger injection (chain.Build) - Extract ResolveKey + ResolveTriggerData to EVM family - Extract simulator_utils.go contents; WorkflowExecutionTimeout to simulate.go - RegisterCapabilities returns []services.Service for lifecycle parity - getTriggerDataForFamily doc comment chain-agnostic - PROMPT.md + SMOKE_TESTS.md audit artifacts (65 tests)
Covers ResolveKey private-key matrix (valid/invalid/sentinel/empty across broadcast and non-broadcast), LimitedEVMChain delegation for every method, trigger hash validation and log-fetch mock scenarios, supported-chains invariants (unique selectors, valid 20-byte forwarders), and extended health-check labelling for experimental, known, and unknown selectors. 100+ new test cases, zero production code changes.
Fletch153
force-pushed
the
plex-2751-modularise-chain-families
branch
from
dab3e25 to
cbf498d
Compare
Replace EVMTxHash/EVMEventIndex on TriggerParams with a generic FamilyInputs string map so each chain family owns its input keys. Unify EVMChainLimits as a type alias for chain.Limits, removing the redundant interface and the type assertion in RegisterCapabilities. Guard against the typed-nil interface trap when boxing SimulationLimits.
…gistry Chain families now own their CLI flag definitions and input collection, removing all EVM-specific knowledge from simulate.go. Fixes a potential deadlock in registry.Get() by extracting namesLocked() helper, and guards ResolveKey to skip families without configured clients.
Fletch153
force-pushed
the
plex-2751-modularise-chain-families
branch
from
88045cc to
b9a4f58
Compare
Drop EVM prefix from the chain.Limits interface method to better reflect the generic abstraction layer.
fetchAndConvertLog now takes a verbose flag. Interactive mode emits ui.Success messages as before; non-interactive mode stays silent, matching the original behavior.
Rename the chain abstraction from "family" to "type" for clearer domain language. EVM is a chain type, not a chain family. - ChainFamily interface → ChainType - EVMFamily struct → EVMChainType - family.go → chaintype.go (file rename) - FamilyClients/Forwarders/Keys/Inputs → ChainTypeClients/etc - getTriggerDataForFamily → getTriggerDataForChainType - Import alias chaintype → corekeys (conflict avoidance) - All comments, error strings, test names updated
- Remove redundant copyloopvar 'tt := tt' (Go 1.22+)
- Remove unused evmCapabilityBaseStub embed in fullStubCapability
- Convert 'switch { case x == a }' to 'switch x { case a }' (staticcheck QF1002)
- Fix goimports formatting
|
| Version | Value |
|---|---|
| Current Fork | v1.17.0 |
| Latest Upstream | null |
Action Required
- Review abigen changes in upstream (only the
accounts/abi/binddirectory matters) - Compare with our fork in
cmd/generate-bindings/bindings/abigen/ - If relevant changes exist, sync them and update
FORK_METADATA.md - If no abigen changes, just update the version in
FORK_METADATA.mdtonull
Files to Review
cmd/generate-bindings/bindings/abigen/bind.gocmd/generate-bindings/bindings/abigen/bindv2.gocmd/generate-bindings/bindings/abigen/template.go
cc @smartcontractkit/bix-framework
Consolidated review-feedback fixes on top of the chain-family refactor.
Interface cleanup (makes the abstraction live up to its chain-agnostic
name):
- chain.Limits: narrow to ChainWriteReportSizeLimit only; move
ChainWriteGasLimit onto a new evm.EVMChainLimits sub-interface so
EVM-specific accessors do not leak into the generic contract.
- chain.ResolvedChains: new struct returned from ResolveClients, carrying
clients, forwarders, and experimental-selector flags. Replaces the
hidden cross-method state the EVM implementation was keeping on its
struct for RunHealthCheck to read.
- ChainType.ResolveClients and ChainType.RunHealthCheck adopt the new
return/argument shape.
User-visible parity with main:
- Per-family error prefix restored when trigger-data resolution fails,
using the family name (e.g. 'Failed to get evm trigger data: ...').
- 'No {name} chain initialized for selector %d' uses the registry-key
name rather than a manually-uppercased variant.
Small cleanups touched on the way:
- Hoist the 'RPC health check failed:' wrap from evm.RunRPCHealthCheck
to the simulate.go caller so a future second chain family cannot
produce doubled headers.
- Remove dead ManualTriggers.Close (no callers before or after this PR).
- Rename the EVMChainLimits test stub to stubEVMLimits to stop it
colliding by one letter with the production EVMChainLimits alias.
…chain-families # Conflicts: # cmd/workflow/simulate/simulate.go
Merge from main pulled in the disableEngineLimits function but the call site in WorkflowSettingsCfgFn was dropped during conflict resolution, leaving the function unused. Re-add the `--limits none` branch so engine limits are actually disabled when requested.
Distinguish manual trigger capabilities (cron, HTTP) from chain-related capabilities by using an explicit name at the call sites.
HasSelector guard upstream and per-selector check inside ResolveTriggerData make the map-level check unreachable; remove to reduce indirection.
- Append manualTriggerCaps cron/HTTP services immediately after creation so the shutdown order matches main (cron/HTTP before chain-type services). - Revert "Failed to create cron/HTTP trigger capabilities" back to "Failed to create trigger capabilities" to match main.
- Rename ManualTriggers receiver t -> m to match main. - Use corekeys.EVM directly for chain.Register; import in chain/evm package. - Push string -> common.Address forwarder conversion into NewEVMChainCapabilities so it happens only for chains with clients; drop intermediate map in RegisterCapabilities. - Remove Inputs.ChainTypeForwarders; read forwarders via ChainTypeResolved[name].Forwarders for a single source of truth.
Remove 29 net-new tests plus delegation test file (~1100 lines) that verified tautologies, library behaviour, compile-time invariants, or one-line pass-throughs. Merge the 'more' and 'validation' split files back into their single-file counterparts. Deletions: - limited_capabilities_delegation_test.go (whole file): verified stub.calls[method]==1 on pass-through methods. WriteReport gating logic (size+gas) remains covered by limited_capabilities_test.go. - chaintype_test.go: literal-constant getters, crypto-library echo, duplicate ResolveKey cases already in the table, one-line delegation, compile-time interface check. - health_more_test.go: nil/empty-map tautologies and exact duplicates of tests in health_test.go. - supported_chains_test.go: pin tests (NotEmpty, EthereumMainnet, Sepolia with hardcoded forwarder), tautologies (Lowercased, Are20Bytes redundant with hex regex), compile-time struct check. - trigger_validation_test.go: regex coverage redundant with parser table; misleading-name edge cases dropped; 5 genuinely novel cases (zero, max uint64, negative, unicode digits, tab) merged into the parser table. - registry_test.go: map round-trip smokes and happy-path nil acceptance; all real branches covered by duplicate-panic, sort, defensive-copy, error-with-names, CLI-branching tests. Consolidations: - Merged health_more_test.go into health_test.go (11 tests total). - Merged trigger_validation_test.go into trigger_test.go (8 tests plus the expanded parser table). Kept tests exercise real branches and data invariants: ResolveKey hex/sentinel logic, RegisterCapabilities type assertions, ExecuteTrigger error paths, CollectCLIInputs filters, experimental/unknown health labels, unique selectors, valid hex forwarders, chain-selector resolution, httptest RPC round-trips in GetEVMTriggerLogFromValues, and registry duplicate-panic / sort / defensive-copy / CLI branching.
The test claimed to cover the triggerData type-assertion branch in ExecuteTrigger but its setup (empty EVMChains map) always tripped the earlier nil-chain check first. The errorContainsAny hedge masked the issue by accepting either error, making the test effectively a duplicate of TestEVMChainType_ExecuteTrigger_UnknownSelector.
Only call site was wrapping an empty function in TestGetEVMTriggerLogFromValues_Success; that dead line was dropped during the trigger test consolidation, leaving the helper unreferenced.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The simulator was EVM-only by construction. Adding a second chain family meant editing dozens of call sites across
simulate.go.What changed
EVM is now one implementation of a chain-family interface instead of the only shape the simulator understands. EVM-specific code — clients, capabilities, health checks, trigger parsing, CLI flags, limits lives under its own package. The simulator iterates registered families rather than calling EVM directly.
No user-facing behaviour changes. No CLI, config, or limit changes.
Out of scope / follow-ups