BuidlGuidl / eth-tech-tree-challenges

This repository houses the many challenges of the Ethereum Development Tech Tree
MIT License
3 stars 9 forks source link

Moloch rage quit #32

Open KcPele opened 1 month ago

KcPele commented 1 month ago

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:

KcPele commented 1 month ago

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 https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/32#discussion_r1626256823 :

I am rethinking this.

  1. Let's make the propose method accept a contractAddr, data and value param (see #33 https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/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 https://github.com/BuidlGuidl/eth-tech-tree-challenges/pull/32#pullrequestreview-2096813495, or unsubscribe https://github.com/notifications/unsubscribe-auth/APJDTO4LVNSS3G6T4RBJJ4LZFXPQXAVCNFSM6AAAAABIQ6UQJOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJWHAYTGNBZGU . You are receiving this because you authored the thread.Message ID: @.***>

KcPele commented 1 week ago

@escottalexander, I want to get your feedback before I completely write out the test

KcPele commented 1 week ago

@escottalexander i have effected the changes

escottalexander commented 1 day ago

@KcPele Can you resolve the merge conflicts please?