Skip to content

Feature SplitPayment contract#417

Merged
frangio merged 3 commits into
OpenZeppelin:masterfrom
abarmat:feature-splitpayment-contract
Nov 10, 2017
Merged

Feature SplitPayment contract#417
frangio merged 3 commits into
OpenZeppelin:masterfrom
abarmat:feature-splitpayment-contract

Conversation

@abarmat

@abarmat abarmat commented Aug 29, 2017

Copy link
Copy Markdown
Contributor

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

abarmat commented Aug 30, 2017

Copy link
Copy Markdown
Contributor Author

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

@frangio

frangio commented Sep 1, 2017

Copy link
Copy Markdown
Contributor

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

frangio commented Sep 1, 2017

Copy link
Copy Markdown
Contributor

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

abarmat commented Sep 4, 2017

Copy link
Copy Markdown
Contributor Author

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

frangio commented Sep 4, 2017

Copy link
Copy Markdown
Contributor

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

abarmat commented Sep 9, 2017

Copy link
Copy Markdown
Contributor Author

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

frangio commented Nov 10, 2017

Copy link
Copy Markdown
Contributor

@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.

2 participants