-
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?
Conversation
} | ||
|
||
// DAO defines the DAO abstraction | ||
type DAO interface { |
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.
move to dao.gno
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.
Moved:
} | ||
|
||
// Vote encompasses a single GOVDAO vote | ||
type Vote struct { |
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.
move to vote.gno
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.
Moved:
// GetVotes returns the votes of the proposal | ||
GetVotes() []Vote | ||
} |
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.
GetVoteByAuthor() Vote
-> for the member portfolio.
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.
Added, along with unit tests:
// 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 comment
The 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 comment
The 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 comment
The 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 (10
):
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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
- add ExecutedError
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.
Renamed, and added:
// 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Also added for the PropStore
:
// 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, have I
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Name string // name associated with the member |
we'll use r/users for this
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.
Dropped:
errInvalidAddressUpdate = errors.New("invalid address update") | ||
) | ||
|
||
type MembStoreOption func(*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.
How is this pattern future-proof if all the helpers can only be implemented locally (the struct only has private objects)?
I'm already skeptical about this pattern in Go, but I believe it's even worse in Gno. Opinions?
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.
It's not future proof by any means, you'd be dependent on the options provided by the MembStore
implementation (for now only 1). The future proofing is that you can swap the govdao implementation at any point if you want a different API
The original DAO API had swap methods for both the PropStore
and MembStore
, so you could change the implementations without changing the DAO, but I decided against this in the end to not overcomplicate the API more
// 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 comment
The 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 comment
The 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 DAO
, but I'm struggling to understand how this is a better option than having 2 getters, if we want to use a DAO
interface in /r/gov/dao
. What if there is a DAO implementation that doesn't publicly expose changing the member set (ex. returns nil
for GetMembStore()
)?
Do we and pipe relevant methods, and let the caller do whatever they want with the DAO
through a fetcher?
What I'm essentially asking is, does our r/gov/dao
realm look like this?
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Through this API, r/gov/dao
is minimal, and we don't have a gazillion methods in our public API, but let the caller wrap them through another Realm that calls GetDAO
I just need clarify whether we need this:
What if there is a DAO implementation that doesn't publicly expose changing the member set (ex. returns nil for GetMembStore())?
- If the answer is yes, this is possible, I'd prefer to keep the
GetXXX
on theDAO
interface, and not embed - If the answer is no, I see no reason for not embedding
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.
You’re right.
Let’s remove the notion of member from p/gov/dao so that a DAO is an object that:
- handles proposals
- have assets
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I've applied this change, but can't find the exact commit 🤦♂️
The high-level feedback is:
|
// Execute runs the executor's callback function. | ||
func (exec *CallbackExecutor) Execute() error { | ||
// Verify the executor is r/gov/dao | ||
assertCaller(exec.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.
here you panic, after you return an error, maybe you should always return an error to be consistent.
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 agree, it's cleaner to return, since Execute
already supports an error:
// YayPercent returns the percentage (0-100) of the yay votes | ||
// in relation to the total voting power | ||
func (v Stats) YayPercent() uint64 { | ||
return (v.YayVotes / v.TotalVotingPower) * 100 |
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.
return (v.YayVotes / v.TotalVotingPower) * 100 | |
return v.YayVotes * 100 / v.TotalVotingPower |
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.
Swapped 🙃
// executor guarantees it, because | ||
// the API of the member store is public and callable | ||
// by anyone who has a reference to the member store instance. | ||
func (m *MembStore) isCallerGOVDAO() bool { |
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.
func (m *MembStore) isCallerGOVDAO() bool { | |
func (m *MembStore) isCallerDAORealm() bool { |
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.
Please, remove the term “GovDAO” from p/ generic pure libraries.
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.
Renamed:
[852291c](https://github.com/gnolang/gno/pull/2581/commits/852291cb342d7eacdce178ae2f24084767a77126)
// the API of the member store is public and callable | ||
// by anyone who has a reference to the member store instance. | ||
func (m *MembStore) isCallerGOVDAO() bool { | ||
return m.daoPkgPath == "" || std.CurrentRealm().PkgPath() == m.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.
Is the first part of the if to handle a specific case, like the initialization? I think we should remove it.
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.
By default the membstore doesn't have a dao Realm guard, but you can initialize it through options:
gno/examples/gno.land/p/demo/membstore/membstore.gno
Lines 35 to 41 in 852291c
// WithDAOPkgPath initializes the member store | |
// with a dao package path guard | |
func WithDAOPkgPath(daoPkgPath string) Option { | |
return func(store *MembStore) { | |
store.daoPkgPath = daoPkgPath | |
} | |
} |
Knowing that you're a big fan of the options pattern, I'm assuming you want to remove this and just always have the dao Realm guard present from init? 🙂
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.
Why do we need it?
If the plan is to expose the membstore as a safe object, I don't see why we should allow constructing a membstore without a pkgpath.
If the plan isn't to expose it, we should expect the using contract to keep the variable private, nope?
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 a big fan of the unnecessary complexity behind the options pattern. However, I dislike it even more when it leads to the creation of new, strange cases.
In my opinion, the New function should always accept all required parameters as direct arguments. If you identify truly optional fields, you can use an "opts" struct. Otherwise, expect users to utilize the zero value when the number of parameters is small. The New constructor should panic if the configuration is invalid.
@@ -0,0 +1,32 @@ | |||
package 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.
add a package-level comment to explain the role of this p/
} | ||
|
||
// Emit the vote cast event | ||
std.Emit( |
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 this be a helper like proposal.EmitAddition
?
} | ||
|
||
// Vote encompasses a single GOVDAO vote | ||
type Vote struct { |
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.
deadcode
@@ -9,7 +9,7 @@ import ( | |||
) | |||
|
|||
const ( | |||
valRealm = "gno.land/r/sys/validators" | |||
valRealm = "gno.land/r/sys/validators/v2" |
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.
valRealm = "gno.land/r/sys/validators/v2" | |
valRealm = "gno.land/r/sys/validators/v2" // XXX: make it configurable from GovDAO |
@@ -0,0 +1,10 @@ | |||
module gno.land/r/sys/vars |
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.
please move r/sys/vars to a standalone draft PR
|
||
// IsExpired returns a flag indicating if the executor is expired. | ||
// The CallbackExecutor never expires | ||
func (exec *CallbackExecutor) IsExpired() bool { |
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 if we simply remove this method?
Perhaps the only entity that should concern itself with expiration is the proposal, not the executor. If a proposal expires, we can expect it to eventually set the executor pointer to nil
to allow for GC.
"gno.land/p/demo/context" | ||
) | ||
|
||
type propContextKey string |
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.
By the way, I need to rewrite the p/demo/context because it is currently unsafe to use. We are not storing the private context key type, which means we could potentially steal a key from another realm.
I have a solution that involves using PrevRealm() as the prefix for a key.
There is nothing to change in this PR, but we must remember that it is currently unsafe.
import "strings" | ||
|
||
// CombinedError is a combined execution error | ||
type CombinedError struct { |
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.
p/combinederr
? :)
"gno.land/p/demo/context" | ||
"gno.land/p/gov/proposal" | ||
"gno.land/p/demo/dao" | ||
govdao "gno.land/r/gov/dao/v2" |
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.
should not depend on a specific govdao version.
"gno.land/p/demo/ufmt" | ||
pVals "gno.land/p/sys/validators" | ||
govdao "gno.land/r/gov/dao" | ||
"gno.land/r/sys/validators" | ||
govdao "gno.land/r/gov/dao/v2" |
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.
We should find a solution to avoid depending on a specific GovDAO version.
You can either use another mechanism or create a r/valopers/v2/govdaov2
helper package. This will allow us to build the necessary bridge if we switch to GovDAO v3.
Let me know if you can't find an elegant solution, and I can help you brainstorm one.
// Craft the proposal comment | ||
comment := ufmt.Sprintf( | ||
// Craft the proposal description | ||
description := ufmt.Sprintf( | ||
"Proposal to add valoper %s (Address: %s; PubKey: %s) to the valset", |
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.
"Proposal to add valoper %s (Address: %s; PubKey: %s) to the valset", | |
"Add valoper %s (Address: %s; PubKey: %s) to the valset", |
|
||
// maxRequestVotes is the maximum number of | ||
// paginated votes that can be requested | ||
maxRequestVotes = 50 |
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.
unused
gno.land/p/nt/poa v0.0.0-latest | ||
gno.land/p/sys/validators v0.0.0-latest | ||
gno.land/r/gov/dao/v2 v0.0.0-latest |
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.
r/sys/validators shouldn't depend on r/gov/dao/v2
} | ||
|
||
// Proposal is the single proposal abstraction | ||
type Proposal interface { |
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.
Note to self: Wouldn't it be better and more secure to use a struct here? What if we define a custom struct that implements the interface for an attack?
@@ -0,0 +1,70 @@ | |||
package 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.
Add a comment to explain that the vote will be removed from this p/demo/dao package. A DAO shouldn't have to comply with or define how the voting mechanism works internally; it should be viewed as an entity that makes decisions. Therefore, we should focus on the proposal rather than the votes.
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 want to merge this PR soon, so I'm in favor of just adding the comment. However, please note that this is the biggest weakness of this implementation, and it's the only issue I see that will require us to create a v2.
func WithInitialMembers(members []Member) Option { | ||
return func(store *MembStore) { | ||
for _, m := range members { | ||
store.members.Set(m.Address.String(), m) |
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.
check for ErrAlreadyMember
status dao.ProposalStatus // status of the proposal | ||
|
||
tally *tally // voting tally | ||
getTotalVotingPowerFn func() uint64 // callback for the total voting power |
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 don't get the reason for having this callback.
Description
This PR introduces an upgrade to the
r/gov/dao
system:p/demo/simpledao
implementationr/sys/vars
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description