feat: zetaclient should be able to activate/deactivate chains at runtime#2411
feat: zetaclient should be able to activate/deactivate chains at runtime#2411ws4charlie wants to merge 24 commits intodevelopfrom
Conversation
…hestrator-add-new-chain-at-runtime
…om TSS and use BTC network params instead; cleanup and simplified methods
WalkthroughThis update encompasses enhancements to the Changes
PoemThank 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
ws4charlie
changed the title
…y when activating new observers
…hestrator-add-new-chain-at-runtime
…hestrator-add-new-chain-at-runtime
ws4charlie
requested review from
brewmaster012,
fbac,
kingpinXD,
lumtis,
skosito and
swift1337
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2411 +/- ##
===========================================
+ Coverage 69.05% 69.15% +0.10%
===========================================
Files 308 309 +1
Lines 19192 19328 +136
===========================================
+ Hits 13253 13367 +114
- Misses 5264 5282 +18
- Partials 675 679 +4
|
|
@coderabbitai can you please make a review of the change in |
|
@lumtis: I've reviewed the changes in the The changes look well-implemented and the unit tests provide good coverage. If you have any specific concerns or areas you'd like me to focus on, please let me know! Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Find and display the contents of modified files within the zetaclient directory.
git diff --name-only HEAD~1 HEAD | grep '^zetaclient/' | xargs -I {} sh -c 'echo "\n\nFile: {}\n"; cat {}'
Length of output: 42737 Actions performedReview triggered.
|
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
It looks to me there were some issues from recent merge for:
- using
NewAppContextinstead of justNew - Using pointer for config object in the source code
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- zetaclient/chains/interfaces/interfaces.go (5 hunks)
- zetaclient/orchestrator/chain_activate_test.go (1 hunks)
- zetaclient/testutils/mocks/chain_clients.go (2 hunks)
- zetaclient/zetacore/client.go (7 hunks)
- zetaclient/zetacore/tx_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- zetaclient/chains/interfaces/interfaces.go
Additional context used
Path-based instructions (4)
zetaclient/testutils/mocks/chain_clients.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/zetacore/client.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/orchestrator/chain_activate_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/zetacore/tx_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (13)
zetaclient/testutils/mocks/chain_clients.go (6)
33-33: LGTM!The
OpenDBmethod is correctly mocked to returnnil.
37-37: LGTM!The
LoadDBmethod is correctly mocked to returnnil.
41-41: LGTM!The
SaveLastBlockScannedmethod is correctly mocked to returnnil.
86-86: LGTM!The
OpenDBmethod is correctly mocked to returnnil.
90-90: LGTM!The
LoadDBmethod is correctly mocked to returnnil.
94-94: LGTM!The
SaveLastBlockScannedmethod is correctly mocked to returnnil.zetaclient/zetacore/client.go (2)
53-53: LGTM!The
CreateClientfunction correctly initializes thekeysandclient.
221-221: LGTM!The
UpdateAppContextmethod correctly retrieves and updates the app context fields.zetaclient/orchestrator/chain_activate_test.go (4)
70-70: LGTM!The
Test_ActivateChainsfunction correctly tests the activation of chains.
158-158: LGTM!The
Test_DeactivateChainsfunction correctly tests the deactivation of chains.
242-242: LGTM!The
Test_CreateSignerObserverEVMfunction correctly tests the creation of EVM chain observers.
328-328: LGTM!The
Test_CreateSignerObserverBTCfunction correctly tests the creation of BTC chain observers.zetaclient/zetacore/tx_test.go (1)
208-208: LGTM!The
TestZetacore_UpdateAppContextfunction correctly tests theUpdateAppContextmethod.
…hestrator-add-new-chain-at-runtime
|
Would be great if you can have a look at this one sometimes @swift1337 |
| } | ||
|
|
||
| // New creates empty app context with given config | ||
| func New(cfg config.Config) *AppContext { |
There was a problem hiding this comment.
This code overrides recently implemented logic in #2395. I think PR should be built on top of it.
There was a problem hiding this comment.
I'm also continuing to improve the codebase according to idiomatic Go code in this PR: #2428.
There was a problem hiding this comment.
For the 1st comment. I had hard time to resolve tons of conflicts. In case I missed something, can you point out what was overwritten? I saw the New method look like similar to this new version.
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (4)
zetaclient/orchestrator/orchestrator.go (4)
Line range hint
98-137:
Add test coverage forNewOrchestrator.Ensure that this method is covered by unit tests to verify its correct behavior.
The code changes are approved as they correctly initialize the orchestrator with the provided parameters.
Line range hint
291-390:
Add test coverage forSchedulePendingCctxs.Ensure that this method is covered by unit tests to verify its correct behavior.
Simplify the
SchedulePendingCctxsmethod.The TODO comment suggests making this function simpler. Consider refactoring to improve readability and maintainability.
The code changes are approved as they correctly schedule pending cross-chain transactions.
Line range hint
392-463:
Add test coverage forScheduleCctxEVM.Ensure that this method is covered by unit tests to verify its correct behavior.
The code changes are approved as they correctly schedule outbound keysigns for EVM chains.
Line range hint
465-515:
Add test coverage forScheduleCctxBTC.Ensure that this method is covered by unit tests to verify its correct behavior.
The code changes are approved as they correctly schedule outbound keysigns for BTC chains.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- zetaclient/orchestrator/chain_activate.go (1 hunks)
- zetaclient/orchestrator/orchestrator.go (8 hunks)
Additional context used
Path-based instructions (2)
zetaclient/orchestrator/chain_activate.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/chain_activate.go
[warning] 20-21: zetaclient/orchestrator/chain_activate.go#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 23-30: zetaclient/orchestrator/chain_activate.go#L23-L30
Added lines #L23 - L30 were not covered by tests
[warning] 48-48: zetaclient/orchestrator/chain_activate.go#L48
Added line #L48 was not covered by tests
[warning] 51-52: zetaclient/orchestrator/chain_activate.go#L51-L52
Added lines #L51 - L52 were not covered by tests
[warning] 55-56: zetaclient/orchestrator/chain_activate.go#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 59-60: zetaclient/orchestrator/chain_activate.go#L59-L60
Added lines #L59 - L60 were not covered by tests
[warning] 154-157: zetaclient/orchestrator/chain_activate.go#L154-L157
Added lines #L154 - L157 were not covered by tests
[warning] 172-175: zetaclient/orchestrator/chain_activate.go#L172-L175
Added lines #L172 - L175 were not covered by tests
Additional comments not posted (2)
zetaclient/orchestrator/chain_activate.go (2)
63-82: LGTM!The function correctly deactivates unsupported chains by stopping the observers and removing them from the maps.
84-114: LGTM!The function correctly activates newly supported chains by loading the database, starting the observers, and updating the maps.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- zetaclient/orchestrator/orchestrator_test.go (9 hunks)
Additional context used
Path-based instructions (1)
zetaclient/orchestrator/orchestrator_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (6)
zetaclient/orchestrator/orchestrator_test.go (6)
28-28: LGTM!The changes to
MockOrchestratorcorrectly utilize the newappContextparameter.Also applies to: 45-45, 55-56
61-61: LGTM!The
CreateTestAppContextfunction correctly initializes theappContextwith the provided chain parameters.Also applies to: 74-92
97-117: LGTM!The
Test_Stopfunction correctly tests theStopmethod of the orchestrator, ensuring it can be called multiple times without panic.
130-130: LGTM!The
Test_GetUpdatedSignerfunction correctly tests various scenarios for theGetUpdatedSignermethod, ensuring it handles missing signers and updated addresses.Also applies to: 138-149
169-169: LGTM!The
Test_GetUpdatedChainObserverfunction correctly tests various scenarios for theGetUpdatedChainObservermethod, ensuring it handles missing observers and updated chain parameters.Also applies to: 204-234
399-399: LGTM!The
Test_GetPendingCctxsWithinRatelimitfunction correctly tests various scenarios for theGetPendingCctxsWithinRatelimitmethod, ensuring it handles different rate limiter scenarios and fallback mechanisms.
…oint connection failure
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
zetaclient/orchestrator/chain_activate.go (1)
21-37: Optimize the ticker interval for different environments.Consider optimizing the ticker interval based on different environments to avoid potential performance issues.
- ticker := time.NewTicker(common.ZetaBlockTime * 2) + ticker := time.NewTicker(time.Duration(getTickerInterval()) * time.Second) func getTickerInterval() int { // Return different intervals based on the environment // Example: return 2 for local, 60 for testnet/mainnet }Tools
GitHub Check: codecov/patch
[warning] 22-23: zetaclient/orchestrator/chain_activate.go#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 25-31: zetaclient/orchestrator/chain_activate.go#L25-L31
Added lines #L25 - L31 were not covered by tests
[warning] 33-35: zetaclient/orchestrator/chain_activate.go#L33-L35
Added lines #L33 - L35 were not covered by tests
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- zetaclient/orchestrator/chain_activate.go (1 hunks)
- zetaclient/orchestrator/chain_activate_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- zetaclient/orchestrator/chain_activate_test.go
Additional context used
Path-based instructions (1)
zetaclient/orchestrator/chain_activate.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Learnings (1)
zetaclient/orchestrator/chain_activate.go (5)
Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/chain_activate.go:184-247 Timestamp: 2024-07-05T00:02:31.446Z Learning: The `CreateSignerObserverBTC` function in `zetaclient/orchestrator/chain_activate.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go`.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`.Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/orchestrator.go:222-237 Timestamp: 2024-07-04T23:47:56.072Z Learning: The `GetUpdatedChainObserver` method in the `Orchestrator` class is covered by unit tests in `zetaclient/orchestrator/orchestrator_test.go` and `zetaclient/orchestrator/chain_activate_test.go`.Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/chain_activate.go:19-33 Timestamp: 2024-07-05T00:00:55.199Z Learning: The `WatchActivatedChains` method in the `Orchestrator` class is covered by end-to-end tests.Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/orchestrator.go:192-217 Timestamp: 2024-07-04T23:46:38.428Z Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.
GitHub Check: codecov/patch
zetaclient/orchestrator/chain_activate.go
[warning] 22-23: zetaclient/orchestrator/chain_activate.go#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 25-31: zetaclient/orchestrator/chain_activate.go#L25-L31
Added lines #L25 - L31 were not covered by tests
[warning] 33-35: zetaclient/orchestrator/chain_activate.go#L33-L35
Added lines #L33 - L35 were not covered by tests
[warning] 53-53: zetaclient/orchestrator/chain_activate.go#L53
Added line #L53 was not covered by tests
[warning] 56-57: zetaclient/orchestrator/chain_activate.go#L56-L57
Added lines #L56 - L57 were not covered by tests
[warning] 60-62: zetaclient/orchestrator/chain_activate.go#L60-L62
Added lines #L60 - L62 were not covered by tests
[warning] 64-66: zetaclient/orchestrator/chain_activate.go#L64-L66
Added lines #L64 - L66 were not covered by tests
[warning] 70-71: zetaclient/orchestrator/chain_activate.go#L70-L71
Added lines #L70 - L71 were not covered by tests
Additional comments not posted (8)
zetaclient/orchestrator/chain_activate.go (8)
53-73: Ensure comprehensive error handling.The function handles errors during the creation of signers and observers. Ensure that all potential errors are logged and handled appropriately.
Tools
GitHub Check: codecov/patch
[warning] 53-53: zetaclient/orchestrator/chain_activate.go#L53
Added line #L53 was not covered by tests
[warning] 56-57: zetaclient/orchestrator/chain_activate.go#L56-L57
Added lines #L56 - L57 were not covered by tests
[warning] 60-62: zetaclient/orchestrator/chain_activate.go#L60-L62
Added lines #L60 - L62 were not covered by tests
[warning] 64-66: zetaclient/orchestrator/chain_activate.go#L64-L66
Added lines #L64 - L66 were not covered by tests
[warning] 70-71: zetaclient/orchestrator/chain_activate.go#L70-L71
Added lines #L70 - L71 were not covered by tests
76-95: Ensure proper synchronization and resource release.The function uses a mutex to ensure synchronization while modifying the maps. Ensure that the observers are stopped and resources are released properly.
97-127: Ensure proper synchronization and resource initialization.The function uses a mutex to ensure synchronization while modifying the maps. Ensure that the observers are started and resources are initialized properly.
129-192: Ensure comprehensive error handling.The function handles errors during the creation of signers and observers. Ensure that all potential errors are logged and handled appropriately.
195-255: Ensure comprehensive error handling.The function handles errors during the creation of signers and observers. Ensure that all potential errors are logged and handled appropriately.
195-255: Ensure test coverage forCreateSignerObserverBTC.The function should be covered by unit tests to verify its correct behavior.
Verification successful
Verified: The
CreateSignerObserverBTCfunction is covered by unit tests.
zetaclient/orchestrator/chain_activate_test.go
Test_CreateSignerObserverBTCoc.CreateSignerObserverBTCis invoked within the test function.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `CreateSignerObserverBTC` is covered by unit tests. # Test: Search for the function usage in test files. Expect: Occurrences of the function in test files. rg 'CreateSignerObserverBTC' --glob '*_test.go'Length of output: 252
129-192: Ensure test coverage forCreateSignerObserverEVM.The function should be covered by unit tests to verify its correct behavior.
Verification successful
Verified:
CreateSignerObserverEVMis covered by unit tests.The function
CreateSignerObserverEVMis adequately tested in thezetaclient/orchestrator/chain_activate_test.gofile, ensuring its correct behavior.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `CreateSignerObserverEVM` is covered by unit tests. # Test: Search for the function usage in test files. Expect: Occurrences of the function in test files. rg 'CreateSignerObserverEVM' --glob '*_test.go'Length of output: 573
21-37: Ensure test coverage forWatchActivatedChains.The function lacks direct unit test coverage. Ensure that this function is covered by unit tests.
Tools
GitHub Check: codecov/patch
[warning] 22-23: zetaclient/orchestrator/chain_activate.go#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 25-31: zetaclient/orchestrator/chain_activate.go#L25-L31
Added lines #L25 - L31 were not covered by tests
[warning] 33-35: zetaclient/orchestrator/chain_activate.go#L33-L35
Added lines #L33 - L35 were not covered by tests
|
This PR has too many conflicts and most of the changes are covered in #2428. @ws4charlie do you mind if I close this PR and open another one with cherry-picked changes from here? cc @lumtis |
|
We can now close this one right @ws4charlie @swift1337 ? |
3 participants
Description
zetaclientinitialization logics.AppContextby consolidating[ZetacoreContext + AppContext] ==> AppContext.ZetacoreContextis removed.AppContext.Closes: 1904
Closes: 1397
How Has This Been Tested?
Summary by CodeRabbit
New Features
AppContextfunctionality.Bug Fixes
Refactor
Orchestratorstruct with new fields and methods.Tests
mockeryto versionv2.42.2for various test utility packages.Chores