Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Moloch rage quit #32
base: moloch-rage-quit
Are you sure you want to change the base?
Moloch rage quit #32
Changes from all commits
c32ae3e
f335446
adcf396
2fb44ed
a512149
787309d
0af893c
7f6d4ca
8a9c30c
b248d9a
accb922
5825570
f6c4832
0b42398
3bfee27
5467799
d76f4f7
04117ae
1714fda
aaf7327
1d39abe
19c2d05
51dc03f
eea0065
a87fe85
59f2654
610bdff
cbcd8b0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
These errors should be defined inside the contract. I realize the layout guidelines are confusing as it lists contracts below errors but what this means is contracts variables defined inside the contract.
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.
NotAMamberNotAMemberThere 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.
totalEth doesn't need to be defined or tracked in any way. You can get the contract balance with address(this).balance
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.
The way quorum is used is somewhat flawed. It should probably be replaced with a memberCount and then a formula is used to make sure a majority has voted. It should never allow one person to be the sole decider of a vote unless there is only one member.
In your current setup the only place they can set the quorum is in the constructor which it could never be set to more than 1 or else there would be no way to add a second member. If the quorum remains one indefinitely then one vote decides the outcomes for any proposal to no matter how many members there are. Yikes.
Perhaps there should be a minimum quorum of voters (maybe as low as 5%) that is required for a proposal to be approved but there is a more robust yea vs nay formula used after the deadline has passed. Because maybe only 10% of members voted and maybe that minimum quorum was reached but there were more nay votes than yea votes. I don't want to over complicate the contract so I am open to your ideas but the current mechanism is broken.
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.
No need for this to be a parameter. We should just use
msg.value
.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.
In regards to my suggestions for updating the
quorum
variable: Maybe the proposal should track yea votes and nay votes instead of just votes in general.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.
Remove
approved
on the proposal and instead track yea votes and nay votes. Approval depends on the proposal deadline being reached, total votes > quorum, and proposal.yeaVotes > proposal.nayVotesThere 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 think it makes sense for a proposal to be approved before the voting period is over. More nay votes might counteract the current approval. The approval/denial should be decided in the
executeProposal function
. That is the only place where it being approved/denied matters.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.
This function is overly complicated. It might help the user to instead have a
executeApprovedProposal
function and arefundDeniedProposal
function. Each function could check if Yea > Nay or the opposite before executing.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 nothing to keep me from executing a proposal multiple times. Yikes! Make sure you set the new property before
call
ing external contracts because this is a reentrancy risk. Would be great to add tests around this too.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.
This check is the only thing keeping someone from executing the proposal multiple times. But what if I get in at a low share price and Rage Quit at a high share price? Then I can become a member again at my low share price by reexecuting my old proposal! I could do this over and over until the contract was drained.