feat(x/authz): Add MsgRevokeAll#20161
Conversation
WalkthroughWalkthroughThe Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional Context UsedLanguageTool (34)
Path-based Instructions (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (9)
api/cosmos/authz/v1beta1/tx.pulsar.go (9)
2881-2884: Ensure that global variables are necessary and used appropriately.Global variables can lead to issues with maintainability and thread safety. Consider if these variables can be encapsulated within a function or struct.
2894-2894: Type aliasing used forfastReflection_MsgRevokeAllshould be justified with comments.Explain why this type aliasing is necessary, as it can confuse future maintainers.
2933-2938: Review the necessity of theTypemethod returning custom type information.If the Go type information is not used elsewhere, consider simplifying this method to return only the descriptor as recommended.
3312-3312: Type aliasing used forfastReflection_MsgRevokeAllResponseshould be justified with comments.Explain why this type aliasing is necessary, as it can confuse future maintainers.
3351-3356: Review the necessity of theTypemethod returning custom type information for the response message.If the Go type information is not used elsewhere, consider simplifying this method to return only the descriptor as recommended.
4715-4720: Review deprecated methodDescriptorforMsgRevokeAll.Consider replacing deprecated methods with their recommended alternatives to maintain compatibility with newer protobuf versions.
4749-4754: Review deprecated methodDescriptorforMsgRevokeAllResponse.Consider replacing deprecated methods with their recommended alternatives to maintain compatibility with newer protobuf versions.
4784-4784: Review deprecated methodDescriptorforMsgPruneExpiredGrants.Consider replacing deprecated methods with their recommended alternatives to maintain compatibility with newer protobuf versions.
4820-4820: Review deprecated methodDescriptorforMsgPruneExpiredGrantsResponse.Consider replacing deprecated methods with their recommended alternatives to maintain compatibility with newer protobuf versions.
julienrbrt
changed the title
MsgRevokeAllMsgRevokeAll
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| x.Granter = value.Interface().(string) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.authz.v1beta1.EventRevokeAll")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.authz.v1beta1.EventRevokeAll")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.authz.v1beta1.EventRevokeAll does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| x.Granter = value.Interface().(string) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.authz.v1beta1.MsgRevokeAll")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.authz.v1beta1.MsgRevokeAll")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.authz.v1beta1.MsgRevokeAll does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| switch fd.FullName() { | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.authz.v1beta1.MsgRevokeAllResponse")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.authz.v1beta1.MsgRevokeAllResponse")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.authz.v1beta1.MsgRevokeAllResponse does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
add IterateGranterGrants and update basic keeper test
|
@TropicalDog17 tks for contributing. @julienrbrt the PR is ready, can you please review this PR? |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
x/authz/CHANGELOG.md (1)
31-31: Ensure consistency in the changelog entry format.The changelog entry for the
RevokeAllmethod could be improved by specifying what it revokes (e.g., "all authorization grants") to provide clearer information to the users.
x/authz/keeper/keeper.go
Outdated
| count := 0 | ||
| err := k.IterateGranterGrants(ctx, granter, func(grantee sdk.AccAddress, msgType string) (stop bool, err error) { | ||
| count++ | ||
| if err := k.DeleteGrant(ctx, grantee, granter, msgType); err != nil { |
There was a problem hiding this comment.
I don't think we should delete in the iterator (? cc @facundomedica).
shall we insted keep a map of that and delete afterwards?
There was a problem hiding this comment.
i think for now we can delete in the iterator. For your comment, perhaps we can move to a followup PR ?
There was a problem hiding this comment.
No, we can't delete in an iterator, as it's not deemed safe. Let's please gather all of the keys first and then delete them
There was a problem hiding this comment.
Actionable comments posted: 6
Out of diff range and nitpick comments (2)
api/cosmos/authz/v1beta1/tx.pulsar.go (2)
4784-4784: Deprecated method usage inDescriptor.The
Descriptormethod is marked as deprecated. Consider using the recommended alternativeProtoReflect.Descriptorto adhere to current best practices.
4820-4820: Deprecated method usage inDescriptor.The
Descriptormethod is marked as deprecated. Consider using the recommended alternativeProtoReflect.Descriptorto adhere to current best practices.
| var ( | ||
| md_MsgRevokeAll protoreflect.MessageDescriptor | ||
| fd_MsgRevokeAll_granter protoreflect.FieldDescriptor | ||
| ) |
There was a problem hiding this comment.
Initialization of global variables should be avoided due to potential initialization order issues and side effects.
Consider moving these initializations into an init() function or directly into the file_cosmos_authz_v1beta1_tx_proto_init() function to ensure they are properly initialized in a controlled manner.
| switch fd.FullName() { | ||
| case "cosmos.authz.v1beta1.MsgRevokeAll.granter": | ||
| return x.Granter != "" | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.authz.v1beta1.MsgRevokeAll")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.authz.v1beta1.MsgRevokeAll does not contain field %s", fd.FullName())) | ||
| } | ||
| } |
There was a problem hiding this comment.
Error handling in Has method uses panics for control flow, which is not recommended.
Replace panics with error returns to improve error handling and avoid potential runtime panics that can terminate the application unexpectedly.
| func (x *fastReflection_MsgRevokeAll) Clear(fd protoreflect.FieldDescriptor) { | ||
| switch fd.FullName() { | ||
| case "cosmos.authz.v1beta1.MsgRevokeAll.granter": | ||
| x.Granter = "" | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.authz.v1beta1.MsgRevokeAll")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.authz.v1beta1.MsgRevokeAll does not contain field %s", fd.FullName())) | ||
| } | ||
| } |
There was a problem hiding this comment.
Use of panics for error handling in Clear method.
Consider revising the error handling strategy to return errors instead of using panics, enhancing the robustness and reliability of the code.
| // Set is a mutating operation and unsafe for concurrent use. | ||
| func (x *fastReflection_MsgRevokeAll) Set(fd protoreflect.FieldDescriptor, value protoreflect.Value) { | ||
| switch fd.FullName() { | ||
| case "cosmos.authz.v1beta1.MsgRevokeAll.granter": | ||
| x.Granter = value.Interface().(string) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.authz.v1beta1.MsgRevokeAll")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.authz.v1beta1.MsgRevokeAll does not contain field %s", fd.FullName())) | ||
| } | ||
| } |
There was a problem hiding this comment.
The Set method uses panics for error handling, which could lead to abrupt termination of the program.
It is safer to handle errors gracefully by returning them to the caller, allowing them to decide on the appropriate action.
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
Could you add an autocli config? (In autocli.go)
|
@julienrbrt I updated code according to your comments |
|
|
||
| func (x *MsgPruneExpiredGrants) slowProtoReflect() protoreflect.Message { | ||
| mi := &file_cosmos_authz_v1beta1_tx_proto_msgTypes[6] | ||
| mi := &file_cosmos_authz_v1beta1_tx_proto_msgTypes[8] |
There was a problem hiding this comment.
Ensure that the method slowProtoReflect for MsgPruneExpiredGrants is fully implemented. This snippet shows an incomplete implementation which might be an error or oversight.
Would you like me to complete the implementation of this method or open a GitHub issue to track this task?
|
|
||
| func (x *MsgPruneExpiredGrantsResponse) slowProtoReflect() protoreflect.Message { | ||
| mi := &file_cosmos_authz_v1beta1_tx_proto_msgTypes[7] | ||
| mi := &file_cosmos_authz_v1beta1_tx_proto_msgTypes[9] |
There was a problem hiding this comment.
Ensure that the method slowProtoReflect for MsgPruneExpiredGrantsResponse is fully implemented. This snippet shows an incomplete implementation which might be an error or oversight.
Would you like me to complete the implementation of this method or open a GitHub issue to track this task?
improve DeleteAllGrants
x/authz/keeper/keeper.go
Outdated
|
|
||
| // DeleteAllGrants revokes all authorizations granted to the grantee by the granter. | ||
| func (k Keeper) DeleteAllGrants(ctx context.Context, granter sdk.AccAddress) error { | ||
| var keysToDelete []string |
There was a problem hiding this comment.
nit: no need to be a []string, make it a [][]byte please
There was a problem hiding this comment.
yeah. I've just updated
facundomedica
left a comment
facundomedica
left a comment
There was a problem hiding this comment.
LGTM, there's a small nit to solve but everything else looks good. Thank you!
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
x/authz/CHANGELOG.md (1)
31-31: Ensure consistency in the changelog entry format.Consider adding a brief description of the
MsgRevokeAllmessage along with theRevokeAllmethod to provide more context about the change.
5 participants
Description
Closes: #20139
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.
I have...
Summary by CodeRabbit
New Features
x/authzmodule.MsgRevokeAllto revoke all grants issued by a specified granter without specifying individual grantee addresses.Documentation
Tests