Fix/query sign requests broadcast type #1127
Conversation
Null value means "query sign request with all broadcast types"
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/cases/warden_precompile_action.go (2)
Line range hint
250-273: Avoid adding+1toBroadcastTypeenum valuesAdding
+1toBroadcastTypeenum values to represent optionality can be confusing and error-prone. Consider refactoring to handle optional broadcast types more explicitly.
Line range hint
218-301: Refactor duplicated code into helper functionsThe code for creating and asserting new sign requests with different broadcast types is duplicated. Refactor this logic into reusable helper functions to improve maintainability and reduce redundancy.
precompiles/warden/IWarden.sol (1)
116-120: LGTM! Well-structured enum for optional broadcast types.The new
OptionalBroadcastTypeenum is well-designed withNonspecifiedas the default (0), maintaining compatibility with the existingBroadcastTypevalues.Consider documenting the enum values and their intended usage in comments, especially the relationship between
OptionalBroadcastTypeandBroadcastType.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
api/warden/warden/v1beta3/query.pulsar.gois excluded by!api/**warden/x/warden/types/v1beta3/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
precompiles/warden/IWarden.go(5 hunks)precompiles/warden/IWarden.sol(2 hunks)precompiles/warden/abi.json(1 hunks)precompiles/warden/query_types.go(1 hunks)proto/warden/warden/v1beta3/query.proto(2 hunks)tests/cases/warden_precompile_action.go(5 hunks)warden/x/warden/keeper/query_sign_requests.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
warden/x/warden/keeper/query_sign_requests.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/cases/warden_precompile_action.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
precompiles/warden/query_types.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
precompiles/warden/IWarden.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (2)
precompiles/warden/IWarden.sol (1)
Learnt from: Svetomech
PR: warden-protocol/wardenprotocol#1042
File: precompiles/warden/abi.json:1633-1637
Timestamp: 2024-11-14T15:15:53.719Z
Learning: In the `precompiles/warden/abi.json` file, the `broadcastType` parameter in the `newSignRequest` method is intentionally defined as `enum BroadcastType` (uint8), even though it's defined as `int32` in other locations.
precompiles/warden/abi.json (1)
Learnt from: Svetomech
PR: warden-protocol/wardenprotocol#1042
File: precompiles/warden/abi.json:1633-1637
Timestamp: 2024-11-14T15:15:53.719Z
Learning: In the `precompiles/warden/abi.json` file, the `broadcastType` parameter in the `newSignRequest` method is intentionally defined as `enum BroadcastType` (uint8), even though it's defined as `int32` in other locations.
🔇 Additional comments (6)
precompiles/warden/IWarden.go (2)
29-29: Approved: Added reference to suppress unused import errors
The addition of _ = abi.ConvertType correctly suppresses unused import errors.
240-244: Approved: Use of GetAbi() enhances ABI handling
Switching to IWardenMetaData.GetAbi() improves maintainability and reduces potential errors from manual ABI string parsing.
warden/x/warden/keeper/query_sign_requests.go (1)
35-35: Approved: Proper handling of optional broadcast type
The updated condition correctly checks if OptionalBroadcastType is nil before comparing, ensuring accurate filtering logic.
proto/warden/warden/v1beta3/query.proto (1)
185-189: Approved: Use of oneof for optional_broadcast_type
The oneof construct appropriately allows broadcast_type to be optional, enhancing flexibility in request handling.
precompiles/warden/IWarden.sol (1)
440-447: LGTM! Parameter type change enhances query flexibility.
The change from BroadcastType to OptionalBroadcastType in the signRequests function allows for more flexible queries by making the broadcast type optional.
Let's verify that all callers of this function are updated:
✅ Verification successful
Parameter type change appears safe to proceed
Based on the codebase analysis:
- The
signRequestsfunction is only defined in the interface (IWarden.sol) - No direct calls to this function were found in the codebase
- The mock implementation (
MockWardenPrecompile.sol) doesn't implement this function - The only usage of
IWardenis inBasicOrder.solwhich doesn't call this specific function
The change from BroadcastType to OptionalBroadcastType appears to be an interface-only modification with no impact on existing implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for function calls to signRequests to ensure they're updated
rg -A 2 'signRequests.*BroadcastType' --type go --type solidity
Length of output: 98
Script:
#!/bin/bash
# Search for function calls to signRequests with proper file type
rg -t sol 'signRequests\(' -A 2
Length of output: 61
Script:
#!/bin/bash
# First, find all Solidity files
fd '\.sol$'
# Then search for signRequests function calls in those files
rg 'signRequests\(' --type-add 'sol:*.sol' -t sol -A 2
Length of output: 869
Script:
#!/bin/bash
# Let's check for any implementations or usages of this interface
rg -B 2 -A 5 'IWarden' --type-add 'sol:*.sol' -t sol
Length of output: 5623
Script:
#!/bin/bash
# Let's check the BasicOrder.sol file for any calls to signRequests
rg -B 5 -A 5 'signRequests' --type-add 'sol:*.sol' -t sol solidity/orders/src/BasicOrder.sol
# Also check the mock implementation for any relevant code
rg -B 5 -A 5 'signRequests' --type-add 'sol:*.sol' -t sol solidity/orders/mocks/MockWardenPrecompile.sol
Length of output: 197
precompiles/warden/abi.json (1)
1929-1930: LGTM! ABI changes correctly reflect the interface modifications.
The ABI correctly defines the new parameter type as enum OptionalBroadcastType while maintaining the underlying uint8 type, consistent with Solidity's enum representation.
precompiles/warden/query_types.go
Outdated
| if input.OptionalBroadcastType != 0 { | ||
| if _, ok := types.BroadcastType_name[int32(input.OptionalBroadcastType-1)]; !ok { | ||
| return nil, fmt.Errorf("invalid BroadcastType value: %d", input.OptionalBroadcastType-1) | ||
| } | ||
|
|
||
| broadcastType := types.QuerySignRequestsRequest_BroadcastType{ | ||
| BroadcastType: types.BroadcastType(int32(input.OptionalBroadcastType - 1)), | ||
| } | ||
| return &types.QuerySignRequestsRequest{ | ||
| Pagination: &input.PageRequest, | ||
| KeychainId: input.KeychainId, | ||
| Status: types.SignRequestStatus(int32(input.Status)), | ||
| OptionalBroadcastType: &broadcastType, | ||
| }, nil | ||
| } else { | ||
| return &types.QuerySignRequestsRequest{ | ||
| Pagination: &input.PageRequest, | ||
| KeychainId: input.KeychainId, | ||
| Status: types.SignRequestStatus(int32(input.Status)), | ||
| OptionalBroadcastType: nil, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid adjusting OptionalBroadcastType by subtracting 1
Subtracting 1 from OptionalBroadcastType to align with the BroadcastType enum can lead to confusion and potential errors. Consider using a more explicit approach to represent optional broadcast types, such as defining a sentinel value or using pointers.
| @@ -113,6 +113,12 @@ enum BroadcastType { | |||
| Automatic | |||
| } | |||
|
|
|||
| enum OptionalBroadcastType { | |||
There was a problem hiding this comment.
Tbh, if we just made BroadcastType to have Unspecified value initially, this problem wouldn't occur
There was a problem hiding this comment.
Agree, but it will require migration for existing sign requests
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
56-56: Improve changelog entry format and clarityThe changelog entry needs some improvements:
- Fix grammar: "return" should be "returns"
- Add GitHub issue reference as per guidelines
- Clarify the change's impact
Consider revising to:
- * (x/warden) Sign requests query return all request (not only with broadcastType=BroadcastType.Disabled) + * (x/warden) #1127 Sign requests query returns all requests (previously filtered to only broadcastType=BroadcastType.Disabled)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)precompiles/warden/query_types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- precompiles/warden/query_types.go
🧰 Additional context used
📓 Path-based instructions (1)
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Summary by CodeRabbit
New Features
OptionalBroadcastTypeenum for more flexible handling of broadcast types in sign requests.signRequestsfunction to accept the new optional broadcast type parameter.Bug Fixes
Tests
Documentation
signRequestsfunction to clarify the new optional parameter.