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.

Sign up for GitHub

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

Jump to bottom

Group module spec #5236

Closed
wants to merge 7 commits into from
Closed

Group module spec #5236

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9df4207
WIP on new group module spec
aaronc Oct 21, 2019
baa8337
Added proposal spec
aaronc Oct 23, 2019
02748b2
Markdown fix
aaronc Oct 23, 2019
f7461b2
Updates to spec
aaronc Oct 24, 2019
0e1d676
Revert from group policy -> group account design after design discussion
aaronc Oct 29, 2019
72de7af
Merge branch 'master' into regen-network/group-spec
alexanderbez Oct 30, 2019
8fb79a8
Update group spec based on PR review
aaronc Nov 6, 2019
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
105 changes: 20 additions & 85 deletions x/group/spec/group.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

## Types

### `GroupID`

```go
// GroupID is the auto-generated ID of the group
type GroupID uint64
```


### `Member`

```go
Expand All @@ -11,32 +19,7 @@ type Member struct {
Address sdk.AccAddress `json:"address"`
// The integral power of this member with respect to other members
Power sdk.Int `json:"power"`
}
```

### `DecisionProtocol`

```go
// DecisionProtocol allows for flexibility in decision policy based both on
// weights (the tally of yes, no, abstain, and veto votes) and votingDuration -
// the amount of time that has been allowed for voting. A zero votingDuration
// means this proposal is being executed as basically a single multi-sig
// transaction with no voting window. This may be okay for some DecisionProtocol's.
// Other protocols may stipulate that at least a few hours or days of voting
// must occur to pass a proposal.
type DecisionProtocol interface {
Allow(tally Tally, totalPower sdk.Int, votingDuration time.Duration)
}
```

### `Tally`

```go
type Tally struct {
YesCount sdk.Int
NoCount sdk.Int
AbstainCount sdk.Int
VetoCount sdk.Int
Description sdk.Int `json:"description"`
aaronc marked this conversation as resolved.
Show resolved Hide resolved
}
```

Expand All @@ -46,75 +29,28 @@ type Tally struct {

```go
type MsgCreateGroup struct {
// The Owner of the group is allowed to change the group structure. A group account
// The admin 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"`
Admin sdk.AccAddress `json:"admin"`
// The members of the group and their associated power
Members []Member `json:"members,omitempty"`
Description string `json:"Description,omitempty"`
DecisionProtocol DecisionProtocol `json:"decision_protocol"`
Description string `json:"description,omitempty"`
}
```

*Returns:* `sdk.AccAddress` based on an auto-incrementing `uint64`.

### `MsgGroupExec`

```go
// MsgGroupExec executed the provided messages using the groups account if the
// provided signers pass the group's DecisionProtocol. This is essentially a
// basic multi-signature execution method.
type MsgGroupExec struct {
Group sdk.AccAddress `json:"group"`
Signers []sdk.AccAddress `json:"signers"`
Msgs []sdk.Msg `json:"msgs"`
// Policy specifies the policy by which this propose will pass or fail.
// If set to DefaultGroupPolicy, the group's root DecisionProtocol is used. If
// it is set to another GroupPolicyID for this group, that policy's
// DecisionProtocol and its Capability grants will be used.
Policy GroupPolicyID `json:"policy"`
}
```
*Returns:* `GroupID` based on an auto-incrementing `uint64`.

### `MsgUpdateGroupMembers`
### `MsgUpdateGroup

```go
// MsgUpdateGroupMembers updates the members of the group, adding, removing,
// and updating members as needed. To remove an existing member set its Power to 0.
type MsgUpdateGroupMembers struct {
Owner sdk.AccAddress `json:"owner"`
Group sdk.AccAddress `json:"group"`
Members []Member `json:"members,omitempty"`
}
```

### `MsgUpdateGroupOwner`

```go
type MsgUpdateGroupOwner struct {
Owner sdk.AccAddress `json:"owner"`
Group sdk.AccAddress `json:"group"`
NewOwner sdk.AccAddress `json:"new_owner"`
}
```

### `MsgUpdateGroupDecisionProtocol`

```go
type MsgUpdateGroupDecisionProtocol struct {
Owner sdk.AccAddress `json:"owner"`
Group sdk.AccAddress `json:"group"`
DecisionProtocol DecisionProtocol `json:"decision_protocol"`
}
```

### `MsgUpdateGroupDescription`

```go
type MsgUpdateGroupDescription struct {
Owner sdk.AccAddress `json:"owner"`
Group sdk.AccAddress `json:"group"`
DecisionProtocol DecisionProtocol `json:"decision_protocol"`
Admin sdk.AccAddress `json:"admin"`
Group GroupID `json:"group"`
NewAdmin sdk.AccAddress `json:"new_admin"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please define behavior if NewAdmin is unset (I assume don't change Admin).

Copy link
Member Author

Choose a reason for hiding this comment

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

We can either make it a no-op like you're proposing or this would seal the group and make it impossible to change? Is that desirable? I think that would probably be unexpected behavior so for now I will define it as a no-op. If there is a use case for that, this could be a separate "seal group" message.

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

Description string `json:"description,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.. I assume this is optional to change it? Maybe use *string to be clear there - otherwise there is no way to unset it.

It seems like this operation is supposed to be a patch, and to do so properly, we need to allow all values (including zero-value if legal) as well as "unset". In my experience with go-json, this is typically done by using pointer fields and checking for nil (which can be missing from json or explicitly set as null, either way, ignored)

Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative to a single update message is to have a separate update message for each field, so MsgUpdateGroupDescription, MsgUpdateGroupAdmin, etc. Would that be preferable? For now I am changing the spec to your approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think separate messages is bad.

Either define POST behavior - overwrite current content with msg.
Or PUT - only include some fields, only update those that are include.

I assume PUT is desired, but this should be made explicit and consistent.

MemberUpdates []Member `json:"member_updates,omitempty"`
}
```

Expand All @@ -131,9 +67,8 @@ to authorize messages send back to the router.
```go
type GroupKeeper interface {
IterateGroupsByMember(member sdk.Address, fn func (group sdk.AccAddress) (stop bool))
IterateGroupsByOwner(member sdk.Address, fn func (group sdk.AccAddress) (stop bool))
IterateGroupsByAdmin(member sdk.Address, fn func (group sdk.AccAddress) (stop bool))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why return the group sdk.AccAddress? Which means you have to then query the group itself? Seems easier to pass group Group into the callback.

Also, I note you never defined the Group struct that is stored in the kvstore above.

Copy link
Member Author

Choose a reason for hiding this comment

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

This part was intentional. I didn't define a group struct because I don't want to return all members at once. See how it is in the changed spec and we can discuss if maybe there should be a GroupMetadata struct like this:

type GroupMetadata struct {
  Group GroupID
  TotalPower sdk.Int
  Description string
  Admin sdk.AccAddress
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh... Okay, then can you mark this in the spec as "not yet defined", but make sure some comment is there, not just an oversight.

GetGroupDescription(group sdk.AccAddress) string
aaronc marked this conversation as resolved.
Show resolved Hide resolved
GetTotalPower(group sdk.AccAddress) sdk.Int
GetDecisionProtocol(group sdk.AccAddress) DecisionProtocol
}
```
74 changes: 74 additions & 0 deletions x/group/spec/group_account.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# Group Policy Specification

## Types

### `DecisionPolicy`

```go
// DecisionPolicy allows for flexibility in decision policy based both on
// weights (the tally of yes, no, abstain, and veto votes) and votingDuration -
// the amount of time that has been allowed for voting. A zero votingDuration
// means this proposal is being executed as basically a single multi-sig
// transaction with no voting window. This may be okay for some DecisionPolicy's.
// Other policies may stipulate that at least a few hours or days of voting
// must occur to pass a proposal.
type DecisionPolicy interface {
Allow(tally Tally, totalPower sdk.Int, votingDuration time.Duration)
}
```

### `Tally`

```go
type Tally struct {
YesCount sdk.Int
NoCount sdk.Int
AbstainCount sdk.Int
VetoCount sdk.Int
}
```


## Messages

### `MsgCreateGroupAccount
Copy link
Contributor

Choose a reason for hiding this comment

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

This name is a bit unwieldy. I don't have a great alternative, but should be refined. Group makes clear sense and is what it says. GroupAccount is not clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, GroupAccount is a bit unwieldy but I also was unable to come up with a better name. I am open to proposals.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will give one when I have one.... wish I was good at naming

```go
// MsgCreateGroupPolicy creates a new group policy with the provided
// DecisionPolicy. A group policy allows groups to set different decision
// policys for different types of action.
type MsgCreateGroupAccount struct {
Admin sdk.AccAddress `json:"admin"`
Group GroupID `json:"group"`
DecisionPolicy DecisionPolicy `json:"decision_policy"`
Description string `json:"Description,omitempty"`
}
```

*Returns:* a new auto-generated `sdk.AccAddress` for this group.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/group./group account./


### `MsgGroupExec`

```go
// MsgGroupExec executed the provided messages using the groups account if the
// provided signers pass the group's DecisionPolicy. This is essentially a
// basic multi-signature execution method.
type MsgGroupExec struct {
GroupAccount sdk.AccAddress `json:"group_account"`
Signers []sdk.AccAddress `json:"signers"`
Msgs []sdk.Msg `json:"msgs"`
}
```

### Update messages

```go
type MsgUpdateGroupAccount struct {
Admin sdk.AccAddress `json:"admin"`
GroupAccount sdk.AccAddress `json:"group_account"`
NewAdmin sdk.AccAddress `json:"new_admin"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above about omitted fields and zero-values - do we set to zero value, do we leave them unchanged? Both have issues, maybe consider pointers, even if a bit funny looking here, they render to the same json.

Copy link
Contributor

@Hyung-bharvest Hyung-bharvest Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I want to suggest two functionality to MsgUpdateGroupAccount.

  1. There can be a GroupAccount without Admin
  2. Members of the GroupAccount can vote for MsgUpdateGroupAccount

This way, we can create a GroupAccount which ownership is fully decentralized but modifiable.

Another question.
Why MsgUpdateGroupAccount does not have a field Members to be updated?

Copy link
Contributor

Choose a reason for hiding this comment

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

questions resolved. thank you @aaronc

DecisionPolicy DecisionPolicy `json:"decision_policy"`
Description string `json:"Description,omitempty"`
}

```

93 changes: 0 additions & 93 deletions x/group/spec/group_policy.md
View file
Open in desktop

This file was deleted.

13 changes: 4 additions & 9 deletions x/group/spec/proposal.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
Groups can submit proposals in order to execute arbitrary transactions.
Authorization to execute messages is performed by the `msg_delegation.Keeper`'s
`DispatchActions` method. The actual protocol that a group uses to determine
whether a certain proposal passes are based on the group or group policy's
`DecisionProtocol`.
whether a certain proposal passes are based on the group accounts's
`DecisionPolicy`.

## Types

Expand Down Expand Up @@ -36,14 +36,9 @@ const (
```go
type MsgPropose struct {
Proposer sdk.AccAddress `json:"proposer"`
// Policy specifies the policy by which this propose will pass or fail.
// If set to DefaultGroupPolicy, the group's root DecisionProtocol is used. If
// it is set to another GroupPolicyID for this group, that policy's
// DecisionProtocol and its Capability grants will be used.
Policy GroupPolicyID `json:"policy"`
Group sdk.AccAddress `json:"group"`
GroupAccount sdk.AccAddress `json:"group_account"`
Msgs []sdk.Msg `json:"msgs"`
Description string `json:"Description,omitempty"`
Description string `json:"Description,omitempty"`
}
```

Expand Down