Skip to content
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

Feature SplitPayment contract #417

Merged
merged 3 commits into from Nov 10, 2017

Conversation

abarmat
Copy link
Contributor

@abarmat abarmat commented Aug 29, 2017

Contract that allows distributing payments done to the registered payees based on their shares.

Expose a distributeFunds function callable by any of the payees to perform the distribution of funds.

Also a SplitPullPayment contract to use PullPayment to allow delayed withdrawal of funds by each payee.

@abarmat
Copy link
Contributor Author

abarmat commented Aug 30, 2017

We can have an even slimmer SplitPayment contract by removing addPayeeMany and many of the getters that can be implemented by child classes.

@frangio
Copy link
Contributor

frangio commented Sep 1, 2017

I like this idea! I would do a bit of a refactoring, though, and remove the struct. I don't think it's necessary, it would be enough to replace payeeIndex and payees by the following:

mapping (address => uint256) shares;
address[] payees;

We could even do without the payees array if the API was directly like PullPayments. I mean, if instead of distributeFunds there was a function claim() which had to be called by each payee to take their share. This is a somewhat different implementation, though, so let me know what you think.

Agree with your comment on getters! Plus removing the struct would remove the need mostly.

I like the idea of an overrideable canUpdate, but there should be a safer default! This is the kind of situation that #366 is about, and I think the role based access control approach would be useful here.

@frangio
Copy link
Contributor

frangio commented Sep 1, 2017

By the way, Travis failed because we're running tests in Solidity 0.4.13. Unless there's a specific bug in that version that would affect this contract, can you change the pragma line to ^0.4.11 (like the rest of the repository) or at least ^0.4.13?

@abarmat
Copy link
Contributor Author

abarmat commented Sep 4, 2017

Will do the changes, thank you for the feedback!

Let me thing about changing the interface to a claim() without distribute(). I didn't want to traverse the array of payees to do the allocation every time a payment is received to avoid extra gas costs to the sender. There might be a different way to do it.

@frangio
Copy link
Contributor

frangio commented Sep 4, 2017

Right, you can solve it without iterating over the array by keeping a variable with the amount of ether that has already been released, and a mapping with how much has been released for each participant.

I did something very similar in this TokenVesting contract (#412).

Hm, though I don't think this works if the list of participants is dynamic. Do you think it's necessary to be able to add participants after the contract is running?

@abarmat
Copy link
Contributor Author

abarmat commented Sep 9, 2017

I updated the contract removing most of the getters and implemented it to resemble how the PullPayment contract works, with a claim() method for payees to take their cut. Please let me know your thoughts.

@frangio
Copy link
Contributor

frangio commented Nov 10, 2017

@abarmat Looks really great! Nice addition. :-)

Will merge after Travis finishes.

@frangio frangio merged commit e72c7ce into OpenZeppelin:master Nov 10, 2017
ProphetDaniel pushed a commit to classicdelta/Smart-Contracts that referenced this pull request Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants