feat: add coordinated zetaclient restart #3252
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📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a series of enhancements to the ZetaChain codebase, primarily focusing on operational flags. Key changes include the addition of a new test case for operational flags, the introduction of new Protocol Buffers definitions, and the implementation of methods for querying and updating operational flags. The changes also expand the command-line interface to support operational flags updates and enhance the testing framework for authorization management. Changes
Possibly related issues
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
|
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
gartnera
force-pushed
the
zetaclient-cooridinated-restart
branch
from
dcac2fb to
ca2cc9f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3252 +/- ##
===========================================
+ Coverage 61.62% 61.77% +0.15%
===========================================
Files 433 440 +7
Lines 31037 31163 +126
===========================================
+ Hits 19126 19251 +125
+ Misses 11052 11049 -3
- Partials 859 863 +4
|
gartnera
force-pushed
the
zetaclient-cooridinated-restart
branch
from
ca2cc9f to
56a323b
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: 11
🧹 Outside diff range and nitpick comments (8)
proto/zetachain/zetacore/observer/operational.proto (1)
6-10: Enhance message documentation and consider field validationThe message structure looks good, but consider these improvements:
- Add comprehensive message documentation:
+// OperationalFlags defines configuration flags for coordinating operational tasks +// across the network, such as client restarts and version updates. message OperationalFlags { // Height for a coordinated zetaclient restart. // Will be ignored if missed. int64 restart_height = 1; }
- Consider adding field validation using protobuf's built-in constraints:
// Height for a coordinated zetaclient restart. // Will be ignored if missed. - int64 restart_height = 1; + int64 restart_height = 1 [(gogoproto.moretags) = "validate:\"gte=0\""];🧰 Tools
🪛 buf (1.47.2)
6-10: Message "OperationalFlags" should have a non-empty comment for documentation.
(COMMENT_MESSAGE)
x/observer/keeper/grpc_query_operational_flags.go (1)
13-26: Add unit tests for the query handlerThe test coverage report indicates this code is not covered by tests. Consider adding tests for:
- Nil request handling
- Flags not found scenario
- Valid flags retrieval
Would you like me to help generate comprehensive unit tests for this query handler?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-25: x/observer/keeper/grpc_query_operational_flags.go#L16-L25
Added lines #L16 - L25 were not covered by testsx/observer/keeper/operational_flags.go (2)
9-26: Add comprehensive test coverageThe keeper methods lack test coverage. Consider adding tests for:
- Setting and retrieving flags
- Handling empty/nil cases
- Concurrent access scenarios
Would you like me to help generate a comprehensive test suite for these keeper methods?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 9-13: x/observer/keeper/operational_flags.go#L9-L13
Added lines #L9 - L13 were not covered by tests
[warning] 16-25: x/observer/keeper/operational_flags.go#L16-L25
Added lines #L16 - L25 were not covered by tests
9-14: Consider atomic updates for operational flagsSince this involves coordinated restarts, ensure atomic updates to prevent race conditions:
- Consider implementing a version number or timestamp for updates
- Add validation to ensure updates are monotonic
- Consider implementing optimistic locking for concurrent updates
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 9-13: x/observer/keeper/operational_flags.go#L9-L13
Added lines #L9 - L13 were not covered by testse2e/e2etests/test_operational_flags.go (2)
19-20: Enhance comment clarity regarding test safetyThe comment about low height could be more explicit about its purpose.
Consider updating the comment:
- // always set to low height so it's ignored by zetaclient + // Set to current height + 1 to ensure the value is valid but too low to trigger + // an actual zetaclient restart, preventing unintended side effects during testing
11-38: Enhance test coverage with additional scenariosThe test currently only covers the happy path. Consider adding test cases for:
- Initial state validation
- Authorization failure
- Invalid operational flags values
Consider structuring the test as subtests:
func TestOperationalFlags(r *runner.E2ERunner, _ []string) { t := r.T t.Run("happy path - successful update", func(t *testing.T) { // existing test code }) t.Run("should fail with unauthorized account", func(t *testing.T) { // test with non-operational policy account }) t.Run("should fail with invalid flags", func(t *testing.T) { // test with invalid restart height }) }proto/zetachain/zetacore/observer/tx.proto (1)
35-36: Consider adding authorization metadataThe RPC method definition could benefit from additional metadata about authorization requirements.
Consider adding a comment documenting the operational policy requirement:
+ // UpdateOperationalFlags updates the operational flags. Requires operational policy authorization. rpc UpdateOperationalFlags(MsgUpdateOperationalFlags) returns (MsgUpdateOperationalFlagsResponse);x/authority/types/authorization_list.go (1)
minimum_versionfield implementation requires trackingThe codebase search reveals no explicit tracking or implementation plans for the
minimum_versionfield inOperationalFlags. To ensure proper feature implementation:
- Create a dedicated issue to track the
minimum_versionfield implementation- Document the requirements and acceptance criteria for this feature
- Consider adding a TODO comment in the relevant code section to reference the tracking issue
🔗 Analysis chain
Line range hint
144-147: Verify tracking of minimum_version implementationThe PR objectives mention plans to add a
minimum_versionfield toOperationalFlags, but this isn't reflected in the current implementation.Let's check if there's a tracking issue:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for issues tracking minimum_version implementation gh issue list --search "minimum_version in:title,body" --state openLength of output: 69
Script:
#!/bin/bash # Let's check for any mentions of minimum_version in the codebase rg -i "minimum.?version" -A 2 -B 2 # Also check for any TODO/FIXME comments related to version rg -i "todo.*version|fixme.*version" -A 2 -B 2 # Check commit messages for any mentions git log --grep="minimum.?version" --oneline # Look for any related issues in pull request descriptions gh pr list --search "minimum_version in:title,body" --state openLength of output: 1128
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
x/observer/types/operational.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/observer/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/observer/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gox/observer/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (16)
cmd/zetae2e/local/local.go(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_operational_flags.go(1 hunks)proto/zetachain/zetacore/observer/operational.proto(1 hunks)proto/zetachain/zetacore/observer/query.proto(2 hunks)proto/zetachain/zetacore/observer/tx.proto(3 hunks)x/authority/types/authorization_list.go(1 hunks)x/authority/types/authorization_list_test.go(1 hunks)x/observer/client/cli/tx.go(1 hunks)x/observer/client/cli/tx_update_operational_flags.go(1 hunks)x/observer/keeper/grpc_query_operational_flags.go(1 hunks)x/observer/keeper/msg_server_update_operational_flags.go(1 hunks)x/observer/keeper/operational_flags.go(1 hunks)x/observer/types/codec.go(2 hunks)x/observer/types/keys.go(1 hunks)x/observer/types/message_update_operational_flags.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/authority/types/authorization_list.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/client/cli/tx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/keys.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/keeper/grpc_query_operational_flags.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/codec.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/observer/tx.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
x/observer/keeper/operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/keeper/msg_server_update_operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/client/cli/tx_update_operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/authority/types/authorization_list_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/observer/query.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
x/observer/types/message_update_operational_flags.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/keeper/grpc_query_operational_flags.go
[warning] 16-25: x/observer/keeper/grpc_query_operational_flags.go#L16-L25
Added lines #L16 - L25 were not covered by tests
x/observer/keeper/operational_flags.go
[warning] 9-13: x/observer/keeper/operational_flags.go#L9-L13
Added lines #L9 - L13 were not covered by tests
[warning] 16-25: x/observer/keeper/operational_flags.go#L16-L25
Added lines #L16 - L25 were not covered by tests
x/observer/keeper/msg_server_update_operational_flags.go
[warning] 16-23: x/observer/keeper/msg_server_update_operational_flags.go#L16-L23
Added lines #L16 - L23 were not covered by tests
[warning] 25-27: x/observer/keeper/msg_server_update_operational_flags.go#L25-L27
Added lines #L25 - L27 were not covered by tests
x/observer/types/message_update_operational_flags.go
[warning] 15-19: x/observer/types/message_update_operational_flags.go#L15-L19
Added lines #L15 - L19 were not covered by tests
[warning] 22-23: x/observer/types/message_update_operational_flags.go#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 26-27: x/observer/types/message_update_operational_flags.go#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 30-33: x/observer/types/message_update_operational_flags.go#L30-L33
Added lines #L30 - L33 were not covered by tests
[warning] 35-35: x/observer/types/message_update_operational_flags.go#L35
Added line #L35 was not covered by tests
[warning] 38-40: x/observer/types/message_update_operational_flags.go#L38-L40
Added lines #L38 - L40 were not covered by tests
[warning] 43-47: x/observer/types/message_update_operational_flags.go#L43-L47
Added lines #L43 - L47 were not covered by tests
🪛 buf (1.47.2)
proto/zetachain/zetacore/observer/operational.proto
2-2: Files with package "zetachain.zetacore.observer" must be within a directory "zetachain/zetacore/observer" relative to root but were in directory "proto/zetachain/zetacore/observer".
(PACKAGE_DIRECTORY_MATCH)
6-10: Message "OperationalFlags" should have a non-empty comment for documentation.
(COMMENT_MESSAGE)
🔇 Additional comments (11)
x/observer/keeper/grpc_query_operational_flags.go (1)
21-22: Document the rationale for ignoring 'found' status
While the comment indicates this is intentional, it would be helpful to document why this is acceptable. Consider:
- What is returned when flags are not found?
- Is this behavior safe for all clients?
✅ Verification successful
Let me check the implementation of GetOperationalFlags to understand the default behavior.
Ignoring 'found' status is acceptable in this context
The current implementation is correct and safe because:
- When flags are not found,
GetOperationalFlagsreturns a zero-valueOperationalFlagsstruct containingRestartHeight: 0 - The
RestartHeightfield is documented as "Will be ignored if missed", making the zero-value behavior appropriate - Clients properly handle the response without needing the found status, as evidenced by the e2e tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any error checks for missing flags in clients
rg -A 5 "OperationalFlags\(" --type go
Length of output: 12589
Script:
#!/bin/bash
# Check the implementation of GetOperationalFlags and OperationalFlags type
ast-grep --pattern 'func (k Keeper) GetOperationalFlags(ctx $_) (val types.OperationalFlags, found bool) {
$$$
}'
# Check the OperationalFlags type definition
ast-grep --pattern 'type OperationalFlags struct {
$$$
}'
Length of output: 1355
x/observer/client/cli/tx.go (1)
35-35: LGTM!
The command registration follows the established pattern and is correctly placed in the command list.
x/observer/types/codec.go (1)
23-23: LGTM: Codec registration follows standard pattern
The registration of MsgUpdateOperationalFlags in both the amino codec and interface registry follows the established Cosmos SDK patterns.
Also applies to: 40-40
proto/zetachain/zetacore/observer/tx.proto (1)
144-149: LGTM: Message definitions follow proto best practices
The message definitions are well-structured:
- Non-nullable constraint on
operationalFlagsensures required field validation - Empty response message follows the pattern used by other similar operations
x/authority/types/authorization_list.go (1)
23-23: LGTM: Appropriate authorization level
The message URL is correctly added to OperationPolicyMessages, which aligns with the operational nature of coordinated zetaclient restarts.
proto/zetachain/zetacore/observer/query.proto (2)
17-17: LGTM!
The import statement is correctly placed and follows the alphabetical ordering convention.
176-179: LGTM!
The message definitions follow protobuf best practices:
- Empty request message for simple queries
- Response message with non-nullable field
- Clear and consistent naming
x/authority/types/authorization_list_test.go (1)
402-402: LGTM!
The addition of MsgUpdateOperationalFlags to the operational policy message list is consistent with the feature's authorization requirements.
cmd/zetae2e/local/local.go (1)
411-411: LGTM!
The addition of TestOperationalFlagsName to the admin test suite ensures proper end-to-end testing of the operational flags feature.
e2e/e2etests/e2etests.go (2)
143-143: LGTM: Constant declaration follows conventions.
The constant TestOperationalFlagsName is well-placed in the admin tests section and follows the established naming pattern.
859-864: Enhance test description and verify test function.
While the test registration follows the correct pattern, consider:
- Expanding the description to be more specific about what aspects of operational flags are being tested.
- Consider adding relevant arguments if the test needs to be parameterized.
Let's verify the test function implementation:
✅ Verification successful
Enhance test description to reflect operational flags restart height functionality
The test function implementation shows that this E2E test specifically verifies:
- Querying operational flags
- Updating the restart height through operational policy
- Verifying the updated restart height
Consider updating the test description to: "verifies operational flags functionality for updating and querying restart height via operational policy"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the TestOperationalFlags function exists and its implementation
# Search for the TestOperationalFlags function implementation
ast-grep --pattern 'func TestOperationalFlags($$$) {
$$$
}'
Length of output: 2126
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (6)
proto/zetachain/zetacore/observer/operational.proto (1)
6-10: Enhance message documentation for better API clarityThe
OperationalFlagsmessage would benefit from comprehensive documentation explaining its purpose and usage in the context of coordinated zetaclient restarts.Consider adding this documentation:
+// OperationalFlags defines configuration flags for coordinating operational tasks +// across the network, such as zetaclient restarts and version management. message OperationalFlags { // Height for a coordinated zetaclient restart. // Will be ignored if missed. int64 restart_height = 1; }🧰 Tools
🪛 buf (1.47.2)
6-10: Message "OperationalFlags" should have a non-empty comment for documentation.
(COMMENT_MESSAGE)
x/observer/keeper/grpc_query_operational_flags.go (1)
13-26: Add test coverage for the query handlerThe query handler lacks test coverage. Consider adding tests for:
- Nil request handling
- Not found scenario
- Successful retrieval
Would you like me to help generate comprehensive test cases for this query handler?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-25: x/observer/keeper/grpc_query_operational_flags.go#L16-L25
Added lines #L16 - L25 were not covered by testsx/observer/keeper/operational_flags.go (1)
1-26: Add comprehensive test coverageBoth keeper methods lack test coverage. Consider adding tests for:
- Setting and retrieving flags
- Validation scenarios
- Error handling cases
- Event emission verification
Would you like me to help generate a comprehensive test suite for these keeper methods?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 9-13: x/observer/keeper/operational_flags.go#L9-L13
Added lines #L9 - L13 were not covered by tests
[warning] 16-25: x/observer/keeper/operational_flags.go#L16-L25
Added lines #L16 - L25 were not covered by testse2e/e2etests/test_operational_flags.go (2)
11-38: Enhance test coverage with additional test casesWhile the happy path is well tested, consider adding the following test cases:
- Attempt to update flags with unauthorized account
- Validate behavior with invalid restart height
- Test concurrent updates
- Verify persistence across chain restarts
Here's a suggested test case for unauthorized access:
func TestOperationalFlagsUnauthorized(r *runner.E2ERunner, _ []string) { // Attempt update with non-policy account updateMsg := observertypes.NewMsgUpdateOperationalFlags( r.ZetaTxServer.MustGetAccountAddressFromName("regular_user"), observertypes.OperationalFlags{ RestartHeight: 100, }, ) _, err := r.ZetaTxServer.BroadcastTx("regular_user", updateMsg) require.Error(r, err) require.Contains(r, err.Error(), "unauthorized") }
19-20: Document the purpose of low restart heightThe comment about setting a low height could be more explicit about why this is necessary for the test.
- // always set to low height so it's ignored by zetaclient + // Set restart height to current height + 1 to ensure it's in the past + // relative to the chain's current height, preventing actual client restartsproto/zetachain/zetacore/observer/query.proto (1)
176-176: Consider adding pagination to request messageThe empty request message could benefit from pagination support for future extensibility when the number of operational flags grows.
-message QueryOperationalFlagsRequest {} +message QueryOperationalFlagsRequest { + cosmos.base.query.v1beta1.PageRequest pagination = 1; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
x/observer/types/operational.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/observer/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/observer/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gox/observer/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (16)
cmd/zetae2e/local/local.go(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_operational_flags.go(1 hunks)proto/zetachain/zetacore/observer/operational.proto(1 hunks)proto/zetachain/zetacore/observer/query.proto(2 hunks)proto/zetachain/zetacore/observer/tx.proto(3 hunks)x/authority/types/authorization_list.go(1 hunks)x/authority/types/authorization_list_test.go(1 hunks)x/observer/client/cli/tx.go(1 hunks)x/observer/client/cli/tx_update_operational_flags.go(1 hunks)x/observer/keeper/grpc_query_operational_flags.go(1 hunks)x/observer/keeper/msg_server_update_operational_flags.go(1 hunks)x/observer/keeper/operational_flags.go(1 hunks)x/observer/types/codec.go(2 hunks)x/observer/types/keys.go(1 hunks)x/observer/types/message_update_operational_flags.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
x/observer/client/cli/tx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/keys.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/authority/types/authorization_list.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.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/keeper/grpc_query_operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/keeper/msg_server_update_operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/codec.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/observer/query.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
x/observer/client/cli/tx_update_operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/authority/types/authorization_list_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/observer/tx.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
x/observer/types/message_update_operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/keeper/operational_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 buf (1.47.2)
proto/zetachain/zetacore/observer/operational.proto
2-2: Files with package "zetachain.zetacore.observer" must be within a directory "zetachain/zetacore/observer" relative to root but were in directory "proto/zetachain/zetacore/observer".
(PACKAGE_DIRECTORY_MATCH)
6-10: Message "OperationalFlags" should have a non-empty comment for documentation.
(COMMENT_MESSAGE)
🪛 GitHub Check: codecov/patch
x/observer/keeper/grpc_query_operational_flags.go
[warning] 16-25: x/observer/keeper/grpc_query_operational_flags.go#L16-L25
Added lines #L16 - L25 were not covered by tests
x/observer/keeper/msg_server_update_operational_flags.go
[warning] 16-23: x/observer/keeper/msg_server_update_operational_flags.go#L16-L23
Added lines #L16 - L23 were not covered by tests
[warning] 25-27: x/observer/keeper/msg_server_update_operational_flags.go#L25-L27
Added lines #L25 - L27 were not covered by tests
x/observer/types/message_update_operational_flags.go
[warning] 15-19: x/observer/types/message_update_operational_flags.go#L15-L19
Added lines #L15 - L19 were not covered by tests
[warning] 22-23: x/observer/types/message_update_operational_flags.go#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 26-27: x/observer/types/message_update_operational_flags.go#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 30-33: x/observer/types/message_update_operational_flags.go#L30-L33
Added lines #L30 - L33 were not covered by tests
[warning] 35-35: x/observer/types/message_update_operational_flags.go#L35
Added line #L35 was not covered by tests
[warning] 38-40: x/observer/types/message_update_operational_flags.go#L38-L40
Added lines #L38 - L40 were not covered by tests
[warning] 43-47: x/observer/types/message_update_operational_flags.go#L43-L47
Added lines #L43 - L47 were not covered by tests
x/observer/keeper/operational_flags.go
[warning] 9-13: x/observer/keeper/operational_flags.go#L9-L13
Added lines #L9 - L13 were not covered by tests
[warning] 16-25: x/observer/keeper/operational_flags.go#L16-L25
Added lines #L16 - L25 were not covered by tests
🔇 Additional comments (9)
proto/zetachain/zetacore/observer/operational.proto (1)
7-9: Clarify restart coordination behavior
The comment "Will be ignored if missed" needs elaboration on the coordination mechanism to prevent race conditions or inconsistent states across nodes.
Could you clarify:
- What happens if some nodes see the height and others miss it?
- Is there a cleanup mechanism after the restart height passes?
Consider adding a status field to track whether the restart was successfully executed.
x/observer/client/cli/tx.go (1)
35-35: LGTM!
The new command is properly integrated into the existing command structure.
x/observer/types/codec.go (1)
23-23: LGTM: Codec registration follows established pattern
The registration of MsgUpdateOperationalFlags in both RegisterCodec and RegisterInterfaces follows the consistent pattern used for other message types.
Also applies to: 40-40
proto/zetachain/zetacore/observer/tx.proto (1)
35-36: LGTM: Well-structured Protobuf definitions
The new message types and RPC method are well-defined, following Protobuf best practices and existing patterns in the codebase.
Also applies to: 144-149
x/authority/types/authorization_list.go (1)
23-23: Verify authorization scope
The message URL is correctly added to OperationPolicyMessages. Let's verify that no other files grant different authorization levels for this message.
✅ Verification successful
Authorization scope verified
The message handler properly enforces authorization through the authority keeper's CheckAuthorization method. The message URL is correctly placed in OperationPolicyMessages and the handler implementation confirms that authorization is checked before executing the operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent authorization level for MsgUpdateOperationalFlags
# Search for any other authorization definitions for this message
rg -l "MsgUpdateOperationalFlags.*Policy" --type go
# Search for any policy checks in the message handler
ast-grep --pattern 'func $_(MsgUpdateOperationalFlags) {
$$$
}'
Length of output: 121
Script:
#!/bin/bash
# Let's try a broader search to find all references to this message type and authorization
# Search for the message type definition and usage
rg "MsgUpdateOperationalFlags" --type go -A 5
# Search for any policy-related files that might contain authorization rules
fd -e go -E 'vendor/*' -E 'testutil/*' . | grep -i 'policy\|auth' | xargs cat
# Search specifically in the observer module for handler implementation
fd -e go . | grep -i 'observer.*handler' | xargs cat
Length of output: 103531
Script:
#!/bin/bash
# Let's check the message handler implementation and authorization checks
# Search for the message handler implementation
rg -A 10 "func.*MsgUpdateOperationalFlags" x/observer/keeper/
# Search for authorization checks in the handler
rg -A 5 "CheckAuthorization.*MsgUpdateOperationalFlags" x/observer/keeper/
Length of output: 140
Script:
#!/bin/bash
# Let's try a different approach to find the message handler and authorization checks
# Search for the message handler file and its contents
fd -t f msg_server_update_operational_flags.go . | xargs cat
# Search for authorization checks in keeper files
fd -t f -e go . | grep -i 'keeper' | xargs rg -l "CheckAuthorization"
Length of output: 5195
proto/zetachain/zetacore/observer/query.proto (1)
178-180: LGTM: Response message structure
The response message is well-structured with proper use of gogoproto.nullable = false.
x/authority/types/authorization_list_test.go (1)
402-402: LGTM: Well-structured test addition
The addition of MsgUpdateOperationalFlags to the OperationalPolicyMessageList is appropriate and maintains consistency with the existing test structure.
cmd/zetae2e/local/local.go (1)
411-411: LGTM: Clean test integration
The addition of TestOperationalFlagsName to the admin test routine is well-integrated and maintains the existing concurrent execution pattern.
e2e/e2etests/e2etests.go (1)
859-864: Verify the test function implementation.
The test definition looks good and follows the established pattern for admin tests. However, let's verify the implementation of TestOperationalFlags.
✅ Verification successful
Test implementation is complete and follows best practices
The test function implementation in e2e/e2etests/test_operational_flags.go is well-structured and comprehensive:
- Verifies the initial operational flags state
- Updates the restart height using the operational policy account
- Confirms the update was successful through a follow-up query
- Properly handles error cases with appropriate assertions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the test function implementation.
# Test: Search for the test function implementation.
ast-grep --pattern $'func TestOperationalFlags(r *runner.E2ERunner, _ []string) {
$$$
}'
Length of output: 2154
gartnera
force-pushed
the
zetaclient-cooridinated-restart
branch
from
8afffab to
5c1e821
Compare
gartnera
force-pushed
the
zetaclient-cooridinated-restart
branch
3 times, most recently
from
baa2beb to
864154c
Compare
gartnera
force-pushed
the
zetaclient-cooridinated-restart
branch
from
864154c to
4654698
Compare
gartnera
force-pushed
the
zetaclient-cooridinated-restart
branch
from
eba17df to
bb11313
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
gartnera
added
the
CONSENSUS_BREAKING_ACK
Add an
OperationalFlagsresource to zetacore which will be used for various operational functionality. The initial field will berestart_heightwhich will be used for scheduling a coordinated zetaclient restart (#3248).The next thing I will add to this resource is a
minimum_versionfield for for the minimum version check.This PR is just zetacore functionality at the moment. Once I get initial reviews, I will continue to add the zetaclient side, add additional unit test coverage, and generate files.
TODO:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores