feat(bank/v2): Introduce global send restriction #21925
Conversation
hieuvubk
requested review from
julienrbrt,
akhilkumarpilli,
a team and
sontrinh16
as code owners
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes to enhance the management of send restrictions within the banking module. Key modifications include the addition of a new field in the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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
|
|
@hieuvubk your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (12)
x/bank/proto/cosmos/bank/module/v2/module.proto (2)
17-22: LGTM! Consider enhancing the comment for clarity.The addition of the
restrictions_orderfield is well-implemented and aligns with the PR objectives. The field is correctly defined as a repeated string with an appropriate field number.Consider slightly modifying the comment to improve clarity:
- // restrictions_order specifies the order of send restrictions and should be - // a list of module names which provide a send restriction instance. If no - // order is provided, then restrictions will be applied in alphabetical order - // of module names. + // restrictions_order specifies the order in which send restrictions should be applied. + // It should contain a list of module names that provide send restriction instances. + // If no order is provided, restrictions will be applied in alphabetical order + // based on the module names.This modification makes the purpose and behavior of the field more explicit and easier to understand.
Line range hint
1-22: Summary: Effective implementation of send restrictions orderThe changes introduced in this file effectively implement the ability to specify the order of send restrictions, which is a key component of the global send restriction feature mentioned in the PR objectives.
Some key points:
- The new
restrictions_orderfield allows for flexible configuration of restriction order.- If no order is specified, a default alphabetical order will be used, ensuring consistent behavior.
- The implementation is backward-compatible, as the new field is simply added to the existing
Modulemessage.Consider the following architectural implications:
- Ensure that the modules implementing send restrictions are aware of this ordering mechanism.
- Update relevant documentation to explain how modules can leverage this new feature.
- Consider adding validation logic elsewhere in the codebase to ensure that the module names provided in
restrictions_orderactually correspond to modules that implement send restrictions.x/bank/v2/keeper/restriction.go (2)
11-15: LGTM: Well-designed struct with clear purpose.The
sendRestrictionstruct is well-designed, allowing for safe updates to theSendRestrictionFnwithout requiring a pointer receiver. This is a good practice for concurrency safety.Consider slightly rewording the comment for improved clarity:
- // sendRestriction is a struct that houses a SendRestrictionFn. - // It exists so that the SendRestrictionFn can be updated in the SendKeeper without needing to have a pointer receiver. + // sendRestriction encapsulates a SendRestrictionFn. + // This design allows updating the SendRestrictionFn in the SendKeeper without requiring a pointer receiver, enhancing concurrency safety.
24-37: LGTM: Well-implemented methods for managing restrictions.The
append,prepend, andclearmethods provide flexible ways to manage send restrictions. The use of theThenmethod suggests a well-designed chaining mechanism for restrictions.For consistency in commenting style, consider updating the
clearmethod's comment:- // clear removes the send restriction (sets it to nil). + // clear removes the send restriction by setting it to nil.This aligns better with the style of the
appendandprependmethod comments.x/bank/v2/types/restrictions.go (2)
9-10: Improve GoDoc comment to follow conventionsAccording to GoDoc conventions, the comment should start with the name of the type
SendRestrictionFn. This helps with automatic documentation generation and improves clarity.Apply this diff to refine the comment:
- // A SendRestrictionFn can restrict sends and/or provide a new receiver address. + // SendRestrictionFn can restrict sends and/or provide a new receiver address.
22-23: Clarify method comment to include receiver contextFor enhanced clarity, consider adjusting the method comment to reference the receiver. This aligns with GoDoc best practices for method documentation.
Apply this diff to improve the comment:
- // Then creates a composite restriction that runs this one then the provided second one. + // Then combines the current `SendRestrictionFn` with a second one, creating a composite restriction that runs both sequentially.x/bank/v2/depinject.go (3)
82-85: Evaluate the default sorting behavior of modulesWhen
RestrictionsOrderis not specified in the configuration, the modules are sorted alphabetically. This automatic sorting may lead to unexpected ordering of send restrictions, especially when new modules are introduced. Consider whether sorting is the desired default behavior or if preserving the insertion order would be more appropriate.If consistent ordering is important, explicitly specifying
RestrictionsOrderin the configuration might be preferable.
87-89: Clarify the error message for length mismatchThe error message can be improved for readability and clarity. Consider rephrasing it to specify the expected and actual lengths, which will help in debugging.
Here's an example of a clearer error message:
-return fmt.Errorf("len(restrictions order: %v) != len(restriction modules: %v)", order, modules) +return fmt.Errorf("restriction order length (%d) does not match number of modules (%d)", len(order), len(modules))
96-98: Handle missing restrictions more gracefullyCurrently, the function returns an error if a send restriction for a module is not found. Depending on the desired behavior, you might want to handle this case more gracefully, such as logging a warning and continuing with the next module, to prevent the entire process from failing due to a missing restriction.
Consider modifying the code as follows if appropriate:
-if !ok { - return fmt.Errorf("can't find send restriction for module %s", module) +if !ok { + fmt.Printf("warning: no send restriction found for module %s\n", module) + continuex/bank/v2/keeper/keeper.go (1)
32-32: Consider renamingsendRestrictiontosendRestrictionsfor clarityIf the
sendRestrictionfield holds multiple send restrictions (as suggested by theappendandprependmethods), consider renaming it tosendRestrictionsto reflect that it may contain multiple restrictions. This can improve code readability and convey the purpose more clearly.x/bank/v2/keeper/keeper_test.go (2)
220-220: Typo in comment: "failt" should be "fail"There's a typo in the comment. Correct "failt" to "fail" for clarity.
Apply this diff to fix the typo:
- // Pass the 1st but failt at the 2nd + // Pass the 1st but fail at the 2nd
225-225: Improve comment grammarConsider rephrasing the comment for better readability.
Apply this diff to improve the comment:
- // Pass both 2 restrictions + // Pass both restrictions
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/bank/v2/types/module/module.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
- x/bank/proto/cosmos/bank/module/v2/module.proto (1 hunks)
- x/bank/v2/depinject.go (3 hunks)
- x/bank/v2/keeper/keeper.go (3 hunks)
- x/bank/v2/keeper/keeper_test.go (2 hunks)
- x/bank/v2/keeper/restriction.go (1 hunks)
- x/bank/v2/types/restrictions.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
x/bank/v2/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/keeper_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/bank/v2/keeper/restriction.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/restrictions.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (15)
x/bank/v2/keeper/restriction.go (4)
1-9: LGTM: Package declaration and imports are appropriate.The package declaration and imports are correct and follow the expected structure for a keeper file in the Cosmos SDK. The use of aliasing for the SDK types package is consistent with common practices.
17-22: LGTM: Correct initialization of sendRestriction.The
newSendRestrictionfunction correctly initializes a newsendRestrictionwith no initial restriction (nilfnfield). The comment accurately describes the function's purpose.
39-39: LGTM: Proper interface compliance check.The type assertion
var _ types.SendRestrictionFn = (*sendRestriction)(nil).applyis a good practice. It ensures at compile-time that theapplymethod ofsendRestrictionsatisfies theSendRestrictionFninterface.
1-47: Overall: Well-implemented send restriction mechanism.This new file introduces a robust and flexible mechanism for managing send restrictions in the banking module, aligning well with the PR objectives. The code is well-structured, properly commented, and follows good Go practices. It provides clear methods for appending, prepending, and clearing restrictions, as well as a type-safe way to apply them.
The implementation allows for easy extension and modification of send restrictions, which should facilitate the introduction of the global send restriction feature as intended in the PR.
x/bank/v2/types/restrictions.go (9)
1-2: Package declaration is correctThe package is appropriately declared as
types, which aligns with the directory structure and Go conventions.
3-7: Imports are properly organizedThe necessary packages are imported correctly, and the import statements are well-organized. The use of aliasing for the
sdkpackage enhances code readability.
12-13: Method comment follows GoDoc conventionsThe comment for
IsOnePerModuleTypecorrectly starts with the method name and succinctly describes its purpose.
15-16: Type assertion ensures function conformityThe use of the blank identifier assignment confirms that
NoOpSendRestrictionFnsatisfies theSendRestrictionFntype. This is a good practice to ensure type compliance at compile time.
17-20:NoOpSendRestrictionFnis correctly implementedThe no-operation function appropriately returns the original
toAddrwithout modification and no error, serving as a default or placeholder restriction function.
27-33: Method comment is thorough and informativeThe comment for
ComposeSendRestrictionscomprehensively explains the function's behavior, including edge cases and the order of execution. This level of detail is helpful for users of the function.
35-40: Efficiently filters out nil restrictionsThe code efficiently filters out
nilentries from therestrictionsslice, ensuring that only valid functions are composed.
41-46: Handles edge cases for restriction compositionThe switch statement correctly handles scenarios with zero or one restriction functions, returning
nilor the single function as appropriate.
47-56: Composite function correctly applies restrictionsThe returned anonymous function correctly applies each restriction in order, handling errors appropriately. The
toAddris updated sequentially, and the function exits early if an error occurs.x/bank/v2/depinject.go (1)
4-7: Verify the usage ofmapsandslicespackagesThe imports
mapsandslicesare part of the Go standard library starting from Go 1.21. Ensure that the project is using Go 1.21 or higher to avoid compatibility issues.To confirm the Go version specified in the project, run:
✅ Verification successful
Confirmed Go version 1.23.1 is used, ensuring compatibility with
mapsandslicespackages.🏁 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.modLength of output: 30
x/bank/v2/keeper/keeper.go (1)
100-103: LGTM!The send restriction is correctly applied before transferring coins, and errors are handled appropriately.
| // apply applies the send restriction if there is one. If not, it's a no-op. | ||
| func (r *sendRestriction) apply(ctx context.Context, fromAddr, toAddr []byte, amt sdk.Coins) ([]byte, error) { | ||
| if r == nil || r.fn == nil { | ||
| return toAddr, nil | ||
| } | ||
| return r.fn(ctx, fromAddr, toAddr, amt) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Correct implementation of apply method with proper error handling.
The apply method correctly implements the send restriction logic, handling cases where the receiver or the function is nil. It properly returns the original toAddr if no restriction is applied.
Consider a minor optimization to reduce nesting:
func (r *sendRestriction) apply(ctx context.Context, fromAddr, toAddr []byte, amt sdk.Coins) ([]byte, error) {
- if r == nil || r.fn == nil {
+ if r == nil || r.fn == nil {
return toAddr, nil
}
- return r.fn(ctx, fromAddr, toAddr, amt)
+ return r.fn(ctx, fromAddr, toAddr, amt)
}This change maintains the same logic but reduces nesting, potentially improving readability.
📝 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.
| // apply applies the send restriction if there is one. If not, it's a no-op. | |
| func (r *sendRestriction) apply(ctx context.Context, fromAddr, toAddr []byte, amt sdk.Coins) ([]byte, error) { | |
| if r == nil || r.fn == nil { | |
| return toAddr, nil | |
| } | |
| return r.fn(ctx, fromAddr, toAddr, amt) | |
| } | |
| // apply applies the send restriction if there is one. If not, it's a no-op. | |
| func (r *sendRestriction) apply(ctx context.Context, fromAddr, toAddr []byte, amt sdk.Coins) ([]byte, error) { | |
| if r == nil || r.fn == nil { | |
| return toAddr, nil | |
| } | |
| return r.fn(ctx, fromAddr, toAddr, amt) | |
| } |
| return nil | ||
| } | ||
|
|
||
| modules := slices.Collect(maps.Keys(restrictions)) |
There was a problem hiding this comment.
Remove unnecessary use of slices.Collect
The function slices.Collect is not part of the standard slices package and may cause a compilation error. Since maps.Keys already returns a slice of keys, you can simplify the code by removing slices.Collect.
Apply this fix to eliminate the undefined function:
-modules := slices.Collect(maps.Keys(restrictions))
+modules := maps.Keys(restrictions)📝 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.
| modules := slices.Collect(maps.Keys(restrictions)) | |
| modules := maps.Keys(restrictions) |
x/bank/v2/keeper/keeper.go
Outdated
| func (k Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) { | ||
| k.sendRestriction.append(restriction) | ||
| } | ||
|
|
||
| // PrependSendRestriction adds the provided SendRestrictionFn to run before previously provided restrictions. | ||
| func (k Keeper) PrependSendRestriction(restriction types.SendRestrictionFn) { | ||
| k.sendRestriction.prepend(restriction) | ||
| } | ||
|
|
||
| // ClearSendRestriction removes the send restriction (if there is one). | ||
| func (k Keeper) ClearSendRestriction() { | ||
| k.sendRestriction.clear() | ||
| } |
There was a problem hiding this comment.
Use pointer receivers for methods that modify Keeper
The methods AppendSendRestriction, PrependSendRestriction, and ClearSendRestriction modify the Keeper by altering its sendRestriction field. According to Go best practices and the Uber Go Style Guide, methods that modify the receiver should have pointer receivers to ensure changes affect the original instance.
Apply this diff to change the receiver to a pointer:
-func (k Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) {
+func (k *Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) {
}-func (k Keeper) PrependSendRestriction(restriction types.SendRestrictionFn) {
+func (k *Keeper) PrependSendRestriction(restriction types.SendRestrictionFn) {
}-func (k Keeper) ClearSendRestriction() {
+func (k *Keeper) ClearSendRestriction() {
}📝 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 (k Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) { | |
| k.sendRestriction.append(restriction) | |
| } | |
| // PrependSendRestriction adds the provided SendRestrictionFn to run before previously provided restrictions. | |
| func (k Keeper) PrependSendRestriction(restriction types.SendRestrictionFn) { | |
| k.sendRestriction.prepend(restriction) | |
| } | |
| // ClearSendRestriction removes the send restriction (if there is one). | |
| func (k Keeper) ClearSendRestriction() { | |
| k.sendRestriction.clear() | |
| } | |
| func (k *Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) { | |
| k.sendRestriction.append(restriction) | |
| } | |
| // PrependSendRestriction adds the provided SendRestrictionFn to run before previously provided restrictions. | |
| func (k *Keeper) PrependSendRestriction(restriction types.SendRestrictionFn) { | |
| k.sendRestriction.prepend(restriction) | |
| } | |
| // ClearSendRestriction removes the send restriction (if there is one). | |
| func (k *Keeper) ClearSendRestriction() { | |
| k.sendRestriction.clear() | |
| } |
| addrRestrictFunc := func(ctx context.Context, from, to []byte, amount sdk.Coins) ([]byte, error) { | ||
| if bytes.Equal(from, to) { | ||
| return nil, fmt.Errorf("Can not send to same address") | ||
| } | ||
| return to, nil | ||
| } |
There was a problem hiding this comment.
Error message should start with lowercase and use "cannot"
Per the Uber Go Style Guide, error messages should start with a lowercase letter, and "cannot" should be a single word. Please update the error message accordingly.
Apply this diff to correct the error message:
- return nil, fmt.Errorf("Can not send to same address")
+ return nil, fmt.Errorf("cannot send to same address")Also, update the test assertion to match the new error message:
- require.Contains(err.Error(), "Can not send to same address")
+ require.Contains(err.Error(), "cannot send to same address")Committable suggestion was skipped due to low confidence.
| if len(amount) > 1 { | ||
| return nil, fmt.Errorf("Allow only one denom per one send") | ||
| } | ||
| return to, nil | ||
| } |
There was a problem hiding this comment.
Error message should start with lowercase and improve wording
According to the Uber Go Style Guide, error messages should start with a lowercase letter. Additionally, consider rephrasing the message for clarity.
Apply this diff to improve the error message:
- return nil, fmt.Errorf("Allow only one denom per one send")
+ return nil, fmt.Errorf("allow only one denom per send")Also, update the test assertion to match the new error message:
- require.Contains(err.Error(), "Allow only one denom per one send")
+ require.Contains(err.Error(), "allow only one denom per send")Committable suggestion was skipped due to low confidence.
x/bank/v2/keeper/keeper.go
Outdated
| @@ -252,3 +258,18 @@ func newBalancesIndexes(sb *collections.SchemaBuilder) BalancesIndexes { | |||
| type BalancesIndexes struct { | |||
| Denom *indexes.ReversePair[[]byte, string, math.Int] | |||
| } | |||
|
|
|||
| // AppendSendRestriction adds the provided SendRestrictionFn to run after previously provided restrictions. | |||
| func (k Keeper) AppendSendRestriction(restriction types.SendRestrictionFn) { | |||
There was a problem hiding this comment.
nit: can be move this in another file, and call the API Global?
There was a problem hiding this comment.
it should relay in keeper I think or we have to make keeper.sendRestriction public
There was a problem hiding this comment.
That's fine, I meant renaming to AppendGlobalSendRestriction and maybe put all sendrestrictions logic ousitde of keeper.go but still in the keeper
Description
Ref: #21873
sendRestrictionto keeperRestrictionsOrderto module.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