feat: support multiple btc chain config#2870
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 WalkthroughWalkthroughThe recent updates to the Zeta Chain node introduce significant enhancements, including support for stateful precompiled contracts, a common importable Changes
Possibly related PRs
Suggested labels
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2870 +/- ##
===========================================
+ Coverage 66.86% 67.14% +0.28%
===========================================
Files 378 378
Lines 21108 21121 +13
===========================================
+ Hits 14113 14182 +69
+ Misses 6330 6273 -57
- Partials 665 666 +1
|
ws4charlie
requested review from
brewmaster012,
fbac,
kingpinXD,
lumtis,
skosito and
swift1337
as code owners
…port-multiple-btc-chains
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
cmd/zetaclientd/start.go (1)
233-233: LGTM!The change in filtering criteria from
IsUTXOtoIsBitcoinis appropriate as it aligns with the current support for only the Bitcoin chain.Consider adding a TODO comment to track the future support for multiple UTXO chains:
+ // TODO: Support multiple UTXO chains in the future. btcChains := appContext.FilterChains(zctx.Chain.IsBitcoin)changelog.md (1)
54-54: Add the pull request number for consistency.To maintain consistency with other changelog entries, please add the pull request number in the format
[PR_NUMBER]at the beginning of the line.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (14)
- changelog.md (1 hunks)
- cmd/zetaclientd/debug.go (1 hunks)
- cmd/zetaclientd/start.go (1 hunks)
- cmd/zetaclientd/start_utils.go (1 hunks)
- zetaclient/config/config_chain.go (3 hunks)
- zetaclient/config/types.go (4 hunks)
- zetaclient/config/types_test.go (2 hunks)
- zetaclient/context/app_test.go (3 hunks)
- zetaclient/context/chain.go (1 hunks)
- zetaclient/context/chain_test.go (1 hunks)
- zetaclient/orchestrator/bootstap_test.go (2 hunks)
- zetaclient/orchestrator/bootstrap.go (2 hunks)
- zetaclient/orchestrator/orchestrator.go (1 hunks)
- zetaclient/orchestrator/orchestrator_test.go (1 hunks)
Additional context used
Path-based instructions (13)
zetaclient/config/config_chain.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetaclientd/start_utils.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/context/chain_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/config/types_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/context/chain.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetaclientd/debug.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/context/app_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/orchestrator/bootstap_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetaclientd/start.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.zetaclient/orchestrator/orchestrator.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
GitHub Check: codecov/patch
zetaclient/orchestrator/bootstrap.go
[warning] 142-142: zetaclient/orchestrator/bootstrap.go#L142
Added line #L142 was not covered by tests
[warning] 148-148: zetaclient/orchestrator/bootstrap.go#L148
Added line #L148 was not covered by tests
[warning] 326-326: zetaclient/orchestrator/bootstrap.go#L326
Added line #L326 was not covered by testszetaclient/orchestrator/orchestrator.go
[warning] 413-413: zetaclient/orchestrator/orchestrator.go#L413
Added line #L413 was not covered by tests
Additional comments not posted (30)
zetaclient/config/config_chain.go (4)
17-17: LGTM!The initialization of the
BTCChainConfigsfield as an empty map in theNewfunction is correct and consistent with theEVMChainConfigsfield.
24-25: LGTM!The population of the
BTCChainConfigsfield with default values using thebtcChainsConfigsfunction whensetDefaultsis true is correct and consistent with theEVMChainConfigsfield.The order of populating the fields is not a concern as long as all the required fields are populated before the
Configobject is returned.
34-34: LGTM!The change in the
RPCUsernamefield from "smoketest" to "e2etest" does not affect the functionality of the code and is likely due to a change in the naming convention or the purpose of the configuration.
80-86: LGTM!The
btcChainsConfigsfunction is consistent with theevmChainsConfigsfunction in terms of structure and purpose. It returns a map with a single entry for theBitcoinRegtestchain using thebitcoinConfigRegnetfunction, which is sufficient for the current use case.The function can be easily extended to include more entries for other Bitcoin chains if needed in the future.
cmd/zetaclientd/start_utils.go (4)
55-59: LGTM!The updated function comment accurately describes the purpose of the
maskCfgfunction and the specific fields that are currently being masked. The changes provide clarity and improve the documentation of the function.
62-67: Excellent simplification!The updated code for masking EVM endpoints is more straightforward and efficient. By directly setting the endpoint to an empty string, the need for URL parsing and associated error handling is eliminated. This simplifies the code and improves its clarity.
71-77: Streamlined Bitcoin configuration masking!The updated code for masking Bitcoin endpoints and credentials is more concise and aligned with the overall masking approach. By only assigning the
RPCParamsfrom the original configuration, the code avoids redundant assignments and improves readability.
78-79: Consistent masking for BitcoinConfig!The change to assign only the
RPCParamsfrom the original configuration to theBitcoinConfigstructure is consistent with the updated masking approach for Bitcoin configuration. This simplifies the masking logic and reduces redundancy in the code.zetaclient/context/chain_test.go (1)
76-76: LGTM!The change in the assertion accurately categorizes the Ethereum chain by explicitly distinguishing it from Bitcoin. This enhances the specificity and correctness of the test case.
zetaclient/config/types_test.go (2)
19-62: Excellent test coverage for EVM configuration retrieval.The
Test_GetEVMConfigfunction is well-structured and covers important scenarios for retrieving EVM configurations. The sub-tests are clearly defined and validate the expected behavior of theGetEVMConfigmethod in various situations, such as when a valid endpoint is provided, when the endpoint is empty, and when the chain is empty.The test cases ensure that the method correctly identifies valid configurations and handles cases where configurations are incomplete or missing. This comprehensive test coverage enhances the reliability and robustness of the EVM configuration handling.
64-125: Comprehensive test coverage for BTC configuration retrieval.The
Test_GetBTCConfigfunction utilizes a table-driven test approach, which is an effective way to test multiple scenarios in a concise and maintainable manner. The test cases cover important scenarios, such as finding a non-empty BTC configuration, falling back to an old configuration when a new one is not set, and ensuring that an empty configuration does not yield a valid result.Each test case is well-defined, specifying the input parameters and the expected outcome. The assertions within each test case verify that the
GetBTCConfigmethod behaves as expected based on the provided configurations.This comprehensive test coverage enhances the reliability and robustness of the BTC configuration handling, ensuring that various edge cases are accounted for in the logic.
zetaclient/context/chain.go (1)
160-160: LGTM!The function renaming from
IsUTXOtoIsBitcoinenhances clarity by explicitly indicating that it checks for a Bitcoin chain. The underlying logic remains unchanged and correctly uses thechains.IsBitcoinChainfunction. The function is concise and follows the single responsibility principle.cmd/zetaclientd/debug.go (3)
172-172: LGTM!The change from
chain.IsUTXO()tochain.IsBitcoin()is appropriate and aligns with the objective of specifically handling Bitcoin chains.
176-179: LGTM!The added check for the existence of the Bitcoin configuration and the corresponding error handling improves the robustness of the code by catching potential configuration issues early in the process.
181-186: LGTM!Using the
btcConfigvalues retrieved based on the chain ID ensures that the correct configuration is applied for the specific Bitcoin chain being processed. This change aligns with the objective of supporting multiple Bitcoin chain configurations.zetaclient/config/types.go (4)
91-91: LGTM!The change from a single
BitcoinConfigfield to aBTCChainConfigsmap is a good enhancement to support multiple Bitcoin chain configurations. The backward compatibility concern raised in the past review comment is addressed in a subsequent commit.
106-108: LGTM!The modifications to the
GetEVMConfigmethod are correct and in line with the introduction of theEVMChainConfigsmap to support multiple EVM chain configurations. Returning the config along with a boolean to indicate if it's empty is a good practice.
124-136: Excellent backward compatibility handling!The modifications to the
GetBTCConfigmethod are well-designed to support the newBTCChainConfigsmap while maintaining backward compatibility with the deprecatedBitcoinConfigfield. This allows for a smooth transition to the new structure without breaking existing configurations.The method first attempts to retrieve the config from the
BTCChainConfigsmap using the providedchainID. If the config is not found or if it's empty, it falls back to the deprecatedBitcoinConfigfield. This ensures that both new and old configurations can be handled gracefully.Great job on implementing this change!
188-192: LGTM!The modifications to the
Emptymethod forEVMConfigand the addition of theEmptymethod forBTCConfigare correct and consistent with the expected behavior.For
EVMConfig, checking both theEndpointandChainfields ensures a comprehensive check for emptiness. This is important because both fields are required for a valid EVM configuration.For
BTCConfig, checking theRPCHostfield is sufficient to determine if the config is empty. TheRPCHostis a mandatory field for a valid Bitcoin configuration, so its presence or absence can be used to assess the emptiness of the config.These changes enhance the reliability and correctness of the configuration checks.
zetaclient/context/app_test.go (4)
33-33: LGTM!The change is part of the test setup and does not affect the production code. The specific key
111and the username "satoshi" are likely chosen for testing purposes and align with the test's objective to support multiple Bitcoin chain configurations.
109-110: LGTM!The assertions are correct and improve the test coverage by ensuring that the Ethereum chain is correctly identified. They align with the test's objective to verify the behavior of the
AppContextwhen updating with new chains and parameters.
124-124: LGTM!The assertion is correct and verifies that the Bitcoin chain configuration is correctly updated with the expected RPCUsername. It aligns with the test's objective to support multiple Bitcoin chain configurations.
33-33: Excellent test coverage and structure!The changes introduce support for multiple Bitcoin chain configurations and assert the correct behavior of the
AppContextwhen updating with new chains and parameters. The test cases cover various scenarios and use clear and descriptive names, making the test's purpose and expected behavior easily understandable.The test cases follow the Given-When-Then (GWT) structure, improving readability and maintainability. The changes are well-structured and improve the overall test coverage of the
AppContext.Great work on enhancing the test suite!
Also applies to: 109-110, 124-124
zetaclient/orchestrator/bootstrap.go (2)
Line range hint
139-152: Improved clarity in handling Bitcoin chain configurations.The changes enhance the clarity and specificity of the code by:
- Explicitly fetching the Bitcoin configuration for the given chain ID using
app.Config().GetBTCConfig(chainID).- Updating the log messages to provide clearer context regarding the inability to find configurations or construct signers for Bitcoin chains.
These modifications improve the readability and maintainability of the code without altering its fundamental logic.
Tools
GitHub Check: codecov/patch
[warning] 142-142: zetaclient/orchestrator/bootstrap.go#L142
Added line #L142 was not covered by tests
[warning] 148-148: zetaclient/orchestrator/bootstrap.go#L148
Added line #L148 was not covered by tests
Line range hint
323-350: Improved clarity in handling Bitcoin chain configurations.Similar to the changes in
syncSignerMap, the modifications insyncObserverMapenhance the clarity and specificity of the code by:
- Explicitly fetching the Bitcoin configuration for the given chain ID using
app.Config().GetBTCConfig(chainID).- Updating the log message to provide clearer context regarding the inability to find configurations for Bitcoin chain observers.
These changes improve the readability and maintainability of the code without altering its fundamental logic.
Tools
GitHub Check: codecov/patch
[warning] 326-326: zetaclient/orchestrator/bootstrap.go#L326
Added line #L326 was not covered by testszetaclient/orchestrator/bootstap_test.go (2)
52-52: LGTM!The change to assign the Bitcoin configuration to a map using the chain ID as the key is a good improvement. It enhances the flexibility and scalability of the configuration management system, allowing for multiple Bitcoin chain configurations to be stored and accessed via their respective chain IDs. This change accommodates future expansions or variations in Bitcoin chain configurations without requiring significant changes to the underlying code structure.
230-230: LGTM!The change to assign the Bitcoin configuration to a map using the chain ID as the key is consistent with the similar change made in the
TestCreateSignerMapfunction. It improves the flexibility and scalability of the configuration management system for chain observers, allowing for multiple Bitcoin chain configurations to be stored and accessed via their respective chain IDs. This change enhances the overall design and maintainability of the codebase.zetaclient/orchestrator/orchestrator_test.go (1)
524-525: LGTM!The change consolidates the Bitcoin chain configuration logic and improves maintainability by handling it within the loop. The functionality remains the same.
zetaclient/orchestrator/orchestrator.go (1)
413-413: LGTM: The change enhances clarity and accuracy.Explicitly checking for the Bitcoin blockchain using
chain.IsBitcoin()instead of checking for the UTXO model is a good change. It directly associates the scheduling function with the Bitcoin blockchain rather than the more general UTXO model, which could potentially apply to other blockchains as well.Tools
GitHub Check: codecov/patch
[warning] 413-413: zetaclient/orchestrator/orchestrator.go#L413
Added line #L413 was not covered by testschangelog.md (1)
49-54: LGTM!The new features are documented clearly and concisely, following the established changelog format.
…port-multiple-btc-chains
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
Let's wait for a review from @swift1337 for this one.
Not specifically for this PR but I think we should remove the concept of EVM and BTC configs in the future. Just keeping a list of config associated with a chain ID
Totally agree. We'll need to a unified config struct (containing endpoint, user, password, etc.) for every external chain. |
5 participants
Description
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
zetacored rpcpackage for easier integration.Bug Fixes
Tests
GetEVMConfigandGetBTCConfigmethods, ensuring robustness against edge cases.