-
Notifications
You must be signed in to change notification settings - Fork 364
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
feat: r/gov/dao
v2 + r/sys/vars
#2581
base: master
Are you sure you want to change the base?
Changes from 5 commits
4dd7c58
aad859b
81396bf
dd5d64b
c342741
3d16ba6
8f2e3ac
ae0639a
3a7ba00
00056a5
209ee06
54c555d
84aeb30
322bc8c
794a353
8c45039
268021d
43e5c66
e1cd617
67e4535
8fbbd5e
fd13d02
ebfd3d2
b3856c7
4d3d94e
1f1d5d7
7fa7530
ef299bd
3d0c056
4396c48
24e0b08
4827d0b
26f21d8
71e8644
93d2b70
efe35cc
63cf23f
4520593
af65764
f567394
4ec9f3b
30ac5ff
c05289f
d8e5b7d
d320493
d33b0df
79be838
8c52893
74948fc
c9b4cc4
93ca6a8
ee77102
dbf0966
2f337a6
31d265f
908b949
6b3b81c
480a287
06fe4fe
dba0379
873ba9b
6ceb72a
8198c7f
8a51452
b0fa6b7
676b202
852291c
ae650d2
aa4f9ef
33343d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module gno.land/p/gov/dao | ||
|
||
require gno.land/p/gov/proposal v0.0.0-latest |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,34 @@ | ||||
package dao | ||||
|
||||
import ( | ||||
"std" | ||||
) | ||||
|
||||
// MemberStore defines the member storage abstraction | ||||
type MemberStore interface { | ||||
// GetMembers returns all members in the store | ||||
GetMembers() []Member | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Size() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also added for the
moul marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
|
||||
// IsMember returns a flag indicating if the given address | ||||
// belongs to a member | ||||
IsMember(address std.Address) bool | ||||
|
||||
// GetMember returns the requested member | ||||
GetMember(address std.Address) (Member, error) | ||||
|
||||
// AddMember attempts to add a member to the store | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "attempts"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed, have I |
||||
AddMember(member Member) error | ||||
|
||||
// RemoveMember attempts to remove a member from the store | ||||
RemoveMember(address std.Address) error | ||||
|
||||
// UpdateMember attempts to update the member in the store | ||||
UpdateMember(address std.Address, member Member) error | ||||
} | ||||
|
||||
// Member holds the relevant member information | ||||
type Member struct { | ||||
Address std.Address // bech32 gno address of the member (unique) | ||||
Name string // name associated with the member | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
we'll use r/users for this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropped: |
||||
VotingPower uint64 // the voting power of the member | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package dao | ||
|
||
import "std" | ||
|
||
type Status string | ||
|
||
// ACTIVE -> ACCEPTED -> EXECUTED | ||
// ACTIVE -> NOT ACCEPTED | ||
var ( | ||
Active Status = "active" // proposal is still active | ||
Accepted Status = "accepted" // proposal gathered quorum | ||
NotAccepted Status = "not accepted" // proposal failed to gather quorum | ||
Executed Status = "executed" // proposal is executed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Timedout There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Freeze? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, just noticed that it was the "DAO Status" please rename the type DAOStatus There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh no, not sorry; please, keep timedout, and freeze, but rename ProposalStatus There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed, and added: |
||
) | ||
|
||
func (s Status) String() string { | ||
return string(s) | ||
} | ||
|
||
// PropStore defines the proposal storage abstraction | ||
type PropStore interface { | ||
// GetProposals returns the given paginated proposals | ||
GetProposals(offset, count uint64) []Proposal | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe add a filter like active=true or a dedicated helper There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, we should not return a slice with all the proposals, but helper to get a specific one by ID, to know the size, and eventually to paginate, but by default, the API should discourage listing more than the last 10 ones or getting a specific one it's a bad gas optimization and a way to hit memory limit to let the current listing for such potentially-infinite objects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, we shouldn't expose this kind of infinite logic, especially on an open API like this (anyone can get the PropStore) I've limited it to a sane default ( |
||
|
||
// GetProposalByID returns the proposal associated with | ||
// the given ID, if any | ||
GetProposalByID(id uint64) (Proposal, error) | ||
|
||
// GetProposalsByAddress returns the proposals associated | ||
// with the given proposer address | ||
GetProposalsByAddress(address std.Address) []Proposal | ||
moul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// Proposal is the single proposal abstraction | ||
type Proposal interface { | ||
// GetAuthor returns the author of the proposal | ||
GetAuthor() std.Address | ||
|
||
// GetDescription returns the description of the proposal | ||
GetDescription() string | ||
|
||
// GetStatus returns the status of the proposal | ||
GetStatus() Status | ||
moul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// GetVotes returns the votes of the proposal | ||
GetVotes() []Vote | ||
moul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GetVoteByAuthor() Vote -> for the member portfolio. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added, along with unit tests: |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
package dao | ||
|
||
import ( | ||
"std" | ||
|
||
"gno.land/p/gov/proposal" | ||
) | ||
|
||
type VoteOption string | ||
|
||
const ( | ||
YesVote VoteOption = "YES" | ||
NoVote VoteOption = "NO" | ||
zivkovicmilos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
|
||
func (v VoteOption) String() string { | ||
return string(v) | ||
} | ||
|
||
// Vote encompasses a single GOVDAO vote | ||
type Vote struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. move to vote.gno There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved: |
||
Address std.Address // the address of the voter | ||
Option VoteOption // the voting option | ||
} | ||
|
||
// DAO defines the DAO abstraction | ||
type DAO interface { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. move to dao.gno There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved: |
||
// Propose adds a new proposal to the executor-based GOVDAO. | ||
// Returns the generated proposal ID | ||
Propose(comment string, executor proposal.Executor) (uint64, error) | ||
|
||
// VoteOnProposal adds a vote to the given proposal ID | ||
VoteOnProposal(id uint64, option VoteOption) error | ||
|
||
// ExecuteProposal executes the proposal with the given ID | ||
ExecuteProposal(id uint64) error | ||
|
||
// GetMembStore returns the member store associated with the DAO | ||
GetMembStore() MemberStore | ||
|
||
// GetPropStore returns the proposal store associated with the DAO | ||
GetPropStore() PropStore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's not an idea to have independent interfaces since they are tightly coupled to this top-level interface. I understand your preference for splitting things, but in this case, it's about making the DAO parameters unavailable from the MembStore and PrepStore, which is unfortunate because most of your helpers in the stores are checking if the caller is the DAO. I suggest embedding (or flattening) the interfaces in the main interface instead of returning an independent interface. This way, you can have a single struct implementing everything or decide to use embedded standalone MemberStore/PropStore in the implementation struct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is tight coupling with what the top level DAO API does, I agree 🙁 I wanted to preserve the shallow API for the DAO, but also not limit the functionality so we can create wrapper Realms to do much more sophisticated and complicated queries (ex. return JSON objects...) I've definitely thought about embedding the interfaces in Do we and pipe relevant methods, and let the caller do whatever they want with the package dao
var d dao.DAO
func init() {
d := simpledao.New(...)
}
func Propose(comment string, executor proposal.Executor) (uint64, error) {
return d.Propose(comment, executor)
}
func VoteOnProposal(id uint64, option VoteOption) error {
return d.VoteOnProposal(id, option)
}
func ExecuteProposal(id uint64) error {
return d.ExecuteProposal(id)
}
func GetDAO() dao.DAO {
return d
}
func Render(path string) string {
// ...
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Through this API, I just need clarify whether we need this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You’re right. Let’s remove the notion of member from p/gov/dao so that a DAO is an object that:
member management is something that can be made generic but it’s not because of a Dao, it’s just for anything that wants members. r/gov/dao should implements p/gov/dao (the proposal framework) AND p/something/membership; while relying on the proposal framework (injecting it?) to manage the members through proposals. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've applied this change, but can't find the exact commit 🤦♂️ |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,206 @@ | ||
package simpledao | ||
|
||
import ( | ||
"errors" | ||
"std" | ||
|
||
"gno.land/p/demo/ufmt" | ||
"gno.land/p/gov/dao" | ||
pproposal "gno.land/p/gov/proposal" | ||
) | ||
|
||
var ( | ||
errInvalidExecutor = errors.New("invalid executor provided") | ||
errInsufficientProposalFunds = errors.New("insufficient funds for proposal") | ||
errInsufficientExecuteFunds = errors.New("insufficient funds for executing proposal") | ||
errProposalExecuted = errors.New("proposal already executed") | ||
errProposalInactive = errors.New("proposal is inactive") | ||
errProposalExpired = errors.New("proposal is expired") | ||
errProposalNotAccepted = errors.New("proposal is not accepted") | ||
|
||
errNotGovDAO = errors.New("caller not correct govdao instance") | ||
) | ||
|
||
var ( | ||
minProposalFeeValue int64 = 100 * 1_000_000 // minimum gnot required for a govdao proposal (100 GNOT) | ||
minExecuteFeeValue int64 = 500 * 1_000_000 // minimum gnot required for a govdao proposal (500 GNOT) | ||
|
||
minProposalFee = std.NewCoin("ugnot", minProposalFeeValue) | ||
minExecuteFee = std.NewCoin("ugnot", minExecuteFeeValue) | ||
) | ||
|
||
// SimpleDAO is a simple DAO implementation | ||
type SimpleDAO struct { | ||
membStore *MembStore | ||
propStore *PropStore | ||
} | ||
|
||
// NewSimpleDAO creates a new instance of the simpledao DAO | ||
func NewSimpleDAO(membStore *MembStore, propStore *PropStore) *SimpleDAO { | ||
moul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return &SimpleDAO{ | ||
membStore: membStore, | ||
propStore: propStore, | ||
} | ||
} | ||
|
||
func (s *SimpleDAO) Propose(description string, executor pproposal.Executor) (uint64, error) { | ||
// Make sure the executor is set | ||
if executor == nil { | ||
return 0, errInvalidExecutor | ||
} | ||
|
||
var ( | ||
caller = getDAOCaller() | ||
sentCoins = std.GetOrigSend() // Get the sent coins, if any | ||
canCoverFee = sentCoins.AmountOf("ugnot") >= minProposalFee.Amount | ||
) | ||
|
||
// Check if the proposal is valid | ||
if !s.membStore.IsMember(caller) && !canCoverFee { | ||
return 0, errInsufficientProposalFunds | ||
} | ||
|
||
// Create the wrapped proposal | ||
prop := &proposal{ | ||
description: description, | ||
executor: executor, | ||
author: caller, | ||
votes: newVotes(), | ||
} | ||
|
||
// Add the proposal | ||
id, err := s.propStore.addProposal(prop) | ||
if err != nil { | ||
return 0, ufmt.Errorf("unable to add proposal, %s", err) | ||
} | ||
|
||
return id, nil | ||
} | ||
|
||
func (s *SimpleDAO) VoteOnProposal(id uint64, option dao.VoteOption) error { | ||
// Verify the GOVDAO member | ||
member, err := s.membStore.GetMember(getDAOCaller()) | ||
if err != nil { | ||
return ufmt.Errorf("unable to get govdao member, %s", err) | ||
} | ||
|
||
// Check if the proposal exists | ||
propRaw, err := s.propStore.GetProposalByID(id) | ||
if err != nil { | ||
return ufmt.Errorf("unable to get proposal %d, %s", id, err) | ||
} | ||
|
||
prop := propRaw.(*proposal) | ||
|
||
// Check the proposal status | ||
if prop.GetStatus() == dao.Executed { | ||
// Proposal was already executed, nothing to vote on anymore. | ||
// | ||
// In fact, the proposal should stop accepting | ||
// votes as soon as a 2/3+ majority is reached | ||
// on either option, but leaving the ability to vote still, | ||
// even if a proposal is accepted, or not accepted, | ||
// leaves room for "principle" vote decisions to be recorded | ||
return errProposalInactive | ||
} | ||
|
||
// Check if the proposal executor is expired | ||
if prop.executor.IsExpired() { | ||
return errProposalExpired | ||
} | ||
|
||
// Cast the vote | ||
if err = prop.votes.castVote(member, option); err != nil { | ||
return ufmt.Errorf("unable to vote on proposal %d, %s", id, err) | ||
} | ||
|
||
// Check the votes to see if quorum is reached | ||
var ( | ||
majorityPower = s.membStore.getMajorityPower() | ||
yays, nays = prop.votes.getTally() | ||
) | ||
|
||
switch { | ||
case yays > majorityPower: | ||
prop.status = dao.Accepted | ||
case nays > majorityPower: | ||
prop.status = dao.NotAccepted | ||
default: | ||
// Quorum not reached | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (s *SimpleDAO) ExecuteProposal(id uint64) error { | ||
var ( | ||
caller = getDAOCaller() | ||
sentCoins = std.GetOrigSend() // Get the sent coins, if any | ||
canCoverFee = sentCoins.AmountOf("ugnot") >= minExecuteFee.Amount | ||
) | ||
|
||
// Check if the non-DAO member can cover the execute fee | ||
if !s.membStore.IsMember(caller) && !canCoverFee { | ||
return errInsufficientExecuteFunds | ||
} | ||
|
||
// Check if the proposal exists | ||
propRaw, err := s.propStore.GetProposalByID(id) | ||
if err != nil { | ||
return ufmt.Errorf("unable to get proposal %d, %s", id, err) | ||
} | ||
|
||
prop := propRaw.(*proposal) | ||
|
||
// Check the proposal status | ||
if prop.GetStatus() != dao.Accepted { | ||
// Proposal is not accepted, cannot be executed | ||
return errProposalNotAccepted | ||
} | ||
|
||
// Check if the proposal is executed | ||
if prop.GetStatus() == dao.Executed { | ||
// Proposal is already executed | ||
return errProposalExecuted | ||
} | ||
|
||
// Check if proposal is expired | ||
if prop.executor.IsExpired() { | ||
return errProposalExpired | ||
} | ||
|
||
// Update the proposal status | ||
prop.status = dao.Executed | ||
|
||
// Attempt to execute the proposal | ||
if err = prop.executor.Execute(); err != nil { | ||
return ufmt.Errorf("error during proposal %d execution, %s", id, err) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (s *SimpleDAO) GetMembStore() dao.MemberStore { | ||
return s.membStore | ||
} | ||
|
||
func (s *SimpleDAO) GetPropStore() dao.PropStore { | ||
return s.propStore | ||
} | ||
|
||
// getDAOCaller returns the DAO caller. | ||
// XXX: This is not a great way to determine the caller, and it is very unsafe. | ||
// However, the current MsgRun context does not persist escaping the main() scope. | ||
// Until a better solution is developed, this enables proposals to be made through a package deployment + init() | ||
func getDAOCaller() std.Address { | ||
return std.GetOrigCaller() | ||
} | ||
|
||
// TODO change to r/sys/vars | ||
const daoPkgPath = "gno.land/r/gov/dao" | ||
|
||
// isCallerGOVDAO returns a flag indicating if the | ||
// current caller context is the active GOVDAO | ||
func isCallerGOVDAO() bool { | ||
moul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return std.CurrentRealm().PkgPath() == daoPkgPath | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deserves Its own package.
A DAO is not necessarily based on members. It could be composed of other DAOs, among other things.
Let's create a memberstore package for DAOs that require a memberstore, as well as other non-DAO entities that need a memberstore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved it out into a separate package:
2f337a6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move these two interfaces in p/demo/membstore,so that Dao.membstore does not depend on gov//dao?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by this?
The
dao.MemberStore
defines the interface that is now implemented inp/demo/membstore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced we should be moving the memberstore interface outside
p/gov/dao
, since this should be a shared interface that implementations fulfillThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until we have a way to to make anything addressable, including gno object, we should remove the member store from p/demo/dao.
A Dao isn’t necessarily about addressable users. And if for instance, we want to make interchain DAOs, we definitely don’t care at all about their members, we just do care about being able to propose things, and read proposals’ state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to pause for a second to grasp this concept. Powerful stuff
I've kept this commit stashed in case I got clarity on it, which I have now (
dao.DAO
is not the defactor API for DAOs yet, but a suggestion that should be moved out ofp/gov/dao
)Moved:
06fe4fe