add expected approval&reject expressions to MsgNewAction #791
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughWalkthroughThe changes introduce new fields to the Changes
Possibly related issues
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
|
This comment has been minimized.
This comment has been minimized.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
![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, codebase verification and nitpick comments (2)
CHANGELOG.md (2)
58-58: Enhance the description forMsgAddActionchanges.The entry for the addition of expected expressions to
MsgAddActioncould benefit from a more detailed description. It would be helpful to explain what "expected expressions" are and how they contribute to the functionality.
58-58: Clarify the impact of non-nullableKeychainFeesfields.While the entry mentions making
KeychainFeesfields non-nullable, it would be beneficial to include information on the potential impact this change may have on existing data and any necessary migrations or updates required to accommodate this change.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
api/warden/act/v1beta1/tx.pulsar.gois excluded by!api/**warden/x/act/types/v1beta1/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- proto/warden/act/v1beta1/tx.proto (2 hunks)
- warden/x/act/keeper/actions.go (3 hunks)
- warden/x/act/keeper/msg_server_new_action.go (1 hunks)
Additional context used
Path-based instructions (3)
warden/x/act/keeper/msg_server_new_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.CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (6)
warden/x/act/keeper/msg_server_new_action.go (1)
18-18: Good error handling and structure.The function properly wraps errors with additional context, making debugging easier. The use of
fmt.Errorfis consistent with best practices in Go:return nil, fmt.Errorf("can't unpack any: %w", err)The overall structure of the function is clear and follows the Uber Golang style guide.
proto/warden/act/v1beta1/tx.proto (2)
12-12: Approved import statement.The import of
shield/ast/ast.protois necessary for the new fields inMsgNewActionthat utilize.shield.ast.Expression.
72-75: Approved new fields inMsgNewAction.The addition of
expected_approval_expressionandexpected_reject_expressionenhances the functionality of theMsgNewActionmessage by allowing it to encapsulate additional logic for action approvals and rejections. Ensure that these new fields are properly integrated and utilized in the system's action handling logic.Run the following script to verify the integration of the new fields:
Verification successful
Verified integration of new fields in
MsgNewAction.The fields
expected_approval_expressionandexpected_reject_expressionare successfully integrated into the system's logic, as evidenced by their presence and usage in multiple parts of the codebase. This confirms their proper incorporation into the action handling logic.
- Locations where the fields are integrated:
warden/x/act/types/v1beta1/tx.pb.goapi/warden/act/v1beta1/tx.pulsar.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of new fields in the system's action handling logic. # Test: Search for the usage of new fields. Expect: Proper integration in the system. rg --type proto -A 5 $'expected_approval_expression|expected_reject_expression'Length of output: 110
Script:
#!/bin/bash # Description: Verify the integration of new fields in the system's action handling logic. # Test: Search for the usage of new fields across the entire codebase. rg 'expected_approval_expression|expected_reject_expression' -A 5Length of output: 10178
warden/x/act/keeper/actions.go (2)
137-137: Review the expanded function signature.The function
AddActionnow includes parameters forexpectedApprovalExpressionandexpectedRejectExpression. This change aligns with the PR objectives to enhance action handling with expressions for approvals and rejections.
Line range hint
137-149: Ensure robust handling of new parameters.The integration of
expectedApprovalExpressionandexpectedRejectExpressioninto the action creation process is crucial. It's important to ensure that these expressions are correctly processed and applied to the action's rule. Consider adding unit tests to verify the correct application of these expressions.CHANGELOG.md (1)
Line range hint
1-58: Approve the structure and formatting of the changelog.The changelog is well-structured and adheres to the recommended format from Keep a Changelog. The entries are appropriately grouped and tagged, facilitating easy navigation and understanding.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
warden/x/act/keeper/actions.go (1)
Line range hint
137-149: Complexity and Maintainability ofAddAction.The
AddActionfunction has significantly increased in complexity with the addition of new logic for handling expressions. This includes interactions withrulesRegistryand preprocessing of rules, which adds several layers of logic that need careful testing.
- Refactor Suggestion: Consider breaking down this function into smaller, more manageable functions that handle specific parts of the action creation process. This can improve maintainability and make the code easier to understand and test.
- Performance: Ensure that the preprocessing and rule evaluation do not introduce performance bottlenecks, especially in contexts where actions are created frequently.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
api/warden/act/v1beta1/tx.pulsar.gois excluded by!api/**warden/x/act/types/v1beta1/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- proto/warden/act/v1beta1/tx.proto (2 hunks)
- warden/x/act/keeper/actions.go (3 hunks)
- warden/x/act/keeper/msg_server_new_action.go (1 hunks)
Additional context used
Path-based instructions (3)
warden/x/act/keeper/msg_server_new_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.CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (6)
proto/warden/act/v1beta1/tx.proto (2)
12-12: Approved import statement.The import of
shield/ast/ast.protois necessary for the new fields inMsgNewActionto reference the.shield.ast.Expressiontype.
72-75: Approved new fields inMsgNewAction.The addition of
expected_approval_expressionandexpected_reject_expressionenhances the message's functionality by allowing it to encapsulate more complex logic for action approvals and rejections. Ensure that the rest of the system integrates these changes correctly.Run the following script to verify the integration:
warden/x/act/keeper/actions.go (2)
137-137: Review the addition of new parameters inAddAction.The function signature of
AddActionhas been expanded to includeexpectedApprovalExpressionandexpectedRejectExpression. This change aligns with the PR objectives to enhance action creation logic by allowing for more complex decision-making based on these expressions.
- Correctness: The parameters are correctly typed as
ast.Expression, which is consistent with handling expressions in this context.- Best Practice: Including these parameters directly in the function signature enhances the clarity and explicitness of what
AddActionexpects as inputs, adhering to the principle of explicit interfaces.
Line range hint
137-149: Ensure robust error handling and validation.The function performs multiple operations that can fail, such as message unpacking, rule retrieval, and action persistence. Each of these steps includes error handling, but it's crucial to ensure that these are handled as expected across different scenarios.
- Verification Needed: Additional tests should be written to cover scenarios where each of these steps might fail, ensuring that the function behaves correctly under error conditions.
- Security: Validate that the expressions provided do not allow for injection of malicious code or execution of unintended actions.
CHANGELOG.md (2)
58-58: Review the entry for clarity and accuracy.The entry for
MsgNewActionunder "Consensus Breaking Changes" correctly links to the GitHub issue and describes the addition of expected expressions. However, ensure that the description is clear about how these expressions affect the action creation logic, enhancing the understanding for readers who might not be familiar with the context.
Line range hint
1-58: Confirm documentation correctness and completeness.The documentation in
CHANGELOG.mdis well-structured and adheres to the guidelines from Keep a Changelog and Semantic Versioning. No grammatical errors or misspellings were detected, and the documentation appears complete, accurately reflecting the changes made in the PR.
| @@ -15,7 +15,7 @@ func (k msgServer) NewAction(ctx context.Context, msg *types.MsgNewAction) (*typ | |||
| return nil, fmt.Errorf("can't unpack any: %w", err) | |||
| } | |||
|
|
|||
| act, err := k.AddAction(ctx, msg.Creator, message, msg.ActionTimeoutHeight) | |||
| act, err := k.AddAction(ctx, msg.Creator, message, msg.ActionTimeoutHeight, *msg.ExpectedApprovalExpression, *msg.ExpectedRejectExpression) | |||
There was a problem hiding this comment.
Ensure safe handling of pointer dereferences.
The function AddAction is called with dereferenced pointers *msg.ExpectedApprovalExpression and *msg.ExpectedRejectExpression. It's crucial to ensure that these fields are not nil before dereferencing to avoid runtime panics. Consider adding a check before this line to ensure that both fields are non-nil.
Here's a suggested change to add nil checks:
+ if msg.ExpectedApprovalExpression == nil || msg.ExpectedRejectExpression == nil {
+ return nil, fmt.Errorf("expected expressions must not be nil")
+ }
act, err := k.AddAction(ctx, msg.Creator, message, msg.ActionTimeoutHeight, *msg.ExpectedApprovalExpression, *msg.ExpectedRejectExpression)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.
| act, err := k.AddAction(ctx, msg.Creator, message, msg.ActionTimeoutHeight, *msg.ExpectedApprovalExpression, *msg.ExpectedRejectExpression) | |
| if msg.ExpectedApprovalExpression == nil || msg.ExpectedRejectExpression == nil { | |
| return nil, fmt.Errorf("expected expressions must not be nil") | |
| } | |
| act, err := k.AddAction(ctx, msg.Creator, message, msg.ActionTimeoutHeight, *msg.ExpectedApprovalExpression, *msg.ExpectedRejectExpression) |
| @@ -144,6 +145,8 @@ func (k Keeper) AddAction(ctx context.Context, creator string, msg sdk.Msg, time | |||
| return nil, errors.Wrapf(types.ErrNoRuleRegistryHandler, "%v", err) | |||
| } | |||
|
|
|||
| // todo: check that expressions from rulesRegistry (templateRegistry) match with expected | |||
There was a problem hiding this comment.
TODO comment needs addressing or removal.
There is a todo comment that suggests a future implementation detail about checking expressions from the rules registry. This indicates that the implementation may not be complete.
- Action Required: It's important to either implement the necessary checks or update the comment to reflect any changes made. Leaving TODOs in production code can lead to technical debt if not tracked properly.
mn13
force-pushed
the
773-expected-expressions-to-msg-new-action
branch
from
b2d91ed
to
3862601
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
api/warden/act/v1beta1/tx.pulsar.gois excluded by!api/**warden/x/act/types/v1beta1/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- proto/warden/act/v1beta1/tx.proto (2 hunks)
- warden/x/act/keeper/actions.go (3 hunks)
- warden/x/act/keeper/msg_server_new_action.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- proto/warden/act/v1beta1/tx.proto
- warden/x/act/keeper/actions.go
- warden/x/act/keeper/msg_server_new_action.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (3)
api/warden/act/v1beta1/tx.pulsar.gois excluded by!api/**warden/x/act/types/v1beta1/tx.pb.gois excluded by!**/*.pb.gowarden/x/warden/types/v1beta3/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (2)
- proto/warden/act/v1beta1/tx.proto (2 hunks)
- warden/x/act/keeper/msg_server_new_action.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- proto/warden/act/v1beta1/tx.proto
- warden/x/act/keeper/msg_server_new_action.go
mn13
force-pushed
the
773-expected-expressions-to-msg-new-action
branch
from
19abc7e
to
bebb7a3
Compare
mn13
force-pushed
the
773-expected-expressions-to-msg-new-action
branch
from
bebb7a3
to
7bdc1e0
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Documentation