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

Add getter for number of unclaimed tokens in PaymentSplitter #3350

Merged
merged 7 commits into from
May 31, 2022

Conversation

alonbg
Copy link
Contributor

@alonbg alonbg commented Apr 22, 2022

payment assignment is done through unclaimed() functions

Fixes #????

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx
Copy link
Collaborator

Amxx commented Apr 22, 2022

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
Copy link
Contributor Author

alonbg commented Apr 23, 2022

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

cheers

@Amxx Amxx added this to the 4.7 milestone Apr 29, 2022
@frangio frangio changed the title refactor release functions Add getter for number of unclaimed tokens in PaymentSplitter May 6, 2022
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests and update the changelog.

@JorgeAtPaladin
Copy link

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
Copy link
Collaborator

Amxx commented May 13, 2022

  • _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
Copy link
Collaborator

Amxx commented May 13, 2022

  • 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
Copy link

JorgeAtPaladin commented May 13, 2022

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
Copy link
Collaborator

Amxx commented May 16, 2022

_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
Copy link
Collaborator

Amxx commented May 18, 2022

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

@frangio frangio self-assigned this May 30, 2022
move payment to unclaimed functions
@alonbg
Copy link
Contributor Author

alonbg commented May 31, 2022

@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

@alonbg alonbg requested a review from frangio May 31, 2022 17:00
frangio
frangio previously approved these changes May 31, 2022
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Amxx Amxx enabled auto-merge (squash) May 31, 2022 19:14
@Amxx Amxx merged commit 6766b2d into OpenZeppelin:master May 31, 2022
@Amxx
Copy link
Collaborator

Amxx commented May 31, 2022

Thank you @alonbg

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.

4 participants