Skip to content

Add AbstractSplitter #74

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jan 29, 2025

This is a prototype for a PaymentSplitter contract that supports dynamic shares.

Unlike the version we had in 4.x, this version allows minting/burning/transferring shares. The downside of this approach is that it only supports one token. If configured to handle (and split) ETH, then any ERC20 token sent to it would be locked*. Similarly, if it is configured to hangle (and split) USDC, then any USDT/Dai/... would be locked*.

* unless a recovery method is implemented.

@arr00
Copy link
Collaborator

arr00 commented Jan 31, 2025

If the balance or allocation exceeds type(int256).max, it could result in reverts for that account. I don't think it's a real issue though, because the user can always transfer shares over the max out, and then proceed as normally. It may be worth documenting in case a dev would restrict transferring while also dealing with massive numbers.

@Amxx
Copy link
Collaborator Author

Amxx commented Jan 31, 2025

because the user can always transfer shares over the max out, and then proceed as normally

If the computation of _allocation overflows, because _balance() + _totalReleased overflow, then:

  • no one can release
  • no one can transfer shares, because that includes a computation of _allocation to estimate the virtual shares to add/remove.
  • moving shares would not fix that anyway.

So this would basically kill the contract. Note that the v4 splitter had a similar issue, but with type(uint256).max instead of type(int256).max ... (the difference between the two is just a factor of 2)

@Amxx
Copy link
Collaborator Author

Amxx commented Jan 31, 2025

Just noticed: we need to test :

  • adding shares to an account that already have some share
  • partially burn the shares

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