Add getter for number of unclaimed tokens in PaymentSplitter

[alonbg]

payment assignment is done through unclaimed() functions

Fixes #????

PR Checklist

• Tests
• Documentation
• Changelog entry

[Amxx]

Hello @alonbg and thank you for your PR.

This is something that I wanted to include for quite a while. I really think it has its place in this contract. I was personally considering the "claimable" name, but "unclaimed" is also nice.

We'll need a changelog entry and some tests, but it should be simple enough to get into the next release.

[alonbg]

Sure @Amxx
actually unclaimed sounds a bit judgmental.
While claimable states the condition and not what should have been done but wasn't. :)

cheers

[frangio]

Please add tests and update the changelog.

[JorgeAtPaladin]

When we audited PaymentSplitter, we found this lacking as well. Here are the other lacking features we would love to see in this contract:

• _payees lacks length public function

• _addPayee should document the side-effect of calling it after users have already started releasing tokens: Line 171 can currently underflow and revert ambiguously because of this.

• accounts should grant senders permission to allow calling release to them, as if the account is a smart contract, they might want to account for the funds they received by the PaymentSplitter and execute logic with them. Recipients might want not want to give everyone permission to release to them.

My apologies for dumping them here. Keep up the great work!

[Amxx]

• _addPayee should document the side-effect of calling it after users have already started releasing tokens: Line 171 can currently underflow and revert ambiguously because of this.

_addPayee is private, so the user cannot call it outside of the normal workflow (at construction). To call it later, they would have to modify the code to make it internal, which IMO falls outside the scope of what we can properly guarantee/document

[Amxx]

• accounts should grant senders permission to allow calling release to them, as if the account is a smart contract, they might want to account for the funds they received by the PaymentSplitter and execute logic with them. Recipients might want not want to give everyone permission to release to them.

This is interesting. I would personally reverse the argument. If a smart contract is a recipient, and if it is not "payment splitter aware" then it might not have the ability to release or to give permission to release. This will result in locked funds.

Now you could easily override the release function to add restrictions. If we were to include them by default, it would be more difficult/expensive to try to remove/circumvent them if you don't need/want them. Thus I think the base version should not include this kind of permission system.

[JorgeAtPaladin]

This is interesting. I would personally reverse the argument. If a smart contract is a recipient, and if it is not "payment splitter aware" then it might not have the ability to release or to give permission to release. This will result in locked funds.

The trick is to default to "grant allowance to all", this would be most compatible with the current implementation (all though obviously still breaking some compatibility).

_addPayee is private, so the user cannot call it outside of the normal workflow (at construction). To call it later, they would have to modify the code to make it internal, which IMO falls outside the scope of what we can properly guarantee/document

Right, my team was looking at code that extended PaymentSplitter. We weren't looking at this contract as if it should be deployed "as-is" 👍 But as it's private and not internal it's furthermore a non-issue, like you indicate.

[Amxx]

_addPayee is private, so the user cannot call it outside of the normal workflow (at construction). To call it later, they would have to modify the code to make it internal, which IMO falls outside the scope of what we can properly guarantee/document

Right, my team was looking at code that extended PaymentSplitter. We weren't looking at this contract as if it should be deployed "as-is" +1 But as it's private and not internal it's furthermore a non-issue, like you indicate.

Such an extension of the payment splitter was already discussed in another issue. A prototype is available here

[Amxx]

@alonbg could you please rename the new function from unclaimed to releasable.

We will also need some tests and a Changelog entry in order to merge the PR.

[alonbg]

@alonbg could you please rename the new function from unclaimed to releasable.

We will also need some tests and a Changelog entry in order to merge the PR.

done @Amxx
as for tests - release functions already have tests and new releasable functions are called within.
cc: @frangio

[frangio]

Thank you @alonbg! We do require tests. I've added some.

[Amxx]

Thank you @alonbg