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

WIP on group module spec #13

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Empty file added x/group/spec/README.md
Empty file.
Empty file added x/group/spec/concepts.md
Empty file.
Empty file added x/group/spec/events.md
Empty file.
Empty file added x/group/spec/keepers.md
Empty file.
46 changes: 46 additions & 0 deletions x/group/spec/messages.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
```go
// Groups get their own GroupID
type MsgCreateGroup struct {
Signer sdk.AccAddress `json:"signer"`

Choose a reason for hiding this comment

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

Why is there a Signer and an Owner?
Either they should be the same, or we don't have to require any Signer (if anyone can create a group)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A Signer is always required in a cosmos SDK message. One reason for having both is that Owner may actually be a contract - for instance some contract that elects the members of the group.

Choose a reason for hiding this comment

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

I see the purpose of Owner in state.
Signers() can return msg.Owner

If we don't care who signs, no need to return anything.
Again, this is a bit more discussion of best practices of cosmos-sdk... not sure.

// The Owner of the group is allowed to change the group structure. A group account
// can own a group in order for the group to be able to manage its own members
Owner sdk.AccAddress `json:"owner"`

Choose a reason for hiding this comment

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

Chicken and egg issue...
I need to create a group (alex: read "electorate") in order to create a group account (alex: read "election rule").
So, how can I set it as owner on creation of the group.

I would propose a 3 step setup:

  • X creates a group and is automatically the owner (owner == signer)
  • X creates a group account with a decision policy (eg. 2 of 3) on this group
  • X abdicates the thrown and makes the given group account the owner of the group

Those can be 3 messages in one tx and we can do that behind the scenes in the user interface, but in order to avoid some weird causality tangle, I suggest to make them explicitly separate

Choose a reason for hiding this comment

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

This brings up the point that we need a MsgUpdateGroup as well

// The members of the group and their associated weight
Members []Member `json:"members,omitempty"`
// TODO maybe make this something more specific to a domain name or a claim on identity? or Info leave it generic
Memo string `json:"memo,omitempty"`

Choose a reason for hiding this comment

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

If we store this on the group itself, I would call it Description or Title.
I think that makes sense, so they can identify themselves.
I do think storing this makes sense.

If it is just a memo for the transaction itself, we don't need it imo. There is a top-level one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Description then?

Copy link

Choose a reason for hiding this comment

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

Agree, memo is used already in a different context. For UX purpose in clients it may be very useful to store human readable information with a group. Title and description both make sense.

}

// group accounts get their own sdk.AccAddress
type MsgCreateGroupAccount struct {
Signer sdk.AccAddress `json:"signer"`
// The Owner of a group account is allowed to change the DecisionPolicy. This can be left nil
// in order for the group account to "own" itself
Owner sdk.AccAddress `json:"owner"`

Choose a reason for hiding this comment

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

Why is nil = group owns itself? Implicit nils are dangerous to me.
I would allow the options of no owner (can never change), self-owner, or external owner (maybe a 1 of N rule to move funds, controlled by a 3 of N rules to update the rules - or change the group).

At least in the state, nil should be no owner, rather than self-owner. Problem in the Msg is you don't know the self-address ahead of time. I would make that explicit somehow here (difference between self-owner and un-owned)

Choose a reason for hiding this comment

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

And if there is an owner... we need a MsgUpdateGroupAccount to change the decision policy

Group GroupID `json:"group"`
DecisionPolicy DecisionPolicy `json:"decision_policy"`

Choose a reason for hiding this comment

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

Memo string `json:"memo,omitempty"`

Choose a reason for hiding this comment

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

Again, Memo is something in the Tx that never gets written to state. If we store it, please call it Name, Title, etc.

}
type MsgCreateProposal struct {
Proposer sdk.AccAddress `json:"proposer"`
GroupAccount sdk.AccAddress `json:"group"`
Msgs []sdk.Msg `json:"msgs"`
// Exec can be set to true in order to attempt to execute the proposal immediately

Choose a reason for hiding this comment

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

Alex: this is the way to unify single-tx multisig, with on-chain governance rules, rather than have two different modules.

Since you worked on those both separately in weave, please reflect if there are any issues that arise by this unifying abstraction

Copy link

Choose a reason for hiding this comment

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

I do understand the motivation but there will be quite some complexity for any UI client that have to show the state or votes. I don't know much about the veto rules, yet but they may add complexity, too.

// with no voting in a single transaction - this is useful for 1/N or 2/N multisig
// key groups. Every signer of the MsgCreateProposal transaction is considered a yes

Choose a reason for hiding this comment

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

There is a problem here, we don't have access to the signers list in the Handler.
(I have an open issue on this one: cosmos#4937 )

Currently, the work-around is to define all signers you want to verify in the msg itself, and return them in GetSigners(). So... we would need to add another array of "co-signers" here to allow 2 of N as positive votes.

Choose a reason for hiding this comment

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

Also, in general, can we assume that the proposal also makes a "yes" vote upon "create proposal", even if exec is false?

// vote
Exec bool `json:"exec,omitempty"`
}

type MsgVote struct {
ProposalID ProposalID `json:"proposal_id"`
// Voters must sign this transaction
Voters []sdk.AccAddress `json:"voters"`
Vote Vote `json:"vote"`
}

type MsgTryExecuteProposal struct {
ProposalID ProposalID `json:"proposal_id"`
Signer sdk.AccAddress `json:"signer"`

Choose a reason for hiding this comment

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

Does the signer need to be a member of the group?
If anyone can sign this, we can just remove this field. Only if we need this info to enforce some permissions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anyone could sign, but again in cosmos-sdk the signer needs to be explicit on the sdk.Msg so that Signers() can be implemented

Choose a reason for hiding this comment

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

If anyone can perform this action, then Signers() can return nil. That is valid afaik

}
```
8 changes: 8 additions & 0 deletions x/group/spec/params.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Parameters

The group module contains the following parameters:

| Key | Type | Usage | Example |
|------------------------|-----------------|---------|
| MaxVotingPeriod | string (time ns) | Proposals older than this time will be garbage-collected | "172800000000000" |

Choose a reason for hiding this comment

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

okay, in general decision policies can't set a voting timeout, smaller than this garbage collection limit.

also, no need for ns precision with ~5s blocktime. Removing 9 zeros, makes this much more legible (it is 2 days, right)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Express it in minutes then? I think that precision is for easy conversion to time.Time

| MaxDescriptionCharacters | string (uint64) | | "256" |
15 changes: 15 additions & 0 deletions x/group/spec/policies.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Decision Policies

```go
type ThresholdDecisionPolicy struct {
// Specifies the number of votes that must be accumulated in order for a decision to be made by the group.
// A member gets as many votes as is indicated by their Weight field.
// A big integer is used here to avoid any potential vulnerabilities from overflow errors
// where large weight and threshold values are used.
DecisionThreshold sdk.Int `json:"decision_threshold"`
}

type PercentageDecisionPolicy struct {
Percent sdk.Dec `json:"percent"`
aaronc marked this conversation as resolved.
Show resolved Hide resolved
}
```
117 changes: 117 additions & 0 deletions x/group/spec/state.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
# State & Types

## Groups

Groups are simply aggregations of members.

### Key Value Store Layout

| Key | Description | Type |
|---------------------|--------------------|--------------------------|
| `g/<group>/desc` | Group description | `string` |
| `g/<group>/<member>` | Member's voting power | `sdk.Int` |
| `g/<group>/<member>/desc` | Member description | `string` |
| `g/<group>/owner` | Group owner | `sdk.AccAdress` |
| `g/<group>/totalPower` | Group's computed total power | `sdk.Int` |
| `mg/<member>/<group>` | Member -> group reverse index | empty |
| `og/<owner>/<group>` | Owner -> group reverse index | empty |

### Key Types

| Key | Type |
|---------------------|------------------|
| `<group>` | `GroupID` |
| `<owner>` | `sdk.AccAddress` |
| `<member>` | `sdk.AccAddress` |

### Custom Types

```go
type GroupID []byte
```

`GroupID` is generated from an auto-incrementing `uint64` preprended with the
prefix `0`. This prefix allows other group composition mechanisms in the future,
specifically via token ownership rather than group membership.

## Group Accounts

Group accounts associate a group with a decision policy.

### Key-Value Store Layout

| Key | Description | Type |
|---------------------|--------------------|--------------------------|
| `a/<group-account>/description` | Group account description | `string` |
| `a/<group-account>/group` | Group account's underlying group | `GroupID` |
| `a/<group-account>/decisionPolicy` | Group account's decision policy | `DecisionPolicy` |
| `a/<group-account>/owner` | Group account's owner | `sdk.AccAddress` |
| `ga/<group>/<group-account>` | Group -> group account reverse index | empty |
| `oa/<owner>/<group-account>` | Owner -> group account reverse index | empty |

### Key Types

| Key | Type | Description |
|---------------------|------------------| ------------|
| `<group-account>` | `GroupID` | Generated from an auto-incremented `uint64` |
| `<group>` | `GroupID` | |
| `<owner>` | `sdk.AccAddress` | |

*TODO:* How to encode group account addresses?

### Custom Types

```go
type Tally struct {
YesCount sdk.Int
NoCount sdk.Int
AbstainCount sdk.Int
VetoCount sdk.Int
}
__
// DecisionPolicy allows for flexibility in decision policy based both on
// powers (the tally of yes, no, abstain, and veto votes) and time since voting
// started
type DecisionPolicy interface {
Allow(tally Tally, totalPower sdk.Int, timeSinceVotingStart time.Duration)
}
```

## Proposals

### Key Value Store Layout

| Key | Description | Type |
|---------------------|--------------------|--------------------------|
| `p/<proposal>/desc` | Proposal description | `string` |
| `p/<proposal>/ga` | Proposal's group account | `sdk.AccAddress` |
| `p/<proposal>/msgs` | Messages that will be run if the proposal succeeds | `[]sdk.Msg` |
| `p/<proposal>/proposer` | Account that proposed the proposal | `sdk.AccAddress` |
| `p/<proposal>/votingStart` | When voting started | `time.Time` |
| `p/<proposal>/<voter>/vote` | A voter's vote on the proposal | `Vote` |
| `p/<proposal>/<voter>/comment` | A voter's comment on their vote | `string` |
| `vp/<voter>/<proposal>` | Voter -> proposal reverse look-up | empty |
| `ap/<group-account>/<proposal>` | Group account -> proposal reverse look-up | empty |
| `pp/<proposer>/<proposal>` | Proposer -> proposal reverse look-up | empty |

### Key Type

| Key | Type |------|
|---------------------|------------------|------|
| `<proposal>` | `ProposalID` | auto-incremented `uint64`
| `<voter>` | `sdk.AccAddress` |
| `<proposer>` | `sdk.AccAddress` |
| `<group-account>` | `sdk.AccAddress` |

### Custom Types

```go
type Vote int

const (
No Vote = 0
Yes = 1
Abstain = 2
Veto = 3
)
```
36 changes: 36 additions & 0 deletions x/group/spec/ucr.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Use Cases and Requirements

## Use Cases

### UC1: Multi-factor authentication (MFA) and key rotation

A single user may want to have an account that requires at least two out of
three key to sign for instance.


That use may also want to have an account that isn't tied to a single permanent
key but rather where they can add and remove keys as their needs evolve. The
member keys could be tied to a single device or hardware wallet, or potentially
tied to a custodial service that users other mechanisms to support MFA

### UC2: Organizational Accounts

### UC3: DAO's

## Requirements

### R1: Groups aren't tied to any permanent key

### R2: Groups are themselves an account

### R3: Keys can be added and removed from a group

### R4: Groups can submit and execute a transaction within a single transaction

### R5: Different keys can be assigned different weights

### R6: Different voting strategies and thresholds can be chosen

### R7: The same group can use different voting stategies for different accounts

### R8: Garbage proposals eventually get cleaned up