fix(x/authz): bring back msg response in DispatchActions
#21044
Conversation
WalkthroughWalkthroughThe recent modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MsgRouterService
participant Keeper
Client->>Keeper: Dispatch Actions
Keeper->>MsgRouterService: Invoke Untyped(msg)
MsgRouterService-->>Keeper: Response (resp)
Keeper->>Keeper: Create Any instance with resp
alt Create Any fails
Keeper-->>Client: Error message
else Create Any succeeds
Keeper->>Keeper: Marshal response
alt Marshal fails
Keeper-->>Client: Error message
else Marshal succeeds
Keeper-->>Client: Success message
end
end
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 Configuration File (
|
This comment has been minimized.
This comment has been minimized.
![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
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/authz/keeper/keeper.go (2 hunks)
Additional context used
Path-based instructions (1)
x/authz/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (3)
x/authz/keeper/keeper.go (3)
149-152: Capture and handle the response fromInvokeUntyped.The response from
InvokeUntypedis now captured inrespand handled further. This change improves the functionality by allowing the response to be processed.
154-157: Create a newAnytype instance and handle errors.The creation of a new
Anytype instance usinggogoprotoany.NewAnyWithCacheWithValue(resp)allows for flexible encapsulation of the response. The error handling ensures that any issues during this process are communicated clearly.
159-162: Marshal the response and handle errors.The response is marshaled using
gogoproto.Marshal(msgRespAny), and errors during this operation are handled appropriately. This ensures that any issues during the marshaling process are communicated clearly.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (8)
x/authz/CHANGELOG.md (8)
36-36: Clarify the description of thek.DispatchActionschange.The description is clear but can be made more concise by removing redundant words.
- `k.DispatchActions` returns a slice of byte slices of proto marshaled anys instead of a slice of byte slices of `sdk.Result.Data`. + `k.DispatchActions` now returns proto marshaled `any` types instead of `sdk.Result.Data`.
37-37: Ensure clarity in theAcceptmethod change description.The description is clear but can be improved for readability.
- `Accept` on the `Authorization` interface now expects the authz environment in the `context.Context`. This is already done when `Accept` is called by `k.DispatchActions`, but should be done manually if `Accept` is called directly. + The `Accept` method on the `Authorization` interface now requires the authz environment to be passed in the `context.Context`. This is handled automatically when `Accept` is called by `k.DispatchActions`, but must be done manually if `Accept` is called directly.
38-39: Improve readability of theNewMsgExec,NewMsgGrant, andNewMsgRevokechanges.The description is clear but can be made more concise.
- Removes the use of Accounts String() method - `NewMsgExec`, `NewMsgGrant` and `NewMsgRevoke` now takes strings as arguments instead of `sdk.AccAddress`. - `ExportGenesis` also returns an error. - `IterateGrants` returns an error, its handler function also returns an error. + Removed the use of the `Accounts String()` method. + `NewMsgExec`, `NewMsgGrant`, and `NewMsgRevoke` now take strings as arguments instead of `sdk.AccAddress`. + `ExportGenesis` and `IterateGrants` now return errors, and the handler function for `IterateGrants` also returns an error.
Line range hint
40-40:
Clarify theNewKeeperchange description.The description is clear but can be improved for readability.
- `NewKeeper` doesn't take a message router anymore. Set the message router in the `appmodule.Environment` instead. + `NewKeeper` no longer takes a message router. Set the message router in the `appmodule.Environment` instead.
Line range hint
41-41:
Clarify theappmodule.Environmentchange description.The description is clear but can be improved for readability.
- `appmodule.Environment` is received on the Keeper to get access to different application services. + The `appmodule.Environment` is now received by the Keeper to access different application services.
Line range hint
42-42:
Clarify theDequeueAndDeleteExpiredGrantschange description.The description is clear but can be improved for readability.
- Update the keeper method `DequeueAndDeleteExpiredGrants` to take a limit argument for the number of grants to prune. + Updated the keeper method `DequeueAndDeleteExpiredGrants` to take a limit argument for the number of grants to prune.
Line range hint
43-43:
Clarify theAcceptResponsechange description.The description is clear but can be improved for readability.
- `AcceptResponse` has been moved to sdk/types/authz and the `Updated` field is now of the type `sdk.Msg` instead of `authz.Authorization`. + Moved `AcceptResponse` to `sdk/types/authz` and changed the `Updated` field type to `sdk.Msg` instead of `authz.Authorization`.
Line range hint
44-44:
Clarify theInitGenesisandExportGenesischange description.The description is clear but can be improved for readability.
- `InitGenesis` and `ExportGenesis` module code and keeper code do not panic but return errors. + The `InitGenesis` and `ExportGenesis` module code and keeper code now return errors instead of panicking.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/authz/CHANGELOG.md (1 hunks)
Additional context used
Path-based instructions (1)
x/authz/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 54df758)
* main: (76 commits) docs: more app v2 renaming (#21336) chore: update link in disclaimer (#21339) refactor(x/distribution): audit QA (#21316) docs: rename app v2 to app di when talking about runtime v0 (#21329) feat(schema): specify JSON mapping (#21243) fix(x/authz): bring back msg response in `DispatchActions` (#21044) chore: fix all lint issue since golangci-lint bump (#21326) refactor(x/mint): v0.52 audit x/mint (#21301) chore: fix spelling errors (#21327) feat: export genesis in simapp v2 (#21199) fix(baseapp)!: Halt at height now does not produce the halt height block (#21256) refactor(scripts): remove unused variable (#21320) chore(schema/testing): upgrade to go 1.23 iterators (#21282) chore: readmes + upgrading docs (#21271) feat(client): add auto cli for node service (#21074) ci: fix github workflow vulnerable to script injection (#21304) build(deps): Bump github.com/prometheus/client_golang from 1.19.1 to 1.20.0 (#21307) build(deps): use Go 1.23 instead of Go 1.22 (#21280) refactor(x/auth): audit x/auth changes (#21146) chore: remove todo: "abstract out staking message back to staking" (#21266) ...
Description
k.DispatchActionswas always returning an empty byte array after the migration to the msg service routerIn v0.50.8 we were writing (deprecated)
msgResponse.Datain that[][]byteand no marshalled anys so I'll add a test and a changelog to prevent regressions.https://github.com/cosmos/cosmos-sdk/blob/v0.50.8/x/authz/keeper/keeper.go#L166-L186
cc @danieljdd
Should be backported to v0.52 after #21041 is merged
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