feat(observer): implement MsgUpdateOperationalChainParams
#3538
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 extensive modifications to enhance chain parameter management. It adds a new field, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as MsgServer
participant A as AuthorityKeeper
participant CP as ChainParamsStore
C->>M: UpdateOperationalChainParams(msg)
M->>A: CheckAuthorization(msg.creator)
A-->>M: Authorization result
alt Authorized
M->>CP: Retrieve chain parameters (by chainID)
alt Chain parameters found
M->>CP: Update parameters with new values
CP-->>M: Success
M-->>C: Return success response
else Not Found
M-->>C: Return error (ChainParamsNotFound)
end
else Not Authorized
M-->>C: Return error (Unauthorized)
end
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
|
lumtis
changed the title
MsgUpdateOperationalChainParams
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3538 +/- ##
===========================================
+ Coverage 64.54% 64.61% +0.07%
===========================================
Files 456 458 +2
Lines 31747 31825 +78
===========================================
+ Hits 20490 20565 +75
- Misses 10353 10355 +2
- Partials 904 905 +1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (17)
x/observer/keeper/msg_server_update_operational_chain_params.go (2)
13-15: Clarify the comment regarding sensitive values.Currently, the link between “MsgUpdateChainParams” and the areas considered “sensitive” is somewhat vague. Consider succinctly reinforcing that sensitive fields (e.g., gateway contract) are purposefully excluded from this operational update to preserve stricter access control.
27-28: Correct or rephrase the inline documentation.The comment “// can't only update existing params” appears to contain a grammatical slip. If the intent is to convey that only existing parameters may be updated (not created), then consider clarifying it for readability, e.g. “// Can only update existing params, not create them.”
x/observer/types/message_update_chain_params.go (1)
51-51: Confirm thatChainParams.Validate()covers all newly introduced fields.Where newly added fields (e.g.,
ConfirmationParams) exist, ensure they are thoroughly validated. Extending test coverage to corner cases (e.g., negative values or zero thresholds) is recommended.x/authority/migrations/v3/migrate.go (2)
30-33: Double-check admin coverage for chain parameter edits.The message
/zetachain.zetacore.observer.MsgUpdateChainParamsis assignedPolicyType_groupAdmin. Validate that administrators truly require broader privileges than operational groups for these specific chain parameter edits, ensuring no overlap or confusion with operational updates.
44-46: Keep the authorization list sorted or structured if expansions continue.As the number of authorized messages grows, consider a consistent organizational pattern or mechanical means (e.g., enumerating them in ascending order by message URL) to improve maintainability and clarity in future migrations.
x/observer/types/message_update_operational_chain_params.go (1)
58-81: Enhance validation in ValidateBasic().The validation should include checks for empty ticker strings and reasonable bounds for schedule parameters.
func (msg *MsgUpdateOperationalChainParams) ValidateBasic() error { _, err := sdk.AccAddressFromBech32(msg.Creator) if err != nil { return cosmoserrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err) } if msg.ChainId < 0 { return cosmoserrors.Wrap(sdkerrors.ErrInvalidChainID, "chain id cannot be negative") } + if msg.GasPriceTicker == "" || msg.InboundTicker == "" || msg.OutboundTicker == "" { + return cosmoserrors.Wrap(sdkerrors.ErrInvalidRequest, "ticker values cannot be empty") + } if msg.OutboundScheduleInterval < 0 { return cosmoserrors.Wrap(sdkerrors.ErrInvalidRequest, "outbound schedule interval cannot be negative") } if msg.OutboundScheduleLookahead < 0 { return cosmoserrors.Wrap(sdkerrors.ErrInvalidRequest, "outbound schedule lookahead cannot be negative") } + // Add reasonable upper bounds for schedule parameters + if msg.OutboundScheduleInterval > 86400 { // 24 hours in seconds + return cosmoserrors.Wrap(sdkerrors.ErrInvalidRequest, "outbound schedule interval too large") + } + if msg.OutboundScheduleLookahead > 3600 { // 1 hour in seconds + return cosmoserrors.Wrap(sdkerrors.ErrInvalidRequest, "outbound schedule lookahead too large") + } if err := msg.ConfirmationParams.Validate(); err != nil { return cosmoserrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) } return nil }x/observer/types/message_update_operational_chain_params_test.go (1)
14-150: Consider adding edge cases to ValidateBasic tests.While the test coverage is good, consider adding these edge cases:
- Zero values for tickers (GasPriceTicker, InboundTicker, etc.)
- Maximum possible values for confirmation counts
- Relative validation between fast and safe confirmation counts
x/observer/keeper/msg_server_update_operational_chain_params_test.go (2)
24-39: Reduce test data duplication by extracting common setup.The message setup is duplicated across multiple test cases. Consider extracting this into a helper function to improve maintainability.
+func createTestMsg(admin string) *types.MsgUpdateOperationalChainParams { + return &types.MsgUpdateOperationalChainParams{ + Creator: admin, + ChainId: 1, + GasPriceTicker: 1, + InboundTicker: 1, + OutboundTicker: 1, + WatchUtxoTicker: 1, + OutboundScheduleInterval: 1, + OutboundScheduleLookahead: 1, + ConfirmationParams: types.ConfirmationParams{ + FastInboundCount: 1, + FastOutboundCount: 1, + SafeInboundCount: 1, + SafeOutboundCount: 1, + }, + } +}Also applies to: 58-73, 92-107, 132-147
135-146: Document magic numbers in test assertions.The test uses magic numbers (1000-1009) for parameter values in the successful update test case. Consider using named constants with descriptive names to improve test readability.
+const ( + testGasPriceTicker = 1000 + testInboundTicker = 1001 + testOutboundTicker = 1002 + testWatchUtxoTicker = 1003 + testOutboundScheduleInterval = 1004 + testOutboundScheduleLookahead = 1005 + testFastInboundCount = 1006 + testFastOutboundCount = 1007 + testSafeInboundCount = 1008 + testSafeOutboundCount = 1009 +)Also applies to: 196-217
e2e/e2etests/test_migrate_chain_support.go (2)
39-196: Consider breaking down the large test function into smaller, focused test cases.The
TestMigrateChainSupportfunction is quite long and handles multiple test scenarios. Consider breaking it down into smaller sub-tests usingt.Run()for better maintainability and clarity.Example structure:
func TestMigrateChainSupport(r *runner.E2ERunner, _ []string) { t.Run("setup_initial_state", func(t *testing.T) { // Initial setup code }) t.Run("deploy_and_configure_new_chain", func(t *testing.T) { // New chain setup }) t.Run("verify_chain_migration", func(t *testing.T) { // Migration verification }) }
41-42: Document magic numbers used in test setup.The test uses magic numbers for ZETA amounts. Consider using named constants with clear documentation of their purpose.
+// TestZetaAmount represents 20B Zeta tokens used for testing +const TestZetaAmount = 20_000_000_000 + -zetaAmount := big.NewInt(1e18) -zetaAmount = zetaAmount.Mul(zetaAmount, big.NewInt(20_000_000_000)) +zetaAmount := big.NewInt(1e18) +zetaAmount = zetaAmount.Mul(zetaAmount, big.NewInt(TestZetaAmount))e2e/runner/setup_zevm.go (2)
296-299: Address TODO comment regarding protocol contracts.The TODO comment indicates a need to update all protocol contracts. This should be tracked and addressed.
Would you like me to create an issue to track this enhancement? The issue would cover:
- Updating all protocol contracts in chain params
- Including the ZETA connector
- Implementing proper validation for all contract addresses
300-332: Enhance error handling in UpdateProtocolContractsInChainParams.The function uses
require.NoErrorbut could benefit from more descriptive error handling and validation.func (r *E2ERunner) UpdateProtocolContractsInChainParams() { res, err := r.ObserverClient.GetChainParams(r.Ctx, &observertypes.QueryGetChainParamsRequest{}) - require.NoError(r, err) + if err != nil { + r.Logger.Error("failed to get chain params: %v", err) + return err + } evmChainID, err := r.EVMClient.ChainID(r.Ctx) - require.NoError(r, err) + if err != nil { + r.Logger.Error("failed to get EVM chain ID: %v", err) + return err + }x/observer/types/chain_params_test.go (2)
15-81: Enhance test table descriptions and coverage.The test table for
TestConfirmationParams_Validatecould benefit from more descriptive test cases and additional edge cases.Add test cases for:
{ name: "invalid when FastInboundCount equals SafeInboundCount", cp: types.ConfirmationParams{ SafeInboundCount: 2, FastInboundCount: 2, SafeOutboundCount: 1, FastOutboundCount: 1, }, isErr: true, }, { name: "invalid when FastOutboundCount equals SafeOutboundCount", cp: types.ConfirmationParams{ SafeInboundCount: 1, FastInboundCount: 1, SafeOutboundCount: 2, FastOutboundCount: 2, }, isErr: true, }
459-478: Consider using a more efficient deep copy method.The
copyParamsfunction could be optimized for better performance when handling large parameter sets.func copyParams(src *types.ChainParams) *types.ChainParams { if src == nil { return nil } - cp := *src + // Use a more efficient copy method for large structs + cp := &types.ChainParams{ + ChainId: src.ChainId, + ConfirmationCount: src.ConfirmationCount, + GasPriceTicker: src.GasPriceTicker, + // ... other fields + } if src.ConfirmationParams == nil { - return &cp + return cp } // ... rest of the function }x/observer/types/chain_params.go (1)
20-35: Enhance error messages in validation.The validation logic is correct but could benefit from more descriptive error messages that include the actual values.
Apply this diff to improve error messages:
func (cp ConfirmationParams) Validate() error { if cp.SafeInboundCount == 0 { - return errors.New("SafeInboundCount must be greater than 0") + return errors.New("SafeInboundCount must be greater than 0, got 0") } if cp.FastInboundCount > cp.SafeInboundCount { - return errors.New("FastInboundCount must be less than or equal to SafeInboundCount") + return errors.Errorf("FastInboundCount (%d) must be less than or equal to SafeInboundCount (%d)", cp.FastInboundCount, cp.SafeInboundCount) } if cp.SafeOutboundCount == 0 { - return errors.New("SafeOutboundCount must be greater than 0") + return errors.New("SafeOutboundCount must be greater than 0, got 0") } if cp.FastOutboundCount > cp.SafeOutboundCount { - return errors.New("FastOutboundCount must be less than or equal to SafeOutboundCount") + return errors.Errorf("FastOutboundCount (%d) must be less than or equal to SafeOutboundCount (%d)", cp.FastOutboundCount, cp.SafeOutboundCount) } return nil }docs/openapi/openapi.swagger.yaml (1)
59941-59942: Enhance Schema Documentation forobserverMsgUpdateOperationalChainParamsResponseThe new response object is defined solely with
"type: object", which meets the minimal specification; however, adding adescriptionfield, and if applicable, enumerating its expected properties, would improve clarity and usability in the API documentation. This enhancement will provide better context for API consumers and maintain production-grade documentation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
typescript/zetachain/zetacore/observer/tx_pb.d.tsis excluded by!**/*_pb.d.tsx/observer/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (28)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)docs/openapi/openapi.swagger.yaml(1 hunks)docs/spec/observer/messages.md(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_migrate_chain_support.go(2 hunks)e2e/e2etests/test_update_operational_chain_params.go(1 hunks)e2e/runner/legacy_setup_evm.go(1 hunks)e2e/runner/setup_solana.go(1 hunks)e2e/runner/setup_ton.go(1 hunks)e2e/runner/setup_zevm.go(1 hunks)proto/zetachain/zetacore/observer/tx.proto(3 hunks)x/authority/migrations/v3/migrate.go(2 hunks)x/authority/migrations/v3/migrate_test.go(1 hunks)x/authority/types/authorization_list.go(2 hunks)x/authority/types/authorization_list_test.go(2 hunks)x/observer/keeper/msg_server_update_chain_params_test.go(1 hunks)x/observer/keeper/msg_server_update_operational_chain_params.go(1 hunks)x/observer/keeper/msg_server_update_operational_chain_params_test.go(1 hunks)x/observer/simulation/operation_update_chain_params.go(2 hunks)x/observer/types/chain_params.go(4 hunks)x/observer/types/chain_params_test.go(6 hunks)x/observer/types/codec.go(2 hunks)x/observer/types/message_update_chain_params.go(1 hunks)x/observer/types/message_update_operational_chain_params.go(1 hunks)x/observer/types/message_update_operational_chain_params_test.go(1 hunks)zetaclient/context/app.go(1 hunks)zetaclient/orchestrator/contextupdater.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/observer/keeper/msg_server_update_chain_params_test.go
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/setup_solana.goe2e/runner/setup_zevm.gox/observer/types/codec.gocmd/zetae2e/local/local.gox/authority/migrations/v3/migrate.gox/authority/migrations/v3/migrate_test.gozetaclient/context/app.goe2e/runner/setup_ton.gox/observer/types/message_update_chain_params.gox/authority/types/authorization_list.goe2e/e2etests/test_migrate_chain_support.goe2e/runner/legacy_setup_evm.gozetaclient/orchestrator/contextupdater.gox/observer/simulation/operation_update_chain_params.gox/authority/types/authorization_list_test.gox/observer/types/chain_params_test.gox/observer/keeper/msg_server_update_operational_chain_params.goe2e/e2etests/test_update_operational_chain_params.gox/observer/keeper/msg_server_update_operational_chain_params_test.gox/observer/types/message_update_operational_chain_params_test.gox/observer/types/chain_params.gox/observer/types/message_update_operational_chain_params.goe2e/e2etests/e2etests.go
`**/*.proto`: Review the Protobuf definitions, point out iss...
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
proto/zetachain/zetacore/observer/tx.proto
🔇 Additional comments (23)
x/observer/keeper/msg_server_update_operational_chain_params.go (2)
33-48: Verify that partial updates will not cause unintended overrides.Each assignment directly overwrites the existing parameter field. Confirm that the intended behavior is to overwrite rather than partially preserve original values. If partial updates are expected in the future, consider granular checks or deeper merge strategies to avoid accidental resets.
50-50: Ensure fallback behavior if multiple potential matches exist.The code returns immediately once it finds a matching chain ID. If the possibility of duplicates in
chainParamsListarises, this early return might skip subsequent conflicting entries. Confirm there are no scenarios where multiple chain configurations share the same ID.x/observer/types/message_update_chain_params.go (1)
47-49: Validate that the chosen default behavior for nilChainParamsaligns with expectations.Rejecting a nil
ChainParamsoutright is typically correct, but confirm any existing usage patterns that might rely on a nil value to signify no change.x/authority/migrations/v3/migrate.go (1)
26-29: Confirm policy alignment for operational chain updates.The message
/zetachain.zetacore.observer.MsgUpdateOperationalChainParamsis assignedPolicyType_groupOperational. This grouping should align with the operational responsibilities. Verify that this operation does not require a higher-level admin oversight.x/authority/migrations/v3/migrate_test.go (1)
20-21: LGTM! Test coverage for new message authorizations.The test correctly removes authorizations for the new message types before migration, ensuring proper validation of the authorization list.
x/observer/types/codec.go (1)
24-24: LGTM! Proper registration of new message type.The new message type is correctly registered in both codec and interface registry, following the established pattern.
Also applies to: 42-42
x/observer/simulation/operation_update_chain_params.go (1)
36-38: LGTM! Enhanced error handling syntax.The refactored error handling using inline error declaration improves code readability while maintaining the same functionality.
Also applies to: 45-47
e2e/e2etests/test_update_operational_chain_params.go (1)
11-92: LGTM! Well-structured test with comprehensive verification.The test follows good practices:
- Proper error handling with
requireassertions- Thorough verification of all updated parameters
- Clear separation of setup, action, and verification phases
zetaclient/orchestrator/contextupdater.go (1)
82-82: LGTM! Improved encapsulation of validation logic.The change from
observertypes.ValidateChainParams(cp)tocp.Validate()better aligns with object-oriented principles by moving validation to the instance method.e2e/runner/setup_ton.go (1)
121-121: LGTM! Appropriate policy update for chain parameters.The change from
OperationalPolicyNametoAdminPolicyNamealigns with the PR objectives to setMsgUpdateChainParamsto admin policy.x/observer/types/message_update_operational_chain_params_test.go (1)
152-210: LGTM! Well-structured test suite.The test suite follows good practices:
- Table-driven tests with clear test cases
- Proper error assertions
- Comprehensive coverage of message functionality
e2e/runner/legacy_setup_evm.go (1)
141-143: LGTM! Policy name updated correctly.The change from
OperationalPolicyNametoAdminPolicyNamealigns with the PR objectives to setMsgUpdateChainParamsto admin.x/authority/types/authorization_list.go (1)
23-23: LGTM! Authorization lists updated correctly.The changes appropriately:
- Add
MsgUpdateOperationalChainParamsto operational policy- Move
MsgUpdateChainParamsto admin policyThis maintains clear separation between admin and operational responsibilities.
Also applies to: 45-45
zetaclient/context/app.go (1)
215-215: LGTM! Improved validation encapsulation.The change from
ValidateChainParams(params)toparams.Validate()improves encapsulation by moving validation logic to the struct method.e2e/runner/setup_solana.go (1)
185-185: LGTM! Policy name updated correctly.The change from
OperationalPolicyNametoAdminPolicyNamefor chain params update maintains proper separation between admin and operational tasks.x/authority/types/authorization_list_test.go (1)
402-402: LGTM! Authorization policy correctly configured.The addition of
MsgUpdateOperationalChainParamsto the operational policy list aligns with the intended functionality.cmd/zetae2e/local/local.go (1)
343-343: LGTM! Test case correctly added.The addition of
TestUpdateOperationalChainParamsto the admin test routine aligns with the PR objectives.e2e/e2etests/e2etests.go (1)
942-948: LGTM! Test properly configured with version requirement.The new test is correctly added with a minimum version requirement of "v28.0.0", ensuring compatibility with the new operational chain parameters functionality.
docs/spec/observer/messages.md (1)
176-194: New Message Definition for MsgUpdateOperationalChainParams is well documented.
The new section clearly explains that this message is meant for updating operational-related chain parameters without allowing modifications to sensitive values (e.g., the gateway contract). The field descriptions, including the inclusion ofConfirmationParams, match the PR objectives and enhance clarity in the documentation.proto/zetachain/zetacore/observer/tx.proto (3)
12-12: New Import for Confirmation Params.
The added import"zetachain/zetacore/observer/confirmation_params.proto"correctly brings in the definitions required for the newconfirmation_paramsfield. Ensure that the imported file remains in sync with the expected schema for consistency across modules.
40-41: Addition of the UpdateOperationalChainParams RPC Method.
The new RPC methodUpdateOperationalChainParamsin theMsgservice is appropriately defined. Its placement and syntax adhere to the conventions used in existing RPC definitions. Verify that the authorization and error handling in the implementation align with the operational policy requirements mentioned in the PR objectives.
75-89: New Message Types for Operational Chain Parameters.
The definitions forMsgUpdateOperationalChainParamsand its corresponding response (MsgUpdateOperationalChainParamsResponse) are clear and well-structured. The message includes all necessary fields (creator, chain_id, various ticker fields, schedule intervals, andConfirmationParams) with the proper field types and options (e.g., non-nullable forconfirmation_params). This aligns well with the updated functional and security requirements.changelog.md (1)
13-14: Changelog Entry for MsgUpdateOperationalChainParams is Accurate.
The new changelog entry for PR [3538] clearly states the purpose of implementingMsgUpdateOperationalChainParamsfor updating operational-related chain parameters with the operational policy. This entry is concise and consistent with both the documentation and the proto changes.
|
!!!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 |
lumtis
added
UPGRADE_TESTS
x/observer/keeper/msg_server_update_operational_chain_params.go
Outdated
Show resolved
Hide resolved
Co-authored-by: skosito <[email protected]>
| if cp.GasPriceTicker <= 0 || cp.GasPriceTicker > 300 { | ||
| return fmt.Errorf("GasPriceTicker %d out of range", cp.GasPriceTicker) | ||
| } | ||
| if params.InboundTicker <= 0 || params.InboundTicker > 300 { | ||
| return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "InboundTicker %d out of range", params.InboundTicker) | ||
| if cp.InboundTicker <= 0 || cp.InboundTicker > 300 { | ||
| return fmt.Errorf("InboundTicker %d out of range", cp.InboundTicker) | ||
| } | ||
| if params.OutboundTicker <= 0 || params.OutboundTicker > 300 { | ||
| return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "OutboundTicker %d out of range", params.OutboundTicker) | ||
| if cp.OutboundTicker <= 0 || cp.OutboundTicker > 300 { | ||
| return fmt.Errorf("OutboundTicker %d out of range", cp.OutboundTicker) | ||
| } | ||
| if params.OutboundScheduleInterval == 0 || params.OutboundScheduleInterval > 100 { // 600 secs | ||
| return errorsmod.Wrapf( | ||
| sdkerrors.ErrInvalidRequest, | ||
| if cp.OutboundScheduleInterval == 0 || cp.OutboundScheduleInterval > 100 { // 600 secs | ||
| return fmt.Errorf( | ||
|
|
||
| "OutboundScheduleInterval %d out of range", | ||
| params.OutboundScheduleInterval, | ||
| cp.OutboundScheduleInterval, | ||
| ) | ||
| } | ||
| if params.OutboundScheduleLookahead == 0 || params.OutboundScheduleLookahead > 500 { // 500 cctxs | ||
| return errorsmod.Wrapf( | ||
| sdkerrors.ErrInvalidRequest, | ||
| if cp.OutboundScheduleLookahead == 0 || cp.OutboundScheduleLookahead > 500 { // 500 cctxs | ||
| return fmt.Errorf( | ||
|
|
||
| "OutboundScheduleLookahead %d out of range", | ||
| params.OutboundScheduleLookahead, | ||
| cp.OutboundScheduleLookahead, |
There was a problem hiding this comment.
I feel like we can remove the upper limits for these values, they are set by a policy after all.
Removing might simply the validation logic.
Does not need to be done in this pr though ,
Description
Closes #3521
Add
MsgUpdateOperationalChainParamsSet
MsgUpdateChainParamsto admin (it's currently not in the migration script, my thinking is that we already plan to migrate it manually as we're setting up the operational group?) [EDIT: actually necessary for making the upgrade test work anyway]Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests