feat(x/circuit): Allow msg Reset with empty msgURL #22459
Conversation
GNaD13
requested review from
kocubinski,
akhilkumarpilli,
raynaudoe and
a team
as code owners
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
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
|
|
Hey! Thanks for this! Feel free to take the other ones, as the other contributors are unresponsive. |
Yeah sir |
![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 (5)
x/circuit/keeper/msg_server_test.go (3)
377-399: Add descriptive test comments.Consider adding a comment block before the test function to describe the test's purpose and scenarios being tested. This would improve test documentation and maintainability.
+// TestResetCircuitBreakerEmptyMsgs tests the reset functionality when provided with empty message URLs. +// It verifies: +// 1. Reset behavior for accounts with SOME_MSGS permission using empty URLs +// 2. Reset behavior for authorized accounts using empty URLs func TestResetCircuitBreakerEmptyMsgs(t *testing.T) {
419-431: Fix comment typo.There's a minor typo in the comment.
- // now let's try to reset url2 with an empty url using acc2 (should pass) + // Now let's try to reset with an empty URL using acc2 (should pass)
433-446: Improve comment and error message clarity.Minor improvements for better readability and clarity.
- // now let's try to reset url with empty url using an authorized account (should pass) + // Now let's try to reset with empty URL using an authorized account (should pass) - // Both 2 url should be reset + // Both URLs should be reset - require.True(t, allowed, "circuit breaker should be reset") + require.True(t, allowed, "circuit breaker should be reset for all URLs when using authorized account")x/circuit/keeper/msg_server.go (2)
141-141: Remove unused variablekeeperThe variable
keeperis declared but not used, which can lead to a compiler warning. It's best to remove unused variables to keep the code clean.Apply this diff:
- keeper := srv.Keeper
153-153: Correct variable name in commentThe comment refers to
msgURLbut should bemsgTypeUrls. Updating the comment improves code clarity and prevents confusion.Apply this diff:
- // check if msgURL is empty + // check if msgTypeUrls is empty
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
x/circuit/keeper/msg_server.go(4 hunks)x/circuit/keeper/msg_server_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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"
🔇 Additional comments (3)
x/circuit/keeper/msg_server_test.go (2)
400-418: LGTM! Well-structured test setup and validation.
The test properly sets up the initial state and includes appropriate sanity checks before proceeding with the main test scenarios.
377-446: LGTM! Comprehensive test coverage for empty URL reset functionality.
The test thoroughly validates the new reset functionality with empty URLs, covering:
- Permission validation
- Reset behavior for accounts with limited permissions
- Reset behavior for fully authorized accounts
- Proper state validation at each step
The implementation aligns well with the PR objectives and maintains consistency with existing test patterns.
x/circuit/keeper/msg_server.go (1)
7-7: 🛠️ Refactor suggestion
Verify compatibility of using slices package
The slices package was introduced in Go 1.21. Ensure that the project is using Go 1.21 or newer. If not, consider replacing slices.Contains with an alternative to maintain compatibility.
Run the following script to check the Go version specified in go.mod:
If the Go version is less than 1.21, consider replacing slices.Contains with a custom function:
func containsString(slice []string, str string) bool {
for _, s := range slice {
if s == str {
return true
}
}
return false
}Update the code:
- "slices"
...
- if slices.Contains(perms.LimitTypeUrls, msgUrl) {
+ if containsString(perms.LimitTypeUrls, msgUrl) {Also applies to: 166-166
✅ Verification successful
slices package usage is compatible with Go version
The project uses Go 1.23.1, which fully supports the slices package introduced in Go 1.21. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the Go version specified in go.mod
grep '^go ' go.mod
Length of output: 30
x/circuit/keeper/msg_server.go
Outdated
| srv.DisableList.Walk(ctx, nil, func(msgUrl string) (stop bool, err error) { | ||
| msgTypeUrls = append(msgTypeUrls, msgUrl) | ||
| return false, nil | ||
| }) |
There was a problem hiding this comment.
Handle errors returned by DisableList.Walk
The errors returned by srv.DisableList.Walk are not being handled. Ignoring these errors could lead to silent failures if an error occurs during iteration over the disabled messages.
Apply this diff to handle the errors:
# In both instances where `DisableList.Walk` is called:
+ err := srv.DisableList.Walk(ctx, nil, func(msgUrl string) (stop bool, err error) {
// existing code...
})
+ if err != nil {
+ return nil, err
+ }Also applies to: 165-170
x/circuit/keeper/msg_server.go
Outdated
| default: | ||
| } |
There was a problem hiding this comment.
Return an error for unauthorized permission levels
The default case in the switch statement is empty. It's important to handle unexpected permission levels by returning an appropriate error to prevent unauthorized access.
Apply this diff to return an error:
default:
+ return nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "account does not have permission to reset circuit breaker")
}📝 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.
| default: | |
| } | |
| default: | |
| return nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "account does not have permission to reset circuit breaker") | |
| } |
julienrbrt
added
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
x/circuit/CHANGELOG.md (2)
44-44: Consider moving this entry to a "Features" section.The changelog guidelines suggest using a "Features" section for new features rather than "Improvements". Consider creating a new "Features" section and moving this entry there for better categorization.
### Improvements +### Features * (feat) [#22459](https://github.com/cosmos/cosmos-sdk/pull/22459) Allow msg Reset with empty msgURL
44-44: Enhance the changelog message with more context.The current message could be more descriptive about the purpose and impact of this feature. Consider expanding it to better explain the functionality.
-* (feat) [#22459](https://github.com/cosmos/cosmos-sdk/pull/22459) Allow msg Reset with empty msgURL +* (feat) [#22459](https://github.com/cosmos/cosmos-sdk/pull/22459) Allow msg Reset with empty msgURL to re-enable all previously disabled messagesx/circuit/keeper/msg_server.go (1)
153-180: Implementation looks good with minor suggestions.The implementation correctly handles empty msgTypeUrls and respects permission levels. A few suggestions for improvement:
- Consider pre-allocating the slice to improve performance:
- msgTypeUrls = append(msgTypeUrls, msgUrl) + // Pre-allocate the slice based on DisableList size + size, err := srv.DisableList.Len(ctx) + if err != nil { + return nil, err + } + msgTypeUrls = make([]string, 0, size) + // Then append + msgTypeUrls = append(msgTypeUrls, msgUrl)
- Add documentation about the reset behavior:
// check if msgURL is empty + // If msgTypeUrls is empty, collect all disabled messages that the authority has permission to reset: + // - For super admins and module authority: collect all disabled messages + // - For SOME_MSGS permission level: collect only the messages in their LimitTypeUrls if len(msgTypeUrls) == 0 {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
x/circuit/CHANGELOG.md(1 hunks)x/circuit/keeper/msg_server.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
x/circuit/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
x/circuit/keeper/msg_server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
x/circuit/keeper/msg_server.go (2)
7-7: LGTM!
The addition of the slices package is appropriate for the slice operations used in the new code.
210-210: LGTM!
The event emission correctly uses the collected msgTypeUrls, maintaining consistency with the reset operation.
…-for-circuit-module
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
x/circuit/CHANGELOG.md (1)
44-45: LGTM with minor suggestions for consistency.The changelog entries are well-structured and properly reference the related PRs. However, for better consistency with other entries in the changelog:
Consider applying these minor formatting improvements:
-* (feat) [#22459](https://github.com/cosmos/cosmos-sdk/pull/22459) Allow msg Reset with empty msgURL -* (feat) [#22460](https://github.com/cosmos/cosmos-sdk/pull/22460) Add validation for permission when an account is assigned and validation for msgURL +* (feat) [#22459](https://github.com/cosmos/cosmos-sdk/pull/22459) Allow message Reset with empty `msgURL` +* (feat) [#22460](https://github.com/cosmos/cosmos-sdk/pull/22460) Add validation for permissions during account assignment and `msgURL` validationThe suggested changes:
- Use "message" instead of "msg" for clarity
- Format technical terms with backticks
- Improve readability of the second entry
x/circuit/keeper/msg_server_test.go (2)
463-463: Add leading slash to URL for consistency.The URL string should have a leading slash to maintain consistency with other URL patterns in the codebase.
- url2 := "the_only_message_acc2_can_trip_and_reset" + url2 := "/the_only_message_acc2_can_trip_and_reset"
515-515: Fix typo in comment.Minor grammatical error in the comment.
- // Both 2 url should be reset + // Both URLs should be reset
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
x/circuit/CHANGELOG.md(1 hunks)x/circuit/keeper/msg_server.go(4 hunks)x/circuit/keeper/msg_server_test.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/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
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 (6)
x/circuit/keeper/msg_server_test.go (6)
454-476: LGTM! Test setup is well-structured.
The test initialization and authorization setup follows good testing practices with clear separation of concerns.
477-490: LGTM! Comprehensive initial state verification.
The test properly verifies the initial state by checking that both URLs are tripped after the admin trip command.
491-495: LGTM! Proper negative test case.
Good practice to verify that unauthorized reset attempts are properly rejected.
496-509: LGTM! Key feature test implementation.
This test case correctly verifies the new empty msgURL reset functionality, ensuring that only authorized messages are reset.
510-523: LGTM! Complete coverage of admin reset scenario.
The test provides good coverage of the admin reset scenario with empty URLs, verifying that all messages are properly reset.
454-523: Verify test coverage with additional edge cases.
Consider adding test cases for:
- Multiple concurrent reset attempts with empty URLs
- Reset with empty URLs when no messages are tripped
- Reset with empty URLs for accounts with ALL_MSGS permission
|
Looks like there an error in the resetting: https://github.com/cosmos/cosmos-sdk/actions/runs/11720288112/job/32645307897?pr=22459 |
…t-module' into dang/add-reset-method-for-circuit-module
(cherry picked from commit 42c616c)
* main: (31 commits) docs: update links for https security protocol (#22514) build(deps): Bump github.com/bytedance/sonic from 1.12.3 to 1.12.4 in /log (#22513) feat(x/protocolpool)!: allow any coins in continuous funds (#21916) docs: Update protobuf tx signing message format outer link (#22510) test(accounts): fix integration tests (#22418) chore(x): fix some typos in comment (#22508) build(deps): Bump cosmossdk.io/log from 1.4.1 to 1.5.0 (#22487) build(deps): Bump cosmossdk.io/core from 1.0.0-alpha.5 to 1.0.0-alpha.6 (#22502) build(deps): Bump golang.org/x/crypto from 0.28.0 to 0.29.0 (#22480) docs(adr75): server v2 (#21069) fix(server/v2): improve server stop (#22455) chore: prepare core changelog (#22495) refactor(store/v2): simplify genesis flow (#22435) build(deps): Bump google.golang.org/grpc from 1.67.1 to 1.68.0 (#22486) build(deps): Bump golang.org/x/sync from 0.8.0 to 0.9.0 (#22482) feat(x/circuit): Allow msg Reset with empty msgURL (#22459) build(deps): Bump actions/xxx-artifact from v3 to v4 (#22468) feat(stf/branch): simplify merged iterator (#22131) refactor(log): disable coloring in testing logger (#22466) chore(x/tx): update changelog to alpha.2 (#22465) ...
Description
ref: #22322 - issue 4
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
msgURL.msgURL.Bug Fixes
Tests