-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(proto): Add proto messages for x/protocolpool Funds Handling #18427
Conversation
Walkthrough<details>
<summary>Walkthrough</summary>
## Walkthrough
The updates expand the functionality of the system by introducing new RPC methods and message types to manage continuous funds. These changes enable the creation, funding, claiming, and cancellation of continuous funds, along with a proposal mechanism. Additionally, there's a minor update to the casing of fields in an existing message type.
## Changes
| File Path | Change Summary |
|-----------|----------------|
| `proto/cosmos/protocolpool/v1/tx.proto` | Introduced new RPC methods, message types, and updated field casing. |
| `x/protocolpool/keeper/msg_server.go` | Added new methods to handle continuous fund operations. |
</details> TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- api/cosmos/protocolpool/v1/tx_grpc.pb.go
- x/protocolpool/types/tx.pb.go
Files selected for processing (2)
- proto/cosmos/protocolpool/v1/tx.proto (2 hunks)
- x/protocolpool/keeper/msg_server.go (1 hunks)
Additional comments: 4
proto/cosmos/protocolpool/v1/tx.proto (4)
29-41: The new RPC methods
AddContinuousFunds
,FundDispensationProposal
, andCancelContinuousFundProposal
have been added to theMsg
service. Ensure that these methods are implemented in theMsgServer
struct inx/protocolpool/keeper/msg_server.go
.114-136: The
MsgAddContinuousFunds
message has been defined with fields for title, description, recipient address, metadata, percentage, and expiry. Ensure that the percentage field is validated to be within the range of 0 to 100.142-154: The
MsgFundDispensationProposal
message has been defined with fields for authority, recipient address, percentage, cap, and expiration. Ensure that the percentage field is validated to be within the range of 0 to 100.160-168: The
MsgCancelContinuousFundProposal
message has been defined with fields for authority and recipient address. Ensure that the recipient address is validated to exist in the list of continuous fund recipients.
@likhita-809 your pull request is missing a changelog! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- api/cosmos/protocolpool/v1/tx_grpc.pb.go
- x/protocolpool/types/tx.pb.go
Files selected for processing (1)
- proto/cosmos/protocolpool/v1/tx.proto (2 hunks)
Additional comments: 2
proto/cosmos/protocolpool/v1/tx.proto (2)
29-38: The new RPC methods
AddContinuousFunds
andCancelContinuousFundProposal
are added correctly to theMsg
service. Ensure that these methods are implemented in the corresponding server code.155-160: The
MsgCancelContinuousFundProposalResponse
message is well defined. Thecanceled_time
andcanceled_height
fields provide useful information about when and at what block height the cancellation occurred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ita/18374-proto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @likhita-809! Just some comments that we should capitalize the field names in the comments so that they'll match the generated field names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- api/cosmos/protocolpool/v1/tx_grpc.pb.go
- x/protocolpool/types/tx.pb.go
Files selected for processing (1)
- proto/cosmos/protocolpool/v1/tx.proto (4 hunks)
Additional comments: 3
proto/cosmos/protocolpool/v1/tx.proto (3)
34-38: The method names
CreateContinuousFund
andCancelContinuousFundProposal
are repeated in the comments, which might be a copy-paste error. Verify if the second method should beFundDispensationProposal
as per the summary provided.152-152: The concern raised by anilCSE about not being able to have two funds streaming for the same address at a time is addressed by tac0turtle, stating that this is expected behavior. However, it's important to ensure that this limitation is clearly documented and understood by users of the protocol to avoid confusion.
157-163: The response message
MsgCancelContinuousFundProposalResponse
includes arecipient_address
field (line 163). This aligns with anilCSE's suggestion to include fund details in the response. Ensure that the implementation of the server-side logic populates this field correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/protocolpool/keeper/msg_server.go (1 hunks)
Additional comments: 3
x/protocolpool/keeper/msg_server.go (3)
106-107: The
CreateContinuousFund
function currently returns an empty response without performing any operations. This should be implemented to handle the creation of continuous funds as described in the pull request summary.110-111: Similarly, the
CancelContinuousFundProposal
function returns an empty response without any logic. This function should be implemented to handle the cancellation of continuous fund proposals.114-116: The
validateAuthority
function checks if the provided authority address is valid and matches the expected authority. This is a good security practice to ensure that only authorized entities can perform certain actions.The rest of the code in the provided hunk seems to be handling various other fund-related operations and includes proper validation and error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/protocolpool/types/tx.pb.go
Files selected for processing (1)
- proto/cosmos/protocolpool/v1/tx.proto (4 hunks)
Additional comments: 5
proto/cosmos/protocolpool/v1/tx.proto (5)
29-38: The addition of
CreateContinuousFund
andCancelContinuousFundProposal
RPC methods expands the functionality of the protocol pool module. Ensure that corresponding server-side implementations and client-side usage are updated to handle these new methods.111-138: The
MsgCreateContinuousFund
message is well-defined with clear fields for title, description, authority, recipient, metadata, percentage, cap, and expiry. The use ofcosmossdk.io/math.LegacyDec
for thepercentage
field is appropriate for representing decimal values. Ensure that the handling of this custom type is consistent across the system.145-163: The
MsgCancelContinuousFundProposal
message includes necessary fields for cancellation such as authority and recipient_address. The response message includes a timestamp and block height which are good for tracking the cancellation event. Ensure that the cancellation logic is robust and handles edge cases, such as attempting to cancel a non-existent or already completed fund.152-152: The previous comment by
anilCSE
suggests that there should be a unique key for each fund to allow for individual fund management. The response bytac0turtle
indicates that this is not necessary as the design expects one fund stream per address. This design decision should be clearly documented and communicated to users to avoid confusion.157-157: The suggestion by
anilCSE
to add fund details to the response message is valid. Including therecipient_address
in theMsgCancelContinuousFundProposalResponse
message would provide clarity on which fund was cancelled. This could be considered for future iterations if it aligns with the design goals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- api/cosmos/protocolpool/v1/tx_grpc.pb.go
- x/protocolpool/types/tx.pb.go
Files selected for processing (1)
- proto/cosmos/protocolpool/v1/tx.proto (4 hunks)
Additional comments: 2
proto/cosmos/protocolpool/v1/tx.proto (2)
29-38: The addition of
CreateContinuousFund
andCancelContinuousFund
RPC methods is a significant enhancement to the protocol pool functionality. Ensure that corresponding backend logic and validation are implemented to handle these new requests.111-137: The
MsgCreateContinuousFund
message includes a comprehensive set of fields for fund management. Ensure that thepercentage
field is correctly handled as a decimal and that thecap
andexpiry
fields are validated to prevent misuse or errors in fund creation.
…ita/18374-proto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/protocolpool/keeper/msg_server.go (1 hunks)
Additional comments: 1
x/protocolpool/keeper/msg_server.go (1)
- 114-116: The
validateAuthority
function checks if the provided authority address is valid but does not seem to check if the authority has the required permissions to perform the action. Ensure that the necessary permission checks are in place.The check against
k.authority
in the subsequent lines (not shown in the hunk) should be reviewed to ensure thatk.authority
is being set correctly elsewhere in the code, as it is being used to validate the authority here.
Description
ref: #18374
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Reviewers 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...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Documentation
Refactor