feat(config): remove chain.Chain from zetaclientd config #3137
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the ZetaChain node, encompassing new features, improvements, refactoring, and bug fixes. Key enhancements include the ability to whitelist SPL tokens on Solana, improved build reproducibility, and integration of SPL deposits. Notable refactoring efforts involve the removal of the HSM signer from the zetaclient and adjustments to the configuration management. The testing framework has been enhanced with new tests and improved coverage. Overall, these changes aim to streamline functionality and enhance the robustness of the ZetaChain node. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
swift1337
changed the title
swift1337
force-pushed
the
feat/drop-chain-from-config
branch
from
5708456
to
b667e17
Compare
swift1337
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
zetaclient/config/config_chain.go (1)
Line range hint
69-71: Consider making the endpoint configurableThe hardcoded endpoint "http://eth:8545" might not be suitable for all environments. Consider making it configurable through environment variables or configuration files.
Example implementation:
func evmChainsConfigs() map[int64]EVMConfig { + endpoint := getEnvOrDefault("GOERLI_LOCALNET_ENDPOINT", "http://eth:8545") return map[int64]EVMConfig{ chains.GoerliLocalnet.ChainId: { - Endpoint: "http://eth:8545", + Endpoint: endpoint, RPCAlertLatency: 60, }, } }cmd/zetaclientd/start.go (4)
Line range hint
58-62: Enhance configuration loading with validation and detailed error handlingConsider adding config validation and more descriptive error messages to help diagnose configuration issues.
// Load Config file given path cfg, err := config.Load(globalOpts.ZetacoreHome) if err != nil { - return err + return errors.Wrapf(err, "failed to load config from path %s", globalOpts.ZetacoreHome) +} + +// Validate essential config fields +if err := cfg.Validate(); err != nil { + return errors.Wrap(err, "invalid configuration") }
Line range hint
1-2: Track TODOs in issue systemThe TODO comments reference issues #3119 and #3112 for revamping. Consider removing the TODO comments once the issues are being tracked.
Line range hint
275-284: Add timeout and context handling for TSS keygen waitThe current implementation could potentially wait indefinitely. Consider adding a timeout and proper context handling.
// Wait for TSS keygen to be successful before proceeding +const keygenTimeout = 5 * time.Minute ticker := time.NewTicker(time.Second * 1) +defer ticker.Stop() +timeout := time.After(keygenTimeout) + for range ticker.C { + select { + case <-ctx.Done(): + return errors.Wrap(ctx.Err(), "context cancelled while waiting for TSS keygen") + case <-timeout: + return errors.New("timed out waiting for TSS keygen") + default: keyGen := appContext.GetKeygen() if keyGen.Status != observerTypes.KeygenStatus_KeyGenSuccess { startLogger.Info().Msgf("Waiting for TSS Keygen to be a success, current status %s", keyGen.Status) continue } break + } }
Line range hint
332-334: Return error when no chains are configuredInstead of just logging an error, the function should return an error when no chains are configured to prevent startup with invalid configuration.
if len(appContext.ListChainIDs()) == 0 { - startLogger.Error().Interface("config", cfg).Msgf("No chains in updated config") + return errors.New("no chains found in updated config") }zetaclient/orchestrator/orchestrator_test.go (2)
Line range hint
401-604: Enhance rate limiter test coverage.The rate limiter tests are comprehensive but could benefit from additional edge cases:
- Maximum rate limit values
- Zero rate limit values
- Rate limit window edge cases
tests := []struct { + { + name: "should handle maximum rate limit values", + rateLimiterFlags: &crosschaintypes.RateLimiterFlags{ + Enabled: true, + Window: math.MaxInt64, + Rate: sdk.NewUint(math.MaxUint64), + }, + response: &crosschaintypes.QueryRateLimiterInputResponse{ + Height: math.MaxInt64, + }, + fail: false, + }, + { + name: "should handle zero rate limit values", + rateLimiterFlags: &crosschaintypes.RateLimiterFlags{ + Enabled: true, + Window: 0, + Rate: sdk.NewUint(0), + }, + fail: true, + },
Hardcoded localhost endpoints in test file are safe, but production code needs attention
The test configuration using "localhost" is appropriate for testing purposes. However, the verification revealed several production code files containing hardcoded localhost references that require attention:
rpc/websockets.go: Contains an explicit FIXME comment acknowledging localhost shouldn't be hardcodedserver/flags/flags.go: Uses localhost as default value for node flagcontrib/rpctest/main.go: Contains hardcoded localhost endpoints for RPC testingThe test configuration in the original review is safe as it's properly isolated in a test file. However, the production code contains instances that should be addressed for better security and configurability.
🔗 Analysis chain
Line range hint
605-618: Verify endpoint configuration security.The hardcoded "localhost" endpoints in test configurations should be verified to ensure they don't accidentally leak into production configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hardcoded localhost endpoints in non-test files rg -g '!*_test.go' 'localhost' --type goLength of output: 540
Script:
#!/bin/bash # Check if these localhost references are properly guarded by environment checks or test flags rg -g '!*_test.go' -B 3 -A 3 'localhost' --type go # Look for any configuration loading mechanisms to verify if these values can be overridden fd -e go -E '*_test.go' -x ast-grep --pattern 'func $_ { $$$ viper.$_( $$$ ) $$$ }'Length of output: 2501
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
changelog.md(1 hunks)cmd/zetaclientd/start.go(1 hunks)zetaclient/chains/evm/observer/observer_test.go(0 hunks)zetaclient/config/config_chain.go(1 hunks)zetaclient/config/types.go(1 hunks)zetaclient/config/types_test.go(0 hunks)zetaclient/orchestrator/bootstap_test.go(0 hunks)zetaclient/orchestrator/orchestrator_test.go(1 hunks)
💤 Files with no reviewable changes (3)
- zetaclient/chains/evm/observer/observer_test.go
- zetaclient/config/types_test.go
- zetaclient/orchestrator/bootstap_test.go
🧰 Additional context used
📓 Path-based instructions (4)
cmd/zetaclientd/start.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/config/config_chain.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/config/types.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/orchestrator_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (1)
zetaclient/config/types.go (4)
Learnt from: kingpinXD
PR: zeta-chain/node#2568
File: zetaclient/config/types.go:161-163
Timestamp: 2024-10-08T15:34:47.578Z
Learning: Optimize the comparison with an empty `chains.Chain` instance by adding an `IsEmpty` method to the `chains.Chain` struct and using it in the `EVMConfig.Empty` method.
Learnt from: kingpinXD
PR: zeta-chain/node#2568
File: zetaclient/config/types.go:161-163
Timestamp: 2024-07-30T15:14:17.100Z
Learning: Optimize the comparison with an empty `chains.Chain` instance by adding an `IsEmpty` method to the `chains.Chain` struct and using it in the `EVMConfig.Empty` method.
Learnt from: kingpinXD
PR: zeta-chain/node#2568
File: zetaclient/config/types.go:161-163
Timestamp: 2024-10-08T15:34:48.217Z
Learning: Optimize the comparison with an empty `chains.Chain` instance by adding an `IsEmpty` method to the `chains.Chain` struct and using it in the `EVMConfig.Empty` method.
Learnt from: kingpinXD
PR: zeta-chain/node#2568
File: zetaclient/config/types.go:161-163
Timestamp: 2024-10-08T15:34:48.217Z
Learning: Optimize the comparison with an empty `chains.Chain` instance by adding an `IsEmpty` method to the `chains.Chain` struct and using it in the `EVMConfig.Empty` method.
🔇 Additional comments (7)
zetaclient/config/config_chain.go (3)
35-37: Well-documented backward compatibility context!
The added comments effectively explain the historical context of the "smoketest" username, which is valuable for maintaining backward compatibility during upgrade tests.
Line range hint 67-73: Successfully simplified chain configuration!
The removal of default chain configurations aligns perfectly with the PR objectives, ensuring that only explicitly needed chains are configured.
Line range hint 67-73: Verify documentation and migration impact
The removal of default chain configurations is a significant change that might affect existing systems.
Let's check for any documentation or configuration examples that need updating:
✅ Verification successful
Chain configuration changes are safe and properly managed
Based on the verification results, the removal of default chain configurations is intentional and well-structured:
- The codebase maintains minimal default configurations, with only
GoerliLocalnetinevmChainsConfigsandBitcoinRegtestinbtcChainsConfigs, which are specifically for local development and testing environments. - Production configurations are expected to be provided through external configuration files, as evidenced by the OpenAPI documentation that defines EVM chain configuration parameters.
- The changes align with the principle of explicit configuration over implicit defaults, reducing the risk of misconfiguration in production environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for documentation files that might reference the old chain configurations
rg -t md -t yaml -t toml "chains\.(Ethereum|BSC|Goerli|Sepolia|Mumbai)"
# Search for configuration examples
fd -e yaml -e toml -e json --exec grep -l "chains\.(Ethereum|BSC|Goerli|Sepolia|Mumbai)" {}
Length of output: 169
Script:
#!/bin/bash
# Let's try a broader search for configuration references
rg -g '!{node_modules,vendor,build,dist}/*' -l "chain.*config" .
# Search for any documentation mentioning chain configuration
fd -e md -e txt --exec grep -l -i "chain.*config" {}
# Look for configuration examples or templates
fd -e yaml -e toml -e json -e example --exec grep -l -i "chain.*config" {}
# Check for any test configurations
fd -e go --type f -E '*_test.go' --exec grep -l "chain.*[Cc]onfig" {}
# Look for specific chain configuration patterns in Go files
rg -t go "map\[.*\].*Config.*{" -A 5
Length of output: 9253
Script:
#!/bin/bash
# Let's check for chain configuration examples and defaults
rg -t go "func.*[Cc]hainsConfigs" -A 10
# Look for configuration loading or initialization
rg -t go "LoadChainConfigs|InitChainConfigs|DefaultChainConfigs" -A 5
# Check for any configuration documentation
rg -p "chain.*configuration" ./docs/ ./README.md
# Search for configuration samples or templates
fd -e json -e yaml -e toml --exec grep -l "chain.*config" {}
Length of output: 1515
zetaclient/config/types.go (2)
216-216: LGTM: Simplified Empty check aligns with field removal.
The simplified implementation correctly reflects the removal of the Chain field, now only checking the Endpoint which is the sole determinant of an empty EVMConfig.
Line range hint 39-42: Consider documenting the breaking change in EVMConfig.
The removal of the Chain field from EVMConfig is a breaking change. Consider adding documentation to help users migrate:
// EVMConfig is the config for EVM chain
type EVMConfig struct {
+ // Endpoint represents the RPC endpoint for the EVM chain
+ // Note: As of <version>, the Chain field has been removed to simplify configuration.
+ // Users should now configure chains directly through the endpoint.
Endpoint string `mask:"filled"`
RPCAlertLatency int64
}Let's verify the impact of this breaking change:
zetaclient/orchestrator/orchestrator_test.go (1)
Line range hint 4-6: Configuration simplification aligns with PR objectives.
The changes to createAppContext align with the PR objective of removing chain.Chain from the configuration. The simplified endpoint configuration is more maintainable and reduces complexity.
Also applies to: 605-605
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3137 +/- ##
===========================================
- Coverage 63.19% 63.16% -0.03%
===========================================
Files 423 423
Lines 29887 29866 -21
===========================================
- Hits 18886 18865 -21
Misses 10163 10163
Partials 838 838
|
Closes #1901
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Tests