[CORE-652] add params validation to bridge message validation and add non-empty authority unit test #570
Conversation
…authority unit test
WalkthroughThe changes primarily focus on enhancing the validation logic in various Changes
TipsChat with CodeRabbit Bot (
|
tqin7
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (6)
- protocol/x/bridge/types/msg_update_event_params.go (2 hunks)
- protocol/x/bridge/types/msg_update_event_params_test.go (2 hunks)
- protocol/x/bridge/types/msg_update_propose_params.go (2 hunks)
- protocol/x/bridge/types/msg_update_propose_params_test.go (2 hunks)
- protocol/x/bridge/types/msg_update_safety_params.go (2 hunks)
- protocol/x/bridge/types/msg_update_safety_params_test.go (2 hunks)
Additional comments (Suppressed): 12
protocol/x/bridge/types/msg_update_event_params.go (2)
1-8: The import order has been changed. While this doesn't affect the functionality, it's good to follow a consistent import order for better readability and maintainability.
23-27: The
ValidateBasicfunction now callsmsg.Params.Validate(). Ensure that theValidatefunction ofmsg.Paramsis implemented and handles all necessary validation.protocol/x/bridge/types/msg_update_propose_params.go (2)
1-8: The import order has been changed. Ensure that this change is consistent with the project's import order conventions.
23-27: The
ValidateBasicfunction now callsmsg.Params.Validate(). Ensure that theValidatefunction ofmsg.Paramsperforms all necessary validation checks and handles errors appropriately.- return nil + return msg.Params.Validate()protocol/x/bridge/types/msg_update_safety_params.go (1)
- 23-27: The
ValidateBasicfunction now includes a call tomsg.Params.Validate(). Ensure that theValidatefunction ofmsg.Paramsis implemented correctly and handles all necessary validation.- return nil + return msg.Params.Validate()protocol/x/bridge/types/msg_update_safety_params_test.go (3)
- 30-56: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [19-56]
The change from comparing error objects to comparing error strings is a good move for test readability and maintainability. However, it's important to ensure that the error messages are unique and descriptive enough to avoid false positives in your tests. Also, the use of
ErrorContainsinstead ofErrorIsis a good practice as it allows for more flexible error message matching.
33-44: The test cases for "Failure: empty authority" and "Failure: invalid authority" are good additions. They help ensure that the
ValidateBasicfunction correctly handles invalid authority values.47-56: The test loop structure and the use of table-driven tests are good practices in Go. They make the tests more readable and maintainable. However, ensure that all possible edge cases are covered in your tests.
protocol/x/bridge/types/msg_update_propose_params_test.go (1)
- 20-95: The test for the
ValidateBasicfunction now covers more edge cases, including negative durations and out-of-bound skip rates. This is a good improvement as it ensures that the function behaves as expected under a wider range of inputs. The change from comparing error objects to comparing error messages (using theError()method) is also a good practice as it makes the tests less brittle.protocol/x/bridge/types/msg_update_event_params_test.go (3)
24-28: The
expectedErrfield in the test cases has been changed from anerrortype to astringtype. This change allows for more flexible error matching in the tests, as it enables the use of partial error messages for validation. However, it's important to ensure that the error messages used for validation are unique and specific enough to prevent false positives.39-72: New test cases have been added to cover invalid values for parameters such as
Denom,EthAddress, andAuthority. These tests are important for ensuring that theValidateBasicfunction correctly handles invalid input and returns the expected errors. The error messages used for validation appear to be unique and specific, which should help prevent false positives.78-82: The error handling in the tests has been updated to use the
require.ErrorContainsfunction when an error is expected. This function checks whether the actual error message contains the expected error message, which allows for more flexible error matching. However, it's important to ensure that the expected error messages are unique and specific enough to prevent false positives.
| package types | ||
|
|
||
| import ( | ||
| errorsmod "cosmossdk.io/errors" | ||
| "fmt" | ||
|
|
||
| errorsmod "cosmossdk.io/errors" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| ) |
There was a problem hiding this comment.
The import order has been changed. While this doesn't affect the functionality, it's good to follow a consistent import order for better readability and maintainability.
| func TestMsgUpdateProposeParams_ValidateBasic(t *testing.T) { | ||
| tests := map[string]struct { | ||
| msg types.MsgUpdateProposeParams | ||
| expectedErr error | ||
| expectedErr string | ||
| }{ | ||
| "Success": { | ||
| msg: types.MsgUpdateProposeParams{ | ||
| Authority: validAuthority, | ||
| Params: types.ProposeParams{ | ||
| MaxBridgesPerBlock: 5, | ||
| ProposeDelayDuration: 10_000, | ||
| ProposeDelayDuration: time.Second, | ||
| SkipRatePpm: 800_000, | ||
| SkipIfBlockDelayedByDuration: 5_000, | ||
| SkipIfBlockDelayedByDuration: time.Minute, | ||
| }, | ||
| }, | ||
| }, | ||
| "Failure: Invalid authority": { | ||
| "Failure: negative propose delay duration": { | ||
| msg: types.MsgUpdateProposeParams{ | ||
| Authority: validAuthority, | ||
| Params: types.ProposeParams{ | ||
| MaxBridgesPerBlock: 5, | ||
| ProposeDelayDuration: -time.Second, | ||
| SkipRatePpm: 800_000, | ||
| SkipIfBlockDelayedByDuration: time.Minute, | ||
| }, | ||
| }, | ||
| expectedErr: types.ErrNegativeDuration.Error(), | ||
| }, | ||
| "Failure: negative skip if blocked delayed by duration": { | ||
| msg: types.MsgUpdateProposeParams{ | ||
| Authority: validAuthority, | ||
| Params: types.ProposeParams{ | ||
| MaxBridgesPerBlock: 5, | ||
| ProposeDelayDuration: time.Second, | ||
| SkipRatePpm: 800_000, | ||
| SkipIfBlockDelayedByDuration: -time.Minute, | ||
| }, | ||
| }, | ||
| expectedErr: types.ErrNegativeDuration.Error(), | ||
| }, | ||
| "Failure: out-of-bound skip rate ppm": { | ||
| msg: types.MsgUpdateProposeParams{ | ||
| Authority: validAuthority, | ||
| Params: types.ProposeParams{ | ||
| MaxBridgesPerBlock: 5, | ||
| ProposeDelayDuration: time.Second, | ||
| SkipRatePpm: 1_000_001, | ||
| SkipIfBlockDelayedByDuration: time.Minute, | ||
| }, | ||
| }, | ||
| expectedErr: types.ErrRateOutOfBounds.Error(), | ||
| }, | ||
| "Failure: empty authority": { | ||
| msg: types.MsgUpdateProposeParams{ | ||
| Authority: "", | ||
| }, | ||
| expectedErr: types.ErrInvalidAuthority, | ||
| expectedErr: types.ErrInvalidAuthority.Error(), | ||
| }, | ||
| "Failure: invalid authority": { | ||
| msg: types.MsgUpdateProposeParams{ | ||
| Authority: "dydx1abc", | ||
| }, | ||
| expectedErr: types.ErrInvalidAuthority.Error(), | ||
| }, | ||
| } | ||
|
|
||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| err := tc.msg.ValidateBasic() | ||
| if tc.expectedErr == nil { | ||
| if tc.expectedErr == "" { | ||
| require.NoError(t, err) | ||
| } else { | ||
| require.ErrorIs(t, err, tc.expectedErr) | ||
| require.ErrorContains(t, err, tc.expectedErr) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The test cases have been expanded to cover more edge cases, including negative durations and out-of-bound skip rates. The expectedErr field has been changed from an error type to a string type, which is a good practice for comparing error messages. However, the test for an invalid authority (lines 78-83) seems to be using a hardcoded string "dydx1abc". It would be better to use a dynamically generated invalid authority for more robust testing.
- Authority: "dydx1abc",
+ Authority: generateInvalidAuthority(),
Changelist
Paramsvalidation to bridge governance messagesTest Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
protocol/CHANGELOG.mdand/orindexer/CHANGELOG.mdappropriately.state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.