-
Notifications
You must be signed in to change notification settings - Fork 441
feat(govdao): add proposal fee-based for non-member #4944
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
base: master
Are you sure you want to change the base?
Conversation
🛠 PR Checks Summary🔴 Pending initial approval by a review team member, or review from tech-staff Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
c0302d5 to
ea1804b
Compare
|
|
||
| // Test: Non-member with insufficient fee should also fail with realm validation error | ||
| testing.SetOriginSend(chain.Coins{chain.Coin{Denom: "ugnot", Amount: 500000}}) // 0.5 GNOT - insufficient | ||
| uassert.AbortsContains(t, "proposal creation must be done directly by a user or through the r/gov/dao proxy", func(cur realm) { |
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 the error here should say "proposal creation must be done directly by a user or through the r/gov/dao proxy", i would expect the test to ensure there is a "insufficient fee, user must send 1_000_000 gnot" or smth like this, same for all your test in this function.
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.
Fix: 856c91e
| if member.InvitationPoints <= 0 { | ||
| panic("proposer does not have enough invitation points for inviting new people to the board") | ||
| } |
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.
for me this line should never have been there, invitations point should be only for the T3 AddMember function but since it was here it makes me wondering if we should keep it in case the proposal is coming from a 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.
I agree, in the end this is just a helper function to create a ProposalRequest object, any non-member could bypass this condition by creating their own ProposalRequest object.
But I don't think we should keep it as it is not protective, or we should rethink how invitationPoint work in GovDAO.
| var ErrMemberNotFound = errors.New("member not found") | ||
|
|
||
| // ProposalFeeAmount is the fee required for non-members to create proposals (in ugnot) | ||
| const ProposalFeeAmount int64 = 1000000 // 1 GNOT |
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 we create a new helper function to create a proposal request containing the executor callback to update this number ?
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.
Fix: 856c91e
Co-authored-by: MikaelVallenet <[email protected]>
… for non-members + fix tests
| func NewProposalFeeAmountRequest(newAmount int64, reason string) dao.ProposalRequest { | ||
| if newAmount < 0 { | ||
| panic("proposal fee amount must be non-negative") | ||
| } | ||
|
|
||
| if reason == "" { | ||
| panic("proposal fee amount change requires a reason") | ||
| } |
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 we should add a txtar test flow of changing the proposal fee else the PR looks good to me i will approve once this done
Close: #2520
I set an arbitrary fee of
1_000_000 ugnotfor a non member proposal.Coins are stored in GovDAO implementation, and can be use by the DAO.
I've removed invitation point on NewMemberProposalRequest, as it doesn't really make sense anymore. Though it is still necessary in
AddMember.Outside of the test files, it is not testable.
See #4866