Donations #41
Donations #41
Conversation
| size := extcodesize(addr) | ||
| } | ||
| return size > 0; | ||
| } |
There was a problem hiding this comment.
I think we should have some methods for removing a contributor. What happens if a contributor wallet address gets hacked. It would be nice if other members could propose to remove the hacked address and then add a new one.
| } | ||
|
|
||
| if (!proposals[newContributor].exists) { | ||
| if (_isContract(newContributor)) { |
There was a problem hiding this comment.
Curious to hear your reasoning on why we shouldn't allow a contract address. I don't see a reentrancy risk by allowing contracts and it would be nice if contributors could use Safe addresses instead of EOAs. It is generally frowned upon to check if an address has code in the smart contract layer because there are ways around it. For instance, lets pretend that reentrancy was possible/beneficial in this contract. If I wanted to I could pre-define the address for an exploiting contract but only deploy the contract after you added the address as a contributor. Lots of holes. But curious if you have other concerns I am not thinking about.
|
|
||
| modifier onlyContributors() { | ||
| bool contains; | ||
| for (uint8 i = 0; i < contributors.length; i++) { |
There was a problem hiding this comment.
Wonder if it would make more sense to create a mapping that stores the contributors mapping(address => bool) alongside the existing array of contributors and that would save us having to loop through this each time a call is made. Pretty sure it would be more gas efficient but I have not tested it.
| revert PaymentFailed(contributors[i], amount); | ||
| } | ||
| } | ||
| emit Donation(msg.sender, msg.value); |
There was a problem hiding this comment.
Does it make sense to fire this higher, like after line 122 so that even small donations are acknowledged. Otherwise this will only fire if the value is greater than the weights. Open to thoughts.
| if (totalWeiToSend < totalWeight) { | ||
| // Not enough balance, not worth splitting | ||
| return; | ||
| } |
There was a problem hiding this comment.
Just wanted to say that the above five lines are so simply genius. Nice work. There will be minuscule amounts left in the contract but it is not a problem because the contract is guaranteed to send what it can without encountering division issues. I will be duplicating this kind of simple yet effective solution in the future.
Implements a contract as a controller of donation mechanism.
I didn't include a "remove contributor" functionality.