fix(zetaclient): use on-chain chain struct #2834
Conversation
gartnera
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
WalkthroughWalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
Possibly related issues
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2834 +/- ##
========================================
Coverage 66.94% 66.94%
========================================
Files 370 370
Lines 20965 20967 +2
========================================
+ Hits 14035 14037 +2
Misses 6290 6290
Partials 640 640
|
There was a problem hiding this comment.
Approved under the expectation that e2e finishes enough that you are comfortable with it @gartnera
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (5)
zetaclient/chains/evm/signer/signer_test.go (1)
Line range hint
75-87: Refactor: Simplify the creation of new EVM chain observer in tests.The changes in the
getNewEvmChainObserverfunction streamline the setup by directly using the chain identifier and a new mock JSON RPC client. This approach reduces the complexity and improves the clarity of the test setup. However, ensure that the new mock setup aligns with the expected behavior in production environments to maintain test reliability.zetaclient/orchestrator/bootstrap.go (1)
302-309: Enhance modularity by introducingethrpc2for Ethereum JSON-RPC client management.The introduction of
ethrpc2as a dependency and its use in thesyncObserverMapfunction enhances the modularity and configurability of the observer's interaction with Ethereum nodes. This change should improve the maintainability of the code by centralizing the RPC client creation. Ensure that the new client is thoroughly tested in different network conditions to verify its robustness.zetaclient/chains/evm/observer/observer_test.go (1)
Line range hint
103-141: Refactor: UpdateMockEVMObserverto includeevmJSONRPCparameter.The addition of the
evmJSONRPCparameter to theMockEVMObserverfunction enhances the flexibility and configurability of the observer setup in tests. This change ensures that the observer can utilize a JSON RPC client during initialization, which is crucial for simulating real-world scenarios in tests. Ensure that the new parameter is properly documented and that existing tests are updated to reflect this change.zetaclient/chains/evm/observer/observer.go (2)
Line range hint
64-94: Refactor Suggestion: Modular Initialization of ObserverThe changes in the
NewObserverfunction are aligned with the PR's objectives to enhance modularity and use on-chain configurations. However, there are several areas where further improvements can be made:
Error Handling: The error from
base.NewObserveris wrapped, which is good practice. However, consider adding more context or specific error handling strategies for different types of errors that might be more common in this context.Dependency Injection: The addition of
evmJSONRPCas a parameter is a good move towards dependency injection, which facilitates easier unit testing and modularity. Ensure that all dependencies are similarly managed to maintain consistency across the codebase.Configuration Validation: Before proceeding with the observer initialization, validate the
chainandchainParamsto ensure they meet expected criteria. This preemptive check can prevent runtime errors and improve system stability.Resource Management: Consider how resources are managed within the observer, especially with the new configurations. For instance, if there are resources that need to be closed or cleaned up, ensure that these are handled correctly to avoid resource leaks.
Logging and Metrics: The function initializes various components and logs errors. Ensure that successful initializations are also logged for better traceability. Additionally, consider enhancing the telemetry by adding more granular metrics for the initialization process.
Consider implementing the above suggestions to enhance the robustness, maintainability, and performance of the observer initialization process.
24-24: Architecture Advice: Use of Interfaces and Dependency InjectionThe introduction of
interfaces.EVMJSONRPCClientand its use in theNewObserverfunction is a commendable approach to dependency injection, which not only makes the code more testable but also enhances its modularity. This change aligns with modern software design practices and helps in isolating the observer functionality from specific client implementations, which can vary between different blockchain networks or configurations.Suggestions:
Expand Interface Usage: Consider defining more granular interfaces for other components within the observer, such as logging, database access, and metric collection. This would further decouple the implementation details from the observer logic, making the system more adaptable to changes.
Configuration Objects: Instead of passing multiple parameters to the
NewObserverfunction, consider using a configuration object. This can encapsulate all necessary parameters and can be validated in a single step, simplifying the function signature and improving code readability.Unit Testing: With the new changes, ensure that unit tests are updated or added to cover the new logic and configurations. Mocking the interfaces can help in testing various scenarios without the need for actual blockchain connections.
Implementing these suggestions can lead to a more robust, scalable, and maintainable system, which is crucial for blockchain-based systems where reliability and performance are paramount.
Also applies to: 64-94
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- zetaclient/chains/evm/observer/observer.go (4 hunks)
- zetaclient/chains/evm/observer/observer_test.go (8 hunks)
- zetaclient/chains/evm/signer/signer_test.go (2 hunks)
- zetaclient/orchestrator/bootstrap.go (2 hunks)
Additional context used
Path-based instructions (4)
zetaclient/chains/evm/signer/signer_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/orchestrator/bootstrap.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/observer/observer_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/observer/observer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Learnings (1)
zetaclient/chains/evm/signer/signer_test.go (1)
Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/chain_activate.go:116-181 Timestamp: 2024-07-05T00:02:36.493Z Learning: The `CreateSignerObserverEVM` function in `zetaclient/orchestrator/chain_activate.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go`.
Use the on-chain
chain.Chain{}rather than the local on disk config. This ensure that all attributes inchain.Chain{}are consistent acrosszetaclientddeployments rather than relying on operators to configure things manually.Also inject the
evmJSONRPCclient as parameter rather than constructing it inNewObserverto match the style of all the other observers.We should probably also update the config structure to not use the
chain.Chain{}type so people don't accidentally use this type in unexpected places.Closes #2829
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests