feat(x/circuit): Add validation for permission when an account is assigned and validation for msgURL #22460
Conversation
GNaD13
requested review from
kocubinski,
akhilkumarpilli,
raynaudoe and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a validation step in the Changes
Assessment against linked issues
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
|
![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
🧹 Outside diff range and nitpick comments (4)
x/circuit/types/permission.go (2)
5-20: Improve comment style and fix typosThe comments should follow Uber style guide conventions and maintain consistency.
Apply this diff:
func (p *Permissions) Validation() error { switch { case p.Level == Permissions_LEVEL_SOME_MSGS: - // if permission is some msg, LimitTypeUrls array must not be empty + // Validation ensures LimitTypeUrls array is not empty for SOME_MSGS permission. if len(p.LimitTypeUrls) == 0 { return errors.New("LimitTypeUrls of LEVEL_SOME_MSGS should NOT be empty") } case p.Level == Permissions_LEVEL_ALL_MSGS || p.Level == Permissions_LEVEL_SUPER_ADMIN: - // if permission is all msg or super addmin, LimitTypeUrls array clear - // all p.LimitTypeUrls since we not use this field + // Validation clears LimitTypeUrls for ALL_MSGS or SUPER_ADMIN permissions + // as this field is not used for these levels. p.LimitTypeUrls = nil default: return errors.New("unknown permission level") } return nil }
5-20: Consider using switch on p.Level directlyThe current switch implementation using boolean conditions could be simplified.
Apply this diff for better readability:
func (p *Permissions) Validation() error { - switch { - case p.Level == Permissions_LEVEL_SOME_MSGS: + switch p.Level { + case Permissions_LEVEL_SOME_MSGS: if len(p.LimitTypeUrls) == 0 { return errors.New("LimitTypeUrls of LEVEL_SOME_MSGS should NOT be empty") } - case p.Level == Permissions_LEVEL_ALL_MSGS || p.Level == Permissions_LEVEL_SUPER_ADMIN: + case Permissions_LEVEL_ALL_MSGS, Permissions_LEVEL_SUPER_ADMIN: p.LimitTypeUrls = nil default: return errors.New("unknown permission level") } return nil }x/circuit/keeper/msg_server_test.go (2)
121-134: Consider adding more assertions for super admin permissionsWhile the test correctly verifies that LimitTypeUrls is empty after authorization, consider adding assertions to verify:
- The permission level remains SUPER_ADMIN
- The authorization event is emitted correctly
perms, err := ft.keeper.Permissions.Get(ft.ctx, add1) require.NoError(t, err) // LimitTypeUrls should be empty require.Equal(t, len(perms.LimitTypeUrls), 0) + require.Equal(t, types.Permissions_LEVEL_SUPER_ADMIN, perms.Level) + require.Equal( + t, + sdk.NewEvent( + "authorize_circuit_breaker", + sdk.NewAttribute("granter", authority), + sdk.NewAttribute("grantee", addresses[1]), + sdk.NewAttribute("permission", adminPerms.String()), + ), + lastEvent(ft.ctx), + )
135-159: Add descriptive comments to clarify test behaviorThe test case would benefit from explicit comments explaining why LimitTypeUrls should be empty for ALL_MSGS permission level, as this is a key requirement from the linked issue #22322.
// successfully add a new super user with LimitTypeUrls not empty + // ALL_MSGS permission should clear LimitTypeUrls regardless of input + // This validates the behavior specified in issue #22322 allmsgs := types.Permissions{Level: types.Permissions_LEVEL_ALL_MSGS, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate"}}
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
x/circuit/keeper/msg_server.go(1 hunks)x/circuit/keeper/msg_server_test.go(1 hunks)x/circuit/types/permission.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
x/circuit/keeper/msg_server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/circuit/keeper/msg_server_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/circuit/types/permission.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (4)
x/circuit/types/permission.go (2)
1-4: LGTM: Clean package declaration and imports
The package declaration and import statement follow Go conventions.
5-20: Implementation successfully meets PR objectives
The validation logic correctly implements the requirements from issue #22322:
- Validates that
limit_type_urlsis not empty for SOME_MSGS permission - Clears
limit_type_urlsfor ALL_MSGS and SUPER_ADMIN permissions
x/circuit/keeper/msg_server.go (1)
61-64: Consider wrapping the validation error for better context.
The validation implementation looks good, but consider wrapping the error to provide more context about where the validation failed.
- err = msg.Permissions.Validation()
- if err != nil {
- return nil, err
- }
+ if err = msg.Permissions.Validation(); err != nil {
+ return nil, errorsmod.Wrap(err, "invalid permissions")
+ }Let's verify if the validation implementation covers all requirements from issue #22322:
✅ Verification successful
The validation implementation is complete and correct
The validation in x/circuit/types/permission.go properly implements all required checks:
- For
LEVEL_SOME_MSGS: Validates thatLimitTypeUrlsis not empty - For
LEVEL_ALL_MSGSandLEVEL_SUPER_ADMIN: Clears theLimitTypeUrlsarray as it's not used - The implementation is tested as shown in
x/circuit/keeper/msg_server_test.go
The original review suggestion about error wrapping remains valid but is just a minor improvement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Validation method implements all required checks
# Look for the Validation method implementation
ast-grep --pattern 'func (p Permissions) Validation() error {
$$$
}'
# Check for SOME_MSGS validation
rg -A 5 'LEVEL_SOME_MSGS.*LimitTypeUrls.*empty'
# Check for ALL_MSGS handling
rg -A 5 'LEVEL_ALL_MSGS.*LimitTypeUrls.*clear'
# Check for message type URL validation
rg -A 5 'validateTypeURL.*leading slash'
Length of output: 214
Script:
#!/bin/bash
# Let's try a different approach to find the validation implementation
# Find all files that might contain Permissions validation
fd -e go | xargs rg -l "Permissions.*Validation"
# Look for permission level constants and validation
rg -A 10 "type Level"
# Search for any validation related to permissions
rg -A 10 "func.*Validation|func.*validate.*Permission"
# Look for type URL validation patterns
rg -A 5 "TypeURL.*validate|validate.*TypeURL"
Length of output: 10512
x/circuit/keeper/msg_server_test.go (1)
114-120: LGTM: Test setup follows established patterns
The test function is well-structured with a clear name that indicates its purpose, and follows consistent setup patterns used throughout the test file.
x/circuit/types/permission.go
Outdated
| func (p *Permissions) Validation() error { | ||
| switch { | ||
| case p.Level == Permissions_LEVEL_SOME_MSGS: | ||
| // if permission is some msg, LimitTypeUrls array must not be empty | ||
| if len(p.LimitTypeUrls) == 0 { | ||
| return errors.New("LimitTypeUrls of LEVEL_SOME_MSGS should NOT be empty") | ||
| } | ||
| case p.Level == Permissions_LEVEL_ALL_MSGS || p.Level == Permissions_LEVEL_SUPER_ADMIN: | ||
| // if permission is all msg or super addmin, LimitTypeUrls array clear | ||
| // all p.LimitTypeUrls since we not use this field | ||
| p.LimitTypeUrls = nil | ||
| default: | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Add validation for unknown permission levels
The default case should return an error for unknown permission levels instead of silently succeeding. This ensures proper error handling for all possible cases.
Apply this diff:
func (p *Permissions) Validation() error {
switch {
case p.Level == Permissions_LEVEL_SOME_MSGS:
// if permission is some msg, LimitTypeUrls array must not be empty
if len(p.LimitTypeUrls) == 0 {
return errors.New("LimitTypeUrls of LEVEL_SOME_MSGS should NOT be empty")
}
case p.Level == Permissions_LEVEL_ALL_MSGS || p.Level == Permissions_LEVEL_SUPER_ADMIN:
// if permission is all msg or super addmin, LimitTypeUrls array clear
// all p.LimitTypeUrls since we not use this field
p.LimitTypeUrls = nil
default:
+ return errors.New("unknown permission level")
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *Permissions) Validation() error { | |
| switch { | |
| case p.Level == Permissions_LEVEL_SOME_MSGS: | |
| // if permission is some msg, LimitTypeUrls array must not be empty | |
| if len(p.LimitTypeUrls) == 0 { | |
| return errors.New("LimitTypeUrls of LEVEL_SOME_MSGS should NOT be empty") | |
| } | |
| case p.Level == Permissions_LEVEL_ALL_MSGS || p.Level == Permissions_LEVEL_SUPER_ADMIN: | |
| // if permission is all msg or super addmin, LimitTypeUrls array clear | |
| // all p.LimitTypeUrls since we not use this field | |
| p.LimitTypeUrls = nil | |
| default: | |
| } | |
| return nil | |
| } | |
| func (p *Permissions) Validation() error { | |
| switch { | |
| case p.Level == Permissions_LEVEL_SOME_MSGS: | |
| // if permission is some msg, LimitTypeUrls array must not be empty | |
| if len(p.LimitTypeUrls) == 0 { | |
| return errors.New("LimitTypeUrls of LEVEL_SOME_MSGS should NOT be empty") | |
| } | |
| case p.Level == Permissions_LEVEL_ALL_MSGS || p.Level == Permissions_LEVEL_SUPER_ADMIN: | |
| // if permission is all msg or super addmin, LimitTypeUrls array clear | |
| // all p.LimitTypeUrls since we not use this field | |
| p.LimitTypeUrls = nil | |
| default: | |
| return errors.New("unknown permission level") | |
| } | |
| return nil | |
| } |
x/circuit/keeper/msg_server_test.go
Outdated
| // grants user perms to Permissions_LEVEL_SOME_MSGS with empty LimitTypeUrls | ||
| somemsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{}} | ||
| msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs} | ||
| _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) | ||
| require.Error(t, err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance SOME_MSGS validation test coverage
The test should include:
- Verification of the specific error message
- A positive test case with non-empty LimitTypeUrls
// grants user perms to Permissions_LEVEL_SOME_MSGS with empty LimitTypeUrls
somemsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{}}
msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs}
_, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg)
- require.Error(t, err)
+ require.ErrorContains(t, err, "limit_type_urls cannot be empty for permission level SOME_MSGS")
+
+ // grants user perms to Permissions_LEVEL_SOME_MSGS with valid LimitTypeUrls
+ somemsgs = types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate"}}
+ msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs}
+ _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg)
+ require.NoError(t, err)
+
+ add3, err := ft.ac.StringToBytes(addresses[3])
+ require.NoError(t, err)
+
+ perms, err = ft.keeper.Permissions.Get(ft.ctx, add3)
+ require.NoError(t, err)
+ require.Equal(t, types.Permissions_LEVEL_SOME_MSGS, perms.Level)
+ require.Equal(t, []string{"cosmos.staking.v1beta1.MsgDelegate"}, perms.LimitTypeUrls)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // grants user perms to Permissions_LEVEL_SOME_MSGS with empty LimitTypeUrls | |
| somemsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{}} | |
| msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs} | |
| _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) | |
| require.Error(t, err) | |
| } | |
| // grants user perms to Permissions_LEVEL_SOME_MSGS with empty LimitTypeUrls | |
| somemsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{}} | |
| msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs} | |
| _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) | |
| require.ErrorContains(t, err, "limit_type_urls cannot be empty for permission level SOME_MSGS") | |
| // grants user perms to Permissions_LEVEL_SOME_MSGS with valid LimitTypeUrls | |
| somemsgs = types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate"}} | |
| msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs} | |
| _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) | |
| require.NoError(t, err) | |
| add3, err := ft.ac.StringToBytes(addresses[3]) | |
| require.NoError(t, err) | |
| perms, err = ft.keeper.Permissions.Get(ft.ctx, add3) | |
| require.NoError(t, err) | |
| require.Equal(t, types.Permissions_LEVEL_SOME_MSGS, perms.Level) | |
| require.Equal(t, []string{"cosmos.staking.v1beta1.MsgDelegate"}, perms.LimitTypeUrls) | |
| } |
GNaD13
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
x/circuit/types/permission.go (2)
5-22: Method has unexpected side effectsThe
Validationmethod name suggests it only performs validation, but it also modifies theLimitTypeUrlsfield. Consider renaming it toValidateAndNormalizeto better reflect its behavior.
15-16: Fix typo in commentThe comment contains a typo: "addmin" should be "admin"
- // if permission is all msg or super addmin, LimitTypeUrls array clear + // if permission is all msg or super admin, LimitTypeUrls array clearx/circuit/keeper/msg_server_test.go (1)
167-167: Consider testing URL normalization separately.The test combines multiple type URLs with different formats (with and without leading slash). Consider splitting this into separate test cases for better clarity.
- permis := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate", "/cosmos.gov.v1beta1.MsgDeposit", "cosmos.gov.v1beta1.MsgVote"}} + // Test URL normalization + permisWithoutSlash := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate"}} + msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[4], Permissions: &permisWithoutSlash} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) + require.NoError(t, err) + + perms, err = ft.keeper.Permissions.Get(ft.ctx, add4) + require.NoError(t, err) + require.Equal(t, []string{"/cosmos.staking.v1beta1.MsgDelegate"}, perms.LimitTypeUrls) + + // Test multiple URLs + permisMultiple := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{"/cosmos.gov.v1beta1.MsgDeposit", "/cosmos.gov.v1beta1.MsgVote"}}
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
x/circuit/keeper/msg_server.go(3 hunks)x/circuit/keeper/msg_server_test.go(1 hunks)x/circuit/types/permission.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/circuit/keeper/msg_server.go
🧰 Additional context used
📓 Path-based instructions (2)
x/circuit/keeper/msg_server_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/circuit/types/permission.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (4)
x/circuit/types/permission.go (1)
13-13: Verify URL validation usage
Let's verify where this URL validation is being used in the codebase to ensure consistent usage.
✅ Verification successful
Let me check the implementation of MsgTypeURLValidation to understand its behavior.
URL validation is consistently implemented and used
The MsgTypeURLValidation function ensures all message type URLs start with a forward slash, and it's consistently used in the codebase:
- In
x/circuit/types/permission.gofor validatingLimitTypeUrls - In
x/circuit/keeper/msg_server.gofor validating message URLs before permission checks
The implementation is simple and its usage is uniform across the codebase, ensuring consistent URL formatting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MsgTypeURLValidation usage
rg "MsgTypeURLValidation" -A 3
Length of output: 1356
Script:
#!/bin/bash
# Get the implementation of MsgTypeURLValidation
ast-grep --pattern 'func MsgTypeURLValidation($_) $_'
Length of output: 469
x/circuit/keeper/msg_server_test.go (3)
114-134: LGTM: Super admin permission validation test is well-structured.
The test correctly verifies that:
- Super admin can be authorized with non-empty LimitTypeUrls
- LimitTypeUrls are properly cleared after authorization
135-159: LGTM: Super user permission validation test is comprehensive.
The test effectively validates:
- ALL_MSGS permission can be granted with non-empty LimitTypeUrls
- LimitTypeUrls are properly cleared as expected
- Event emission is correctly verified
166-189: LGTM: SOME_MSGS permission validation is thorough.
The test effectively covers:
- Authorization with multiple valid type URLs
- Proper event emission
- Verification of stored permissions
- Leading slash handling in type URLs
Note: The test aligns well with PR objective #1 regarding message type URL validation improvement.
x/circuit/types/permission.go
Outdated
| func MsgTypeURLValidation(urls []string) []string { | ||
| for idx, url := range urls { | ||
| if url[0] != '/' { | ||
| urls[idx] = "/" + url | ||
| } | ||
| } | ||
| return urls | ||
| } |
There was a problem hiding this comment.
Add bounds checking to prevent panic
The function will panic if any URL in the slice is an empty string. Add length validation before accessing the first character.
Apply this diff:
func MsgTypeURLValidation(urls []string) []string {
for idx, url := range urls {
+ if len(url) == 0 {
+ continue
+ }
if url[0] != '/' {
urls[idx] = "/" + url
}
}
return urls
}Additionally, consider renaming the function to EnsureLeadingSlash to better describe its specific purpose.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func MsgTypeURLValidation(urls []string) []string { | |
| for idx, url := range urls { | |
| if url[0] != '/' { | |
| urls[idx] = "/" + url | |
| } | |
| } | |
| return urls | |
| } | |
| func MsgTypeURLValidation(urls []string) []string { | |
| for idx, url := range urls { | |
| if len(url) == 0 { | |
| continue | |
| } | |
| if url[0] != '/' { | |
| urls[idx] = "/" + url | |
| } | |
| } | |
| return urls | |
| } |
| // grants user perms to Permissions_LEVEL_SOME_MSGS with empty LimitTypeUrls | ||
| somemsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{}} | ||
| msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs} | ||
| _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) | ||
| require.Error(t, err) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error message assertion for SOME_MSGS validation.
The test should verify the specific error message when SOME_MSGS permission is granted with empty LimitTypeUrls.
msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs}
_, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg)
- require.Error(t, err)
+ require.ErrorContains(t, err, "limit_type_urls cannot be empty for permission level SOME_MSGS")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // grants user perms to Permissions_LEVEL_SOME_MSGS with empty LimitTypeUrls | |
| somemsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{}} | |
| msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs} | |
| _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) | |
| require.Error(t, err) | |
| // grants user perms to Permissions_LEVEL_SOME_MSGS with empty LimitTypeUrls | |
| somemsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{}} | |
| msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs} | |
| _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) | |
| require.ErrorContains(t, err, "limit_type_urls cannot be empty for permission level SOME_MSGS") |
julienrbrt
added
the
backport/v0.52.x
|
Could you add a changelog under x/circuit/changelog.md? Under the improvement section |
…rmission-circuit-module
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/circuit/CHANGELOG.md (1)
44-44: Enhance the changelog entry with more specific details.While the entry follows the correct format, it could be more descriptive about the specific validations implemented. Consider expanding it to:
-* (feat) [#22460](https://github.com/cosmos/cosmos-sdk/pull/22460) Add validation for permission when an account is assigned and validation for msgURL +* (feat) [#22460](https://github.com/cosmos/cosmos-sdk/pull/22460) Add validations in x/circuit: ensure non-empty limit_type_urls for SOME_MSGS permission, clear limit_type_urls for ALL_MSGS permission, and validate msgURL format (leading slash)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
x/circuit/CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/circuit/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/circuit/keeper/msg_server_test.go (1)
166-170: Add test cases for message URL format validation.Consider adding test cases to verify that message URLs without leading slashes are automatically handled, as per the PR objectives.
// grants user perms to Permissions_LEVEL_SOME_MSGS with empty LimitTypeUrls permis := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate", "/cosmos.gov.v1beta1.MsgDeposit", "cosmos.gov.v1beta1.MsgVote"}} msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[4], Permissions: &permis} _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) require.NoError(t, err) + + // Test automatic handling of URLs without leading slash + permisNoSlash := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{"cosmos.bank.v1beta1.MsgSend"}} + msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[5], Permissions: &permisNoSlash} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) + require.NoError(t, err) + + add5, err := ft.ac.StringToBytes(addresses[5]) + require.NoError(t, err) + + perms, err = ft.keeper.Permissions.Get(ft.ctx, add5) + require.NoError(t, err) + require.Equal(t, []string{"/cosmos.bank.v1beta1.MsgSend"}, perms.LimitTypeUrls, "URL should have leading slash")
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
x/circuit/keeper/msg_server_test.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/circuit/keeper/msg_server_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (3)
x/circuit/keeper/msg_server_test.go (3)
16-16: LGTM! Constant declaration follows best practices.
The constant is well-named and its value aligns with the message URL format requirements.
114-189: LGTM! Comprehensive test coverage for permission validation.
The test thoroughly validates the permission levels and their interactions with LimitTypeUrls.
238-238: LGTM! Consistent URL format updates.
The message URLs have been updated to consistently include leading slashes, aligning with the PR objectives.
Also applies to: 339-339, 359-359, 406-406
…igned and validation for msgURL (backport #22460) (#22464) Co-authored-by: GnaD <[email protected]>
Description
Closes: #22322 issue 1, 2, 3
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Tests