[x/act V2]: Add MsgVote handling #794
Conversation
WalkthroughWalkthroughThe changes introduce new types and methods to enhance the voting mechanism for actions within the Changes
Possibly related issues
Tip New featuresWalkthrough comment now includes:
Notes:
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
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Hey @artur-abliazimov and thank you for opening this pull request! 👋🏼 It looks like you forgot to add a changelog entry for your changes. Make sure to add a changelog entry in the 'CHANGELOG.md' file. |
artur-abliazimov
changed the title
warden/x/act/keeper/actions.go
Outdated
| @@ -30,6 +30,73 @@ func (approvers ApproversEnv) Get(name string) (object.Object, bool) { | |||
|
|
|||
| var _ shield.Environment = ApproversEnv{} | |||
There was a problem hiding this comment.
@Pitasi why do we need this here?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- warden/x/act/keeper/actions.go (1 hunks)
- warden/x/act/keeper/msg_server_vote_for_action.go (1 hunks)
- warden/x/act/types/v1beta1/action.go (2 hunks)
- warden/x/act/types/v1beta1/errors.go (1 hunks)
Additional context used
Path-based instructions (4)
warden/x/act/keeper/msg_server_vote_for_action.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/act/types/v1beta1/errors.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/act/types/v1beta1/action.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/act/keeper/actions.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (10)
warden/x/act/keeper/msg_server_vote_for_action.go (8)
11-11: Context unwrapping is correctly implemented.The use of
sdk.UnwrapSDKContext(goCtx)is appropriate for extracting the SDK context from the Go context, which is a standard practice in Cosmos SDK modules.
12-16: Proper error handling for action retrieval.The function correctly handles the scenario where an action cannot be retrieved, which is crucial for robust error management. Returning the error directly is a good practice as it allows the caller to handle it appropriately.
18-24: Timeout condition and status update are well-handled.The check for
act.TimeoutHeightand the subsequent status update toACTION_STATUS_TIMEOUTare well implemented. The error handling within this block ensures that any failure in updating the action's status or saving it back to the keeper results in an early return with an error, which is a good practice.
26-29: Clear and concise response construction for timeout scenario.The response construction using
types.MsgVoteForActionResponsewith the action's status is clear and follows good coding practices by directly mapping the status to its string representation.
31-33: Error handling for adding a vote.The function
AddVoteis called with proper error handling. If adding a vote fails, the function returns immediately with the error, which is a good practice to prevent further execution with an invalid state.
35-37: Error handling for updating the action keeper.After adding a vote, the action is updated in the keeper. The error handling here is consistent with the rest of the function, ensuring that any issues during the save operation prevent further execution.
39-41: Error handling for executing the voted action.The function
TryExecuteVotedActionis called, and any errors are handled appropriately by returning them. This ensures that execution errors do not silently fail and are reported up the call stack.
43-43: Final response construction is correct and concise.The final response is constructed correctly, providing the current status of the action. This is a clean and effective way to communicate the result of the voting process back to the caller.
warden/x/act/types/v1beta1/errors.go (1)
26-26: Well-defined new error variable.The addition of
ErrActionVoteAlreadyExistsis well-integrated within the existing error definitions. The use of a unique error code and a clear, descriptive message aligns with best practices for error handling in the module.warden/x/act/types/v1beta1/action.go (1)
18-24: Review of theNewVotefunctionThe
NewVotefunction is well-implemented and follows the Go best practices. It initializes anActionVotestruct with the provided parameters. The function is concise and effectively encapsulates the creation of a vote object.
- Correctness: The function correctly assigns the
participant,vote, andtimestampto the respective fields in theActionVotestruct.- Maintainability: The function is simple and straightforward, making it easy to maintain.
- Performance: There are no apparent performance issues as the function simply constructs an object.
Overall, this function is approved as it meets the requirements and follows good coding practices.
Co-authored-by: Max Nabokov <[email protected]>
I do not think that the initial approver must be set during action creation. AddVote is added separately @Pitasi what do you think? |
i think that 99% of the times if I'm creating an action I also want to approve it, so it makes sense to save some time and gas and do it |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- warden/x/act/keeper/actions.go (3 hunks)
- warden/x/act/keeper/msg_server_vote_for_action.go (1 hunks)
- warden/x/act/types/v1beta1/action.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- warden/x/act/keeper/actions.go
Additional context used
Path-based instructions (2)
warden/x/act/keeper/msg_server_vote_for_action.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/act/types/v1beta1/action.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
warden/x/act/types/v1beta1/action.go (1)
18-24: Constructor functionNewVoteis correctly implemented.The
NewVotefunction is straightforward and correctly sets upActionVoteinstances. It follows best practices for constructor functions in Go.
Co-authored-by: Antonio Pitasi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- warden/x/act/types/v1beta1/action.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- warden/x/act/types/v1beta1/action.go
Co-authored-by: Antonio Pitasi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- warden/x/act/keeper/msg_server_vote_for_action.go (1 hunks)
Additional context used
Path-based instructions (1)
warden/x/act/keeper/msg_server_vote_for_action.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
GitHub Check: lint
warden/x/act/keeper/msg_server_vote_for_action.go
[failure] 48-48:
undefined: fmt (typecheck)
[failure] 48-48:
undefined: fmt) (typecheck)
[failure] 48-48:
undefined: fmt) (typecheck)
[failure] 48-48:
undefined: fmt) (typecheck)
GitHub Check: test
warden/x/act/keeper/msg_server_vote_for_action.go
[failure] 48-48:
undefined: fmt
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| act, err := k.ActionKeeper.Get(ctx, msg.ActionId) | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if act.TimeoutHeight > 0 && act.TimeoutHeight < uint64(ctx.BlockHeight()) { | ||
| if err := act.SetStatus(ctx, types.ActionStatus_ACTION_STATUS_TIMEOUT); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := k.ActionKeeper.Set(ctx, act); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &types.MsgVoteForActionResponse{ | ||
| Status: act.Status.String(), | ||
| }, nil | ||
| } | ||
|
|
||
| if err := act.AddOrUpdateVote(ctx, msg.Participant, msg.VoteType); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := k.ActionKeeper.Set(ctx, act); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if msg.VoteType == types.ActionVoteType_VOTE_TYPE_APPROVED { | ||
| if err := k.TryExecuteVotedAction(ctx, &act); err != nil { | ||
| return nil, err | ||
| } | ||
| } else if msg.VoteType == types.ActionVoteType_VOTE_TYPE_REJECTED { | ||
| if err := k.TryRejectVotedAction(ctx, &act); err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| return nil, fmt.Errorf("unhandled VoteType value: %v", msg.VoteType) | ||
| } | ||
|
|
||
| return &types.MsgVoteForActionResponse{Status: act.Status.String()}, nil |
There was a problem hiding this comment.
Comprehensive Review of VoteForAction Function
The VoteForAction function has been significantly updated to handle the complete logic flow for voting on actions. Here are the key observations and suggestions:
-
Error Handling and Logic Flow:
- The function correctly handles errors at multiple points, ensuring that any failure in retrieving actions or updating their status results in an immediate return of the error (lines 14-15, 19-20, 22-23, 32, 36, 41, 45). This is a good practice as it prevents further execution when an error state is detected.
-
Handling of Timeout:
- The logic to handle action timeouts is well implemented (lines 18-29). If the action has timed out, it updates the action's status and returns a response indicating the timeout status. This is crucial for maintaining the integrity of the action's lifecycle.
-
Vote Handling and Action Execution:
- The function handles different types of votes and attempts to execute or reject the action based on the vote type (lines 39-47). This is a critical part of the function, ensuring that actions are processed according to the collective decision of the participants.
-
Performance Consideration:
- As previously noted by another reviewer, the function makes multiple calls to
ActionKeeper.Set(lines 22, 35). While necessary, it's important to review whether these calls can be optimized or batched to reduce overhead, especially in a blockchain context where such operations can be costly.
- As previously noted by another reviewer, the function makes multiple calls to
-
Error Handling Improvement:
- The function lacks the import for
fmt, which is used in line 48. This results in a compilation error when handling unhandledVoteTypevalues. This needs to be addressed by adding the appropriate import statement.
- The function lacks the import for
-
Code Style and Best Practices:
- The code adheres well to the Uber Golang style guide, with clear error handling and separation of concerns. However, the missing import highlights a need for better static analysis tool integration or review processes to catch such issues before PR submission.
Suggestions:
- Consider optimizing the multiple
Setcalls if possible to improve performance. - Add the missing
fmtimport to fix the compilation error.
Tools
GitHub Check: lint
[failure] 48-48:
undefined: fmt (typecheck)
[failure] 48-48:
undefined: fmt) (typecheck)
[failure] 48-48:
undefined: fmt) (typecheck)
[failure] 48-48:
undefined: fmt) (typecheck)
GitHub Check: test
[failure] 48-48:
undefined: fmt
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- warden/x/act/keeper/msg_server_vote_for_action.go (1 hunks)
Additional context used
Path-based instructions (1)
warden/x/act/keeper/msg_server_vote_for_action.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (4)
warden/x/act/keeper/msg_server_vote_for_action.go (4)
12-17: Robust error handling for action retrieval.The function correctly handles errors when retrieving the action, ensuring that any failure results in an immediate return of the error. This is a good practice as it prevents further execution when an error state is detected.
19-25: Proper handling of action timeouts.The logic to handle action timeouts is well implemented. If the action has timed out, it updates the action's status and returns a response indicating the timeout status. This is crucial for maintaining the integrity of the action's lifecycle.
32-38: Vote handling and action execution logic.The function handles different types of votes and attempts to execute or reject the action based on the vote type. This is a critical part of the function, ensuring that actions are processed according to the collective decision of the participants.
49-50: Consider handling unhandledVoteTypevalues.The function includes a default case for unhandled
VoteTypevalues, which is a good practice for robustness. However, consider documenting potentialVoteTypevalues that might lead to this error to aid in debugging and future maintenance.
https://github.com/orgs/warden-protocol/projects/12/views/1?pane=issue&itemId=78574759
Summary by CodeRabbit
New Features
Bug Fixes