feat(x/accounts): On-chain multisig #19988
Conversation
WalkthroughWalkthroughThe recent updates focus on expanding multisig account capabilities within a Cosmos SDK application. These changes introduce new features for creating, managing, and testing multisig accounts. They also enhance the ability to update configurations and handle proposals. Additionally, improvements in context handling across modules establish a unified method for setting the sender context, enhancing consistency in system interactions. Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
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 Configration 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: 1
Out of diff range and nitpick comments (3)
x/accounts/defaults/multisig/update_config.go (1)
11-59: The logic for updating the multisig configuration and handling member updates is well-implemented. Consider adding more detailed comments to explain the validation logic and the conditions under which members are added or removed.x/accounts/internal/implementation/context.go (1)
73-77: The implementation ofSetSendercorrectly updates the sender in the context. Consider adding a comment to explain the importance of this operation, especially in a multisig context where the correct sender identity is crucial.x/accounts/defaults/multisig/account.go (1)
44-341: The implementation of multisig account management in this file is comprehensive and robust. The methods for initializing accounts, managing proposals, and handling votes are well-structured. Consider adding more detailed comments to explain the logic in complex methods, especially those involving proposal execution and voting validation.
| &v1.MsgInit{ | ||
| Config: nil, | ||
| Members: []*v1.Member{ | ||
| { | ||
| Address: "addr1", | ||
| Weight: 500, | ||
| }, | ||
| }, | ||
| }, | ||
| "config must be specified", | ||
| }, | ||
| { | ||
| "member weight zero", | ||
| &v1.MsgInit{ | ||
| Config: &v1.Config{ | ||
| Threshold: 666, | ||
| Quorum: 400, | ||
| VotingPeriod: 60, | ||
| }, | ||
| Members: []*v1.Member{ | ||
| { | ||
| Address: "addr1", | ||
| Weight: 0, | ||
| }, | ||
| }, | ||
| }, | ||
| "member weight must be greater than zero", | ||
| }, | ||
| { | ||
| "threshold is zero", | ||
| &v1.MsgInit{ | ||
| Config: &v1.Config{ | ||
| Threshold: 0, | ||
| Quorum: 400, | ||
| VotingPeriod: 60, | ||
| }, | ||
| Members: []*v1.Member{ | ||
| { | ||
| Address: "addr1", | ||
| Weight: 500, | ||
| }, | ||
| }, | ||
| }, | ||
| "threshold, quorum and voting period must be greater than zero", | ||
| }, | ||
| { | ||
| "threshold greater than total weight", | ||
| &v1.MsgInit{ | ||
| Config: &v1.Config{ | ||
| Threshold: 2000, | ||
| Quorum: 400, | ||
| VotingPeriod: 60, | ||
| }, | ||
| Members: []*v1.Member{ | ||
| { | ||
| Address: "addr1", | ||
| Weight: 500, | ||
| }, | ||
| { | ||
| Address: "addr2", | ||
| Weight: 1000, | ||
| }, | ||
| }, | ||
| }, | ||
| "threshold must be less than or equal to the total weight", | ||
| }, | ||
| { | ||
| "quorum greater than total weight", | ||
| &v1.MsgInit{ | ||
| Config: &v1.Config{ | ||
| Threshold: 666, | ||
| Quorum: 2000, | ||
| VotingPeriod: 60, | ||
| }, | ||
| Members: []*v1.Member{ | ||
| { | ||
| Address: "addr1", | ||
| Weight: 500, | ||
| }, | ||
| { | ||
| Address: "addr2", | ||
| Weight: 1000, | ||
| }, | ||
| }, | ||
| }, | ||
| "quorum must be less than or equal to the total weight", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testcases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| _, err := acc.Init(ctx, tc.msg) | ||
| if tc.expErr != "" { | ||
| require.EqualError(t, err, tc.expErr) | ||
| return | ||
| } | ||
| require.NoError(t, err) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The TestInit function is well-structured and covers a wide range of scenarios. Consider adding a test case for a null MsgInit to ensure robust error handling.
|
hmmmm do we need to keep the proposal and votes after proposal being executed |
Sounds good, I can remove votes given that we are producing events for them. And I guess we can say the same about the proposal? So we'll rely on an indexer to keep these |
|
|
||
| // set members | ||
| totalWeight := uint64(0) | ||
| for i := range msg.Members { |
There was a problem hiding this comment.
I suppose we'd need to check that a member does not appear twice
| * [MsgInit](#msginit) | ||
| * [MsgUpdateConfig](#msgupdateconfig) | ||
| * [MsgCreateProposal](#msgcreateproposal) | ||
| * [MsgVote](#msgvote) | ||
| * [MsgExecuteProposal](#msgexecuteproposal) |
There was a problem hiding this comment.
Indentation of list items is inconsistent with Markdown standards.
- * [MsgInit](#msginit)
- * [MsgUpdateConfig](#msgupdateconfig)
- * [MsgCreateProposal](#msgcreateproposal)
- * [MsgVote](#msgvote)
- * [MsgExecuteProposal](#msgexecuteproposal)
+ * [MsgInit](#msginit)
+ * [MsgUpdateConfig](#msgupdateconfig)
+ * [MsgCreateProposal](#msgcreateproposal)
+ * [MsgVote](#msgvote)
+ * [MsgExecuteProposal](#msgexecuteproposal)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.
| * [MsgInit](#msginit) | |
| * [MsgUpdateConfig](#msgupdateconfig) | |
| * [MsgCreateProposal](#msgcreateproposal) | |
| * [MsgVote](#msgvote) | |
| * [MsgExecuteProposal](#msgexecuteproposal) | |
| * [MsgInit](#msginit) | |
| * [MsgUpdateConfig](#msgupdateconfig) | |
| * [MsgCreateProposal](#msgcreateproposal) | |
| * [MsgVote](#msgvote) | |
| * [MsgExecuteProposal](#msgexecuteproposal) |
| Members collections.Map[[]byte, uint64] | ||
| Sequence collections.Sequence | ||
| Config collections.Item[v1.Config] | ||
|
|
||
| addrCodec address.Codec | ||
| headerService header.Service | ||
| eventService event.Service | ||
|
|
||
| Proposals collections.Map[uint64, v1.Proposal] | ||
| Votes collections.Map[collections.Pair[uint64, []byte], int32] // key: proposalID + voter address | ||
| } |
There was a problem hiding this comment.
Replace hard tabs with spaces to maintain consistency in indentation.
- Members collections.Map[[]byte, uint64]
- Sequence collections.Sequence
- Config collections.Item[v1.Config]
- addrCodec address.Codec
- headerService header.Service
- eventService event.Service
- Proposals collections.Map[uint64, v1.Proposal]
- Votes collections.Map[collections.Pair[uint64, []byte], int32] // key: proposalID + voter address
+ Members collections.Map[[]byte, uint64]
+ Sequence collections.Sequence
+ Config collections.Item[v1.Config]
+ addrCodec address.Codec
+ headerService header.Service
+ eventService event.Service
+ Proposals collections.Map[uint64, v1.Proposal]
+ Votes collections.Map[collections.Pair[uint64, []byte], int32] // key: proposalID + voter addressCommittable 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.
| Members collections.Map[[]byte, uint64] | |
| Sequence collections.Sequence | |
| Config collections.Item[v1.Config] | |
| addrCodec address.Codec | |
| headerService header.Service | |
| eventService event.Service | |
| Proposals collections.Map[uint64, v1.Proposal] | |
| Votes collections.Map[collections.Pair[uint64, []byte], int32] // key: proposalID + voter address | |
| } | |
| Members collections.Map[[]byte, uint64] | |
| Sequence collections.Sequence | |
| Config collections.Item[v1.Config] | |
| addrCodec address.Codec | |
| headerService header.Service | |
| eventService event.Service | |
| Proposals collections.Map[uint64, v1.Proposal] | |
| Votes collections.Map[collections.Pair[uint64, []byte], int32] // key: proposalID + voter address |
|
|
||
| ### Config | ||
|
|
||
| The config contains the basic rules defining how the multisig will work. All of these fields can be modified afterwards by calling [MsgUpdateConfig](#msgupdateconfig). |
There was a problem hiding this comment.
Consider using "afterward" instead of "afterwards" to align with American English conventions.
- All of these fields can be modified afterwards by calling [MsgUpdateConfig](#msgupdateconfig).
+ All of these fields can be modified afterward by calling [MsgUpdateConfig](#msgupdateconfig).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.
| The config contains the basic rules defining how the multisig will work. All of these fields can be modified afterwards by calling [MsgUpdateConfig](#msgupdateconfig). | |
| The config contains the basic rules defining how the multisig will work. All of these fields can be modified afterward by calling [MsgUpdateConfig](#msgupdateconfig). |
|
|
||
| ### Proposal | ||
|
|
||
| The proposal contains the title, summary, messages and the status of the proposal. The messages are stored as `google.protobuf.Any` to allow for any type of message to be stored. |
There was a problem hiding this comment.
Add a space after the period for better readability.
- The proposal contains the title, summary, messages and the status of the proposal. The messages are stored as `google.protobuf.Any` to allow for any type of message to be stored.
+ The proposal contains the title, summary, messages and the status of the proposal. The messages are stored as `google.protobuf.Any` to allow for any type of message to be stored.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.
| The proposal contains the title, summary, messages and the status of the proposal. The messages are stored as `google.protobuf.Any` to allow for any type of message to be stored. | |
| The proposal contains the title, summary, messages and the status of the proposal. The messages are stored as `google.protobuf.Any` to allow for any type of message to be stored. |
| message MsgExecuteProposal { | ||
| uint64 proposal_id = 1; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Ensure the file ends with a single newline character.
| return &v1.MsgVoteResponse{}, a.Votes.Set(ctx, collections.Join(msg.ProposalId, sender), int32(msg.Vote)) | ||
| } | ||
|
|
||
| func (a Account) CreateProposal(ctx context.Context, msg *v1.MsgCreateProposal) (*v1.MsgCreateProposalResponse, error) { |
There was a problem hiding this comment.
can we add go docs to all these functions
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
x/accounts/CHANGELOG.md (1)
30-30: Ensure the file ends with a single newline character.Please add a newline at the end of the file to adhere to POSIX standards and ensure compatibility with various tools and systems.
| ### Features | ||
|
|
||
| * [#19988](https://github.com/cosmos/cosmos-sdk/pull/19988) Implemented `x/accounts/multisig`. |
There was a problem hiding this comment.
Correct the markdown link syntax.
- * [#19988](https://github.com/cosmos/cosmos-sdk/pull/19988) Implemented `x/accounts/multisig`.
+ * [[#19988](https://github.com/cosmos/cosmos-sdk/pull/19988)] Implemented `x/accounts/multisig`.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.
| * [#19988](https://github.com/cosmos/cosmos-sdk/pull/19988) Implemented `x/accounts/multisig`. | |
| * [[#19988](https://github.com/cosmos/cosmos-sdk/pull/19988)] Implemented `x/accounts/multisig`. |
Description
This multisig design requires members to be existing accounts ... WIP
Closes: #18859
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.
I have...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
multisig.protofile.Tests