ADR-042: Group module #9089
ADR-042: Group module #9089
Conversation
blushi
requested review from
aleem1314,
amaury1093,
anilcse,
atheeshp,
clevinson,
tac0turtle,
robert-zaremba and
sunnya97
| While the group module is not meant to be a total replacement for the current multi-signature accounts, it provides a solution to the limitations described above, with a more flexible key management system where keys can be added, updated or removed, as well as configurable thresholds. | ||
| It's meant to be used with other key management modules such as [`x/feegrant`](./adr-029-fee-grant-module.md) ans [`x/authz`](adr-030-authz-module.md) to simplify key management for individuals and organizations. | ||
|
|
||
| The current implementation of the group module can be found in https://github.com/regen-network/regen-ledger/tree/master/proto/regen/group/v1alpha1 and https://github.com/regen-network/regen-ledger/tree/master/x/group. |
There was a problem hiding this comment.
This code implements adr033, correct?
Another note, the coding style could cause some confusion since we have a recommended module layout, the groups module implements something new that seems to lack documentation as well.
There was a problem hiding this comment.
yes indeed, we're currently having a broader conversion about ADR 033 module wiring here: regen-network/regen-ledger#301 so this needs to be figured out before implementing the group module as part of the sdk
|
Excited to see this land in the sdk. Thank you for writing the ADR!! |
| message Member { | ||
|
|
||
| // address is the member's account address. | ||
| string address = 1; |
There was a problem hiding this comment.
Will this be able to be a GroupAccountInfo.Address i.e. does the groups module support nested groups
There was a problem hiding this comment.
Yes it's theoretically possible, same for the group admin account.
There was a problem hiding this comment.
A group account is a module account itself so this works in this case but how could a regular module account vote?
There was a problem hiding this comment.
Might want to create an automated voting module or offload to cosmwasm.
| string weight = 2; | ||
|
|
||
| // metadata is any arbitrary metadata to attached to the member. | ||
| bytes metadata = 3; |
There was a problem hiding this comment.
What do you imagine a use case for this sort of meta data to be?
There was a problem hiding this comment.
Here's a use case in the context of regen-ledger: this could represent a hash of some data from the x/data module which is used for timestamping, signing, and storing data.
|
|
||
| #### Changing Group Membership | ||
|
|
||
| In the current implementation, updating a group or a group account after submitting a proposal will make it invalid. It will simply fail if someone calls `Msg/Exec` and will eventually be garbage collected. |
There was a problem hiding this comment.
I feel this adds a great deal of inflexibility. If for example a proposal lasts two weeks, then not only must arriving and leaving members wait until the proposal is finished but member weights can not be updates until after the proposal is complete. I'm guessing a default way of assigning weights is based on the amount of tokens a member contributes. By freezing this, I feel we take away a lot of sovereignty for group members and usability of groups. This will be even more problematic in large groups with potentially thousands of members.
Have you considered taking a snapshot of the member weights at the moment the proposal is submitted and using that as the reference for the decision policy? Perhaps this is more of a future improvement
There was a problem hiding this comment.
I agree that this is very inflexible. It works quite well for the multisig use case, or even very-infrequently-updated committees. But is completely unsuited for frequently changing groups (like groups based on stake).
However, it is much more efficient for this (quite useful) use case. And adding snapshotting to a company multisig might be overkill. It should be clear what the scope of the group module is.
There was a problem hiding this comment.
Right. That makes sense in the case where groups hardly change. I guess I was coming more from the angle of governance. Perhaps this could be handled by the DecisionPolicy. That way groups pick a decision policy that matches their use case. For relatively static groups, the decision policy voids a proposal when the group changes. For more dynamic groups, the decision policy takes a snapshot of the weights of all members at the submission of the proposal
There was a problem hiding this comment.
Group Account is a limited DAO contract. I don't think we should expect from the Group Account same complexity and same features as we would from a fully fledged DAO.
There was a problem hiding this comment.
This whole issue is addressed more thoroughly in #9066. Briefly, I believe that when weights are calculated should be a pluggable part of DecisionPolicy. For many use cases, the final weights are ideal IMHO and tallying can simply be done at the end of voting. In other use cases, we need a snapshot at the start of voting and this brings other technical complexities we will need to address (see #9066 (reply in thread))
There was a problem hiding this comment.
It's great to read the ADR. In the past I read the functional overview. Reading the spec gives another level of thinking. Left many comments and language changes. Some of my comments may relate to the audit. Overall I think we should separate the implementation details and put all notes about ORM in a new section rather then using it in the spec. Other implementations may not use the ORM.
Even though ORM simplifies some things, it may not feel right to use it by others.
Co-authored-by: Robert Zaremba <[email protected]>
There was a problem hiding this comment.
Nice stuff and almost the same design as cosmwasm group accounts.
I wonder if all the data types and orm comments need to be in here. Or rather to explain the needed interfaces and functions (not internal storage).
Comments were made on "first version of group module in regen". This ADR should be the general functionality, and allow the implementation to be iterated on without a new ADR
| multiple decision policies for different types of actions. Managing group | ||
| membership separately from decision policies results in the least overhead | ||
| and keeps membership consistent across different policies. The pattern that | ||
| is recommended is to have a single master group account for a given group, |
There was a problem hiding this comment.
This pattern is very nice to highlight
There was a problem hiding this comment.
I have left a few more minor questions.
Other than that I'm happy to give my approval (for what it's worth).
Coming from the angle of governance, I feel that we'll need to come to a decision about whether groups should accommodate for both large, complex and dynamic groups as well as the small, relatively static ones or whether this is too much of a stretch and it's better then that governance still remains it's own thing.
|
|
||
| ValidateBasic() error | ||
| GetTimeout() types.Duration | ||
| Allow(tally Tally, totalPower string, votingDuration time.Duration) (DecisionPolicyResult, error) |
There was a problem hiding this comment.
How many decision policies have you guys experimented with? I'm just wondering how flexible this interface actually is. One wouldn't be able to create a DecisionPolicy for quadratic voting right?
There was a problem hiding this comment.
afaik, we've only had the ThresholdDecisionPolicy.
I guess that for quadratic voting, it wouldn't be only about the decision policy but also how the members weights are managed.
There was a problem hiding this comment.
Is it possible for a DecisionPolicy to keep some state or is it ideal for it to be stateless. I guess another way of putting this is did you guys consider instead of giving the decision policy the entire tally that you just give it the vote. In this regard the decision policy would manage the tally
github-actions
bot
added
T: ADR
There was a problem hiding this comment.
LGTM, glad to see this landing in the sdk.
@fedekunze, give this a read. It will fit your use cases
tac0turtle
requested review from
fedekunze and
zmanian
and removed request for
clevinson,
anilcse,
jgimeno,
fdymylja and
atheeshp
|
Oh, this should have had more reviews before merging. ADRs require an approval from @alexanderbez or @aaronc. For formality’s sake, could @alexanderbez or @aaronc please leave an approval. |
|
ACK. Going forward, let's avoid using the |
<!-- The default pull request template is for types feat, fix, or refactor. For other templates, add one of the following parameters to the url: - template=docs.md - template=other.md --> ## Description ref: #7633 <!-- Add a description of the changes that this PR introduces and the files that are the most critical to review. --> Following up on #9089, this PR is the first step towards the migration of [x/group](https://github.com/regen-network/regen-ledger/tree/master/x/group) into the SDK. It introduces the group module proto definitions (types, tx, query) and other types. The rest of the code (module, server, client, genesis...) is dependent on various other discussions (#9238, #9182, #9237, #7773) and will be added in follow-up PRs incrementally. --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed ### 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... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)
<!-- The default pull request template is for types feat, fix, or refactor. For other templates, add one of the following parameters to the url: - template=docs.md - template=other.md --> ## Description ref: cosmos/cosmos-sdk#7633 <!-- Add a description of the changes that this PR introduces and the files that are the most critical to review. --> Following up on cosmos/cosmos-sdk#9089, this PR is the first step towards the migration of [x/group](https://github.com/regen-network/regen-ledger/tree/master/x/group) into the SDK. It introduces the group module proto definitions (types, tx, query) and other types. The rest of the code (module, server, client, genesis...) is dependent on various other discussions (cosmos/cosmos-sdk#9238, cosmos/cosmos-sdk#9182, cosmos/cosmos-sdk#9237, cosmos/cosmos-sdk#7773) and will be added in follow-up PRs incrementally. --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed ### 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... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)
Description
ref: #7633
This ADR introduces the Group module.
rendered
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes