Skip to content
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

ADR-042: Group module #9089

Merged
merged 33 commits into from
May 10, 2021
Merged
Changes from 31 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
02633ab
Add ADR-042
blushi Apr 9, 2021
8582590
Merge branch 'master' into marie/adr-042-group
blushi Apr 9, 2021
2ccc701
Fix link
blushi Apr 9, 2021
f9a16b3
Merge branch 'marie/adr-042-group' of github.com:cosmos/cosmos-sdk in…
blushi Apr 9, 2021
217ca16
Small improvements
blushi Apr 9, 2021
64f87bc
Update link
blushi Apr 9, 2021
992a86b
Merge branch 'master' into marie/adr-042-group
Apr 11, 2021
432b93f
Update docs/architecture/adr-042-group-module.md
blushi Apr 14, 2021
abe6245
Update docs/architecture/adr-042-group-module.md
blushi Apr 15, 2021
08dcd47
Update docs/architecture/adr-042-group-module.md
blushi Apr 15, 2021
16578be
Update docs/architecture/adr-042-group-module.md
blushi Apr 15, 2021
180eb50
Update docs/architecture/adr-042-group-module.md
blushi Apr 15, 2021
ce4510b
Update docs/architecture/adr-042-group-module.md
blushi Apr 15, 2021
8031db1
Update docs/architecture/adr-042-group-module.md
blushi Apr 15, 2021
90dd35d
Update docs/architecture/adr-042-group-module.md
blushi Apr 16, 2021
9969a6f
Update docs/architecture/adr-042-group-module.md
blushi Apr 16, 2021
bc20dcc
Update docs/architecture/adr-042-group-module.md
blushi Apr 16, 2021
d2be7ff
Update docs/architecture/adr-042-group-module.md
blushi Apr 16, 2021
f13b343
Update docs/architecture/adr-042-group-module.md
blushi Apr 16, 2021
146ea46
Update docs/architecture/adr-042-group-module.md
blushi Apr 16, 2021
ee0567f
Update docs/architecture/adr-042-group-module.md
blushi Apr 16, 2021
9897020
Update docs/architecture/adr-042-group-module.md
blushi Apr 16, 2021
d6a0654
Update docs/architecture/adr-042-group-module.md
blushi Apr 16, 2021
b74b534
Move orm to specific section
blushi Apr 16, 2021
783d51d
Update docs/architecture/adr-042-group-module.md
blushi Apr 16, 2021
9afc938
Update naming
blushi Apr 16, 2021
ce103bf
Add concrete use cases
blushi Apr 22, 2021
1afa664
Rework ### Proposal
blushi Apr 22, 2021
5d609c6
Rework Vote, Exec and implementation sections
blushi Apr 22, 2021
adf1525
Merge branch 'master' into marie/adr-042-group
blushi May 5, 2021
e9c39b1
Update to account for removal of ServiceMsg
blushi May 5, 2021
c3bf028
Merge branch 'master' into marie/adr-042-group
blushi May 10, 2021
08d9be5
Merge branch 'master' into marie/adr-042-group
mergify[bot] May 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
277 changes: 277 additions & 0 deletions docs/architecture/adr-042-group-module.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,277 @@
# ADR 042: Group Module

## Changelog

- 2020/04/09: Initial Draft

## Status

Draft

## Abstract

This ADR defines the `x/group` module which allows the creation and management of on-chain multi-signature accounts and enables voting for message execution based on configurable decision policies.

## Context

The legacy amino multi-signature mechanism of the Cosmos SDK has certain limitations:
- Key rotation is not possible, although this can be solved with [account rekeying](adr-034-account-rekeying.md).
- Thresholds can't be changed.
- UX is cumbersome for non-technical users ([#5661](https://github.com/cosmos/cosmos-sdk/issues/5661)).
- It requires `legacy_amino` sign mode ([#8141](https://github.com/cosmos/cosmos-sdk/issues/8141)).

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 access control 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 proof of concept 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.

## Decision

We propose merging the `x/group` module with its supporting [ORM/Table Store package](https://github.com/regen-network/regen-ledger/tree/master/orm) ([#7098](https://github.com/cosmos/cosmos-sdk/issues/7098)) into the Cosmos SDK and continuing development here. There will be a dedicated ADR for the ORM package.
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved

### Group

A group is a composition of accounts with associated weights. It is not
an account and doesn't have a balance. It doesn't in and of itself have any
sort of voting or decision weight.
Group members can create proposals and vote on them through group accounts using different decision policies.

It has an `admin` account which can manage members in the group, update the group
metadata and set a new admin.

```proto
message GroupInfo {

// group_id is the unique ID of this group.
uint64 group_id = 1;

// admin is the account address of the group's admin.
string admin = 2;

// metadata is any arbitrary metadata to attached to the group.
bytes metadata = 3;

// version is used to track changes to a group's membership structure that
// would break existing proposals. Whenever a member weight has changed,
// or any member is added or removed, the version is incremented and will
// invalidate all proposals from older versions.
uint64 version = 4;

// total_weight is the sum of the group members' weights.
string total_weight = 5;
}
```

```proto
message GroupMember {

// group_id is the unique ID of the group.
uint64 group_id = 1;

// member is the member data.
Member member = 2;
}

// Member represents a group member with an account address,
// non-zero weight and metadata.
message Member {

// address is the member's account address.
string address = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be able to be a GroupAccountInfo.Address i.e. does the groups module support nested groups

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's theoretically possible, same for the group admin account.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or module addresses?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A group account is a module account itself so this works in this case but how could a regular module account vote?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to create an automated voting module or offload to cosmwasm.


// weight is the member's voting weight that should be greater than 0.
string weight = 2;

// metadata is any arbitrary metadata to attached to the member.
bytes metadata = 3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you imagine a use case for this sort of meta data to be?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, re audit notes - we may remove Group prefix from all type names. It's already provided by a package.
cc: @aaronc


### Group Account
cmwaters marked this conversation as resolved.
Show resolved Hide resolved

A group account is an account associated with a group and a decision policy.
A group account does have a balance.

Group accounts are abstracted from groups because a single group may have
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern is very nice to highlight

and then to create separate group accounts with different decision policies
and delegate the desired permissions from the master account to
those "sub-accounts" using the [`x/authz` module](adr-030-authz-module.md).

```proto
message GroupAccountInfo {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have Info suffix? Based on the paragraph above, I would call this PolicyAccount.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was in order to be consistent with x/ecocredit from regen-ledger which uses this suffix too, but I'd be ok with dropping it.
We use Group prefix because it's related to a given group and I believe it feels more natural as part of the x/group module.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, how about PolicyGroup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then you lose info about the fact that it's an actual account. I kinda like the current GroupAccount naming but don't have a strong preference either. It'd be good to have some thoughts from @aaronc who's the primarily designer of the group module afaik.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, how about PolicyAccount? I don't think we need to put Group everywhere.


// address is the group account address.
string address = 1;

// group_id is the ID of the Group the GroupAccount belongs to.
uint64 group_id = 2;

// admin is the account address of the group admin.
string admin = 3;
blushi marked this conversation as resolved.
Show resolved Hide resolved

// metadata is any arbitrary metadata of this group account.
bytes metadata = 4;

// version is used to track changes to a group's GroupAccountInfo structure that
// invalidates active proposal from old versions.
uint64 version = 5;

// decision_policy specifies the group account's decision policy.
google.protobuf.Any decision_policy = 6 [(cosmos_proto.accepts_interface) = "DecisionPolicy"];
}
```

Similarly to a group admin, a group account admin can update its metadata, decision policy or set a new group account admin.

A group account can also be an admin or a member of a group.
For instance, a group admin could be another group account which could "elects" the members or it could be the same group that elects itself.

### Decision Policy

A decision policy is the mechanism by which members of a group can vote on
proposals.
Comment on lines +135 to +136
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A decision policy is the mechanism by which members of a group can vote on
proposals.
A decision policy is an interface for voting policies for group members.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this suggestion is a bit redundant "A decision policy is an interface for voting policies..."


All decision policies should have a minimum and maximum voting window.
The minimum voting window is the minimum duration that must pass in order
for a proposal to potentially pass, and it may be set to 0. The maximum voting
window is the maximum time that a proposal may be voted on and executed if
it reached enough support before it is closed.
Both of these values must be less than a chain-wide max voting window parameter.

We define the `DecisionPolicy` interface that all decision policies must implement:

```go
type DecisionPolicy interface {
codec.ProtoMarshaler

ValidateBasic() error
GetTimeout() types.Duration
Allow(tally Tally, totalPower string, votingDuration time.Duration) (DecisionPolicyResult, error)
blushi marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Validate(g GroupInfo) error
}

type DecisionPolicyResult struct {
Allow bool
Final bool
cmwaters marked this conversation as resolved.
Show resolved Hide resolved
}
```

#### Threshold decision policy

A threshold decision policy defines a minimum support votes (_yes_), based on a tally
of voter weights, for a proposal to pass. For
this decision policy, abstain and veto are treated as no support (_no_).

```proto
message ThresholdDecisionPolicy {

// threshold is the minimum weighted sum of support votes for a proposal to succeed.
string threshold = 1;

// voting_period is the duration from submission of a proposal to the end of voting period
// Within this period, votes and exec messages can be submitted.
google.protobuf.Duration voting_period = 2 [(gogoproto.nullable) = false];
}
```

### Proposal

Any member of a group can submit a proposal for a group account to decide upon.
blushi marked this conversation as resolved.
Show resolved Hide resolved
A proposal consists of a set of `sdk.Msg`s that will be executed if the proposal
passes as well as any metadata associated with the proposal. These `sdk.Msg`s get validated as part of the `Msg/CreateProposal` request validation. They should also have their signer set as the group account.

Internally, a proposal also tracks:
- its current `Status`: submitted, closed or aborted
- its `Result`: unfinalized, accepted or rejected
- its `VoteState` in the form of a `Tally`, which is calculated on new votes and when executing the proposal.

```proto
// Tally represents the sum of weighted votes.
message Tally {
option (gogoproto.goproto_getters) = false;

// yes_count is the weighted sum of yes votes.
string yes_count = 1;

// no_count is the weighted sum of no votes.
string no_count = 2;

// abstain_count is the weighted sum of abstainers.
string abstain_count = 3;

// veto_count is the weighted sum of vetoes.
string veto_count = 4;
blushi marked this conversation as resolved.
Show resolved Hide resolved
}
```
blushi marked this conversation as resolved.
Show resolved Hide resolved

### Voting

Members of a group can vote on proposals. There are four choices to choose while voting - yes, no, abstain and veto. Not
all decision policies will support them. Votes can contain some optional metadata.
In the current implementation, the voting window begins as soon as a proposal
is submitted.

Voting internally updates the proposal `VoteState` as well as `Status` and `Result` if needed.

### Executing Proposals

Proposals will not be automatically executed by the chain in this current design,
but rather a user must submit a `Msg/Exec` transaction to attempt to execute the
proposal based on the current votes and decision policy. A future upgrade could
automate this and have the group account (or a fee granter) pay.

#### 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))


### Notes on current implementation

This section outlines the current implementation used in the proof of concept of the group module but this could be subject to changes and iterated on.

#### ORM

The [ORM package](https://github.com/cosmos/cosmos-sdk/discussions/9156) defines tables, sequences and secondary indexes which are used in the group module.

Groups are stored in state as part of a `groupTable`, the `group_id` being an auto-increment integer. Group members are stored in a `groupMemberTable`.

Group accounts are stored in a `groupAccountTable`. The group account address is generated based on an auto-increment integer which is used to derive the group module `RootModuleKey` into a `DerivedModuleKey`, as stated in [ADR-033](adr-033-protobuf-inter-module-comm.md#modulekeys-and-moduleids). The group account is added as a new `ModuleAccount` through `x/auth`.

Proposals are stored as part of the `proposalTable` using the `Proposal` type. The `proposal_id` is an auto-increment integer.

Votes are stored in the `voteTable`. The primary key is based on the vote's `proposal_id` and `voter` account address.

#### ADR-033 to route proposal messages

Inter-module communication introduced by [ADR-033](adr-033-protobuf-inter-module-comm.md) can be used to route a proposal's messages using the `DerivedModuleKey` corresponding to the proposal's group account.

## Consequences

### Positive

- Improved UX for multi-signature accounts allowing key rotation and custom decision policies.

### Negative

### Neutral

- It uses ADR 033 so it will need to be implemented within the Cosmos SDK, but this doesn't imply necessarily any large refactoring of existing Cosmos SDK modules.
- The current implementation of the group module uses the ORM package.

## Further Discussions

- Convergence of `/group` and `x/gov` as both support proposals and voting: https://github.com/cosmos/cosmos-sdk/discussions/9066
- `x/group` possible future improvements:
- Execute proposals on submission (https://github.com/regen-network/regen-ledger/issues/288)
- Withdraw a proposal (https://github.com/regen-network/cosmos-modules/issues/41)
- Make `Tally` more flexible and support non-binary choices

## References

- Initial specification:
- https://gist.github.com/aaronc/b60628017352df5983791cad30babe56#group-module
- [#5236](https://github.com/cosmos/cosmos-sdk/pull/5236)
- Proposal to add `x/group` into the SDK: [#7633](https://github.com/cosmos/cosmos-sdk/issues/7633)