feat: add zetaclient minimum version check #3320
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 📝 WalkthroughWalkthroughThis pull request introduces a minimum version check mechanism for the ZetaClient. The changes span multiple files to implement version validation during client startup. The primary objective is to ensure that the ZetaClient meets a specified minimum version requirement, potentially preventing incompatible client versions from running and triggering automatic shutdown if version requirements are not met. 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
|
gartnera
added
ADMIN_TESTS
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3320 +/- ##
===========================================
+ Coverage 61.86% 61.88% +0.02%
===========================================
Files 444 444
Lines 31457 31492 +35
===========================================
+ Hits 19461 19490 +29
- Misses 11124 11129 +5
- Partials 872 873 +1
|
gartnera
changed the title
gartnera
force-pushed
the
zetaclient-signer-latency-correction
branch
from
d5d0c22 to
d80e97d
Compare
gartnera
force-pushed
the
zetaclient-version-check
branch
3 times, most recently
from
abbdba6 to
ae0816e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
zetaclient/maintenance/shutdown_listener.go (2)
169-171: Default version retrieval might benefit from testing.
getVersionDefaultreferencesconstant.Version. Static analysis indicates these lines are not covered by tests. Adding a brief test ensures code stability if future changes are introduced.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 169-170: zetaclient/maintenance/shutdown_listener.go#L169-L170
Added lines #L169 - L170 were not covered by tests
173-178: Prefix helper is straightforward.
ensurePrefixis concise and clear. Including a small test for edge cases (e.g., empty prefix) might further improve confidence.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 177-177: zetaclient/maintenance/shutdown_listener.go#L177
Added line #L177 was not covered by testsx/observer/types/operational.go (1)
27-29: Validation for minimum_version is correct, but test coverage for invalid semver is recommended.
This logic properly returns an error iff.MinimumVersionis an invalid semver. However, static analysis indicates no direct test coverage for this path.Do you want to add or generate a test case verifying the behavior when
MinimumVersionis not valid semver?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 28-29: x/observer/types/operational.go#L28-L29
Added lines #L28 - L29 were not covered by testsx/observer/types/operational_test.go (1)
55-66: Valid version scenarios are tested; consider adding an invalid version test.
The test checks a valid semver. A counterpart test catching invalid semver strings would solidify coverage and confidence in the new logic.cmd/zetaclientd/start.go (1)
125-129: Confirm TSS setup order and context cancellation.Initializing TSS after the preflight checks ensures the system only proceeds if validations pass. As an added safeguard, confirm that the TSS setup respects context cancellation (e.g., if the pre-check fails or a shutdown signal is triggered during setup) to avoid partial or corrupted initialization.
docs/openapi/openapi.swagger.yaml (1)
58118-58122: Implementation looks good but could be enhanced.The property definition and description are clear and well-placed within the operational flags section.
Consider enhancing the schema definition with:
- A pattern regex to validate semver format
- Example values for better documentation
minimum_version: type: string + pattern: '^v\d+\.\d+\.\d+$|^$' description: |- Minimum version of zetaclient that is allowed to run. This must be either a valid semver string (v23.0.1) or empty. + example: "v23.0.1"zetaclient/maintenance/shutdown_listener_test.go (1)
105-131: Enhance version string handling and test coverageThe test case effectively verifies the basic version check functionality, but could be improved in the following areas:
- Version strings should consistently follow semantic versioning format (with 'v' prefix)
- Consider using constants for version strings to maintain consistency
- Add edge cases for version comparison
Consider applying these improvements:
+const ( + testVersionOk = "v1.1.2" + testVersionMin = "v1.1.1" +) t.Run("minimum version ok", func(t *testing.T) { client := mocks.NewZetacoreClient(t) listener := NewShutdownListener(client, logger) listener.getVersion = func() string { - return "1.1.2" + return testVersionOk } client.Mock.On("GetOperationalFlags", ctx).Return(observertypes.OperationalFlags{ - MinimumVersion: "v1.1.1", + MinimumVersion: testVersionMin, }, nil)Consider adding test cases for:
- Equal versions
- Version with different number of components (e.g., "v1.1" vs "v1.1.0")
- Invalid version strings
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
typescript/zetachain/zetacore/observer/operational_pb.d.tsis excluded by!**/*_pb.d.tsx/observer/types/operational.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (8)
cmd/zetaclientd/start.go(1 hunks)docs/openapi/openapi.swagger.yaml(1 hunks)proto/zetachain/zetacore/observer/operational.proto(1 hunks)x/observer/types/errors.go(1 hunks)x/observer/types/operational.go(2 hunks)x/observer/types/operational_test.go(2 hunks)zetaclient/maintenance/shutdown_listener.go(5 hunks)zetaclient/maintenance/shutdown_listener_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
x/observer/types/errors.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/operational.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/observer/operational.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
x/observer/types/operational_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/maintenance/shutdown_listener_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/maintenance/shutdown_listener.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
x/observer/types/operational.go
[warning] 28-29: x/observer/types/operational.go#L28-L29
Added lines #L28 - L29 were not covered by tests
zetaclient/maintenance/shutdown_listener.go
[warning] 47-48: zetaclient/maintenance/shutdown_listener.go#L47-L48
Added lines #L47 - L48 were not covered by tests
[warning] 67-67: zetaclient/maintenance/shutdown_listener.go#L67
Added line #L67 was not covered by tests
[warning] 169-170: zetaclient/maintenance/shutdown_listener.go#L169-L170
Added lines #L169 - L170 were not covered by tests
[warning] 177-177: zetaclient/maintenance/shutdown_listener.go#L177
Added line #L177 was not covered by tests
🔇 Additional comments (15)
zetaclient/maintenance/shutdown_listener.go (10)
5-6: Imports look appropriate for added functionality.
No immediate issues are detected with adding"fmt"and"strings", as they serve error structuring and prefix handling in this file.
11-11: Semantic version comparison import is appropriate.
Usinggolang.org/x/mod/semveris a good choice for implementing robust semver checks.
14-14: Constant import is well-scoped.
No concerns regarding the import ofgithub.com/zeta-chain/node/pkg/constantfor version retrieval.
29-29: Injecting version retrieval enhances testability.
DeclaringgetVersionas a function field is a clean approach, allowing you to inject and mock version retrieval for testing.
36-38: Constructor assigns default implementation correctly.
Storing the default version retrieval function in the constructor is neat. This design fosters better modularity and potential overrides.
42-53: Pre-start checks implemented correctly, but lines 47-48 lack test coverage.
TheRunPreStartCheckmethod gracefully retrieves flags and checks versions before P2P starts. However, static analysis warns that lines 47-48 are not covered by tests. Consider adding a failing retrieval scenario test to improve coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 47-48: zetaclient/maintenance/shutdown_listener.go#L47-L48
Added lines #L47 - L48 were not covered by tests
65-67: Comprehensive retry approach for operational flags.
Line 67 returns the wrapped error if retrieval fails after retries. To ensure reliability, consider testing retry-failure scenarios around these lines, as static analysis indicates line 67 lacks coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 67-67: zetaclient/maintenance/shutdown_listener.go#L67
Added line #L67 was not covered by tests
93-98: Typed backoff and retry logic looks sound.
The usage ofretry.DoTypedWithBackoffAndRetryis consistent with resilient design. Ensure that potential repeated failures are logged or handled appropriately.
102-105: Immediate return on version check failure ensures timely shutdown.
WhencheckMinimumVersionfails, returningtruepromptly signals shutdown. This behavior appears correct. If possible, add integration tests to confirm that a mismatch triggers the intended shutdown logic.
153-167: Version comparison is correct for semver compliance.
The call to ensure a 'v' prefix and subsequentsemver.Compareis straightforward and correct. No concurrency, correctness, or security issues detected.x/observer/types/operational.go (1)
7-7: Importing semver for enhanced validation.
Leveraginggolang.org/x/mod/semveraligns with the addition of MinimumVersion checks.proto/zetachain/zetacore/observer/operational.proto (1)
20-22: New field for minimum version is clearly documented.
The addedminimum_versionfield, along with descriptive comments, is consistent with semver-based validation elsewhere.x/observer/types/operational_test.go (1)
18-21: Empty OperationalFlags test is appropriate.
Confirming that an empty instance passes validation ensures default behavior remains intact.x/observer/types/errors.go (1)
74-78: New semantic version error is clearly defined.
ErrOperationalFlagsInvalidMinimumVersionadds clarity and maintains consistency with existing error-handling patterns.cmd/zetaclientd/start.go (1)
113-118: Ensure pre-start checks handle all required validations gracefully.The flow introduced here correctly invokes
RunPreStartCheckbefore continuing the startup process. Consider verifying that any additional operational checks (e.g., version compatibility or environment preconditions) are included within this method to avoid unexpected failures later. Otherwise, it appears consistent and aligns well with graceful shutdown handling.
gartnera
force-pushed
the
zetaclient-version-check
branch
from
f2cd32a to
bc77fe1
Compare
gartnera
force-pushed
the
zetaclient-version-check
branch
from
bc77fe1 to
ae373be
Compare
gartnera
force-pushed
the
zetaclient-version-check
branch
from
ae373be to
0c6d0b4
Compare
Closes #3248
Should probably check the major version before start too. I don't think there's anything that actually disconnects zetaclient from TSS p2p when the upgrade happens either.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests