Moloch rage quit #32
Moloch rage quit #32
Conversation
|
Great review
…On Tue, 4 Jun 2024, 16:55 escottalexander, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I have some bigger thoughts for you that will require larger changes.
------------------------------
On packages/foundry/contracts/MolochRageQuit.sol
<BuidlGuidl/eth-tech-tree-challenges#32 (comment)>
:
I am rethinking this.
1. Let's make the propose method accept a contractAddr, data and value
param (see #33
<BuidlGuidl/eth-tech-tree-challenges#33> for
an example of this) and remove ethAmount and shareAmount from the
struct. Also add a deadline param so that there is a time window for
voting.
2. Add a new method called executeProposal or similar. Anyone can call
it but it will check that the proposal has been approved and the proposal
deadline is over. If true then it executes the data with the value. This
way the method can be used to execute any transaction (not just approving
members). If the proposal is rejected then it should refund any value that
was deposited when the proposal was made.
3. Change the addMember method to have a modifier that only allows it
to be called as the result of a proposal being executed (maybe msg.sender
== address(this)?). addMember should accept all the params needed to
give shares to the proposed new member.
4. removeMember should be very similar to add addMember but perform
the opposite logic.
As you think through it you may find you have to add/remove other methods
and logic that I haven't thought of yet.
—
Reply to this email directly, view it on GitHub
<BuidlGuidl/eth-tech-tree-challenges#32 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APJDTO4LVNSS3G6T4RBJJ4LZFXPQXAVCNFSM6AAAAABIQ6UQJOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJWHAYTGNBZGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
@escottalexander, I want to get your feedback before I completely write out the test |
|
@escottalexander i have effected the changes |
|
@KcPele Can you resolve the merge conflicts please? |
|
|
||
| if (proposal.votes >= quorum) { | ||
| proposal.approved = true; | ||
| emit ProposalApproved(proposalId, msg.sender); |
There was a problem hiding this comment.
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.
| * - if the proposal is approved and the deadline has passed: execute the calldata with the value. Reverts with MolochRageQuit__FailedToExecute if the execution fails. | ||
| * - Emit a `ProposalExecuted` event if the proposal is executed. | ||
| */ | ||
| function executeProposal( |
There was a problem hiding this comment.
This function is overly complicated. It might help the user to instead have a executeApprovedProposal function and a refundDeniedProposal function. Each function could check if Yea > Nay or the opposite before executing.
There was a problem hiding this comment.
There is nothing to keep me from executing a proposal multiple times. Yikes! Make sure you set the new property before calling external contracts because this is a reentrancy risk. Would be great to add tests around this too.
| emit Voted(proposalId, msg.sender); | ||
|
|
||
| if (proposal.votes >= quorum) { | ||
| proposal.approved = true; |
There was a problem hiding this comment.
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.nayVotes
| { | ||
| Proposal storage proposal = proposals[proposalId]; | ||
|
|
||
| if (members[newMember]) { |
There was a problem hiding this comment.
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.
Description
Concise description of proposed changes, We recommend using screenshots and videos for better description
Additional Information
Related Issues
Closes #{issue number}
Note: If your changes are small and straightforward, you may skip the creation of an issue beforehand and remove this section. However, for medium-to-large changes, it is recommended to have an open issue for discussion and approval prior to submitting a pull request.
Your ENS/address: