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

fixed bug, tests, compiles #51

Merged
merged 26 commits into from
Aug 11, 2023
Merged

fixed bug, tests, compiles #51

merged 26 commits into from
Aug 11, 2023

Conversation

mejango
Copy link
Contributor

@mejango mejango commented Jul 24, 2023

@filipviz and I discovered a bug July 24, 2023 while helping project #548 raise funds for an auction.
The project had heldFees on. After failing to win the auction and upon returned the funds, the project was not made whole.
The bug is that the protocol expected a deposit equivalent to the amount paid out. In other words, if 10 ETH was paid out (before the fee), the protocol was expected a deposit of 10 ETH to return the full fee amount. The problem is the recipient doesn't have the full 10 ETH, they only have the amount after the fee. The protocol should only expect the a deposit of the amount paid out after fees... the amount the recipient actually has.

This PR introduces JBPayoutRedemptionTerminal3_1_2 that fixes this issue.

It also

  • moved fee calculations into a JBFees library
  • removes the isTerminalOf check from pay and addToBalance to reduce contract size to be deployable. In pay these checks are already made when minting tokens. Clients are now responsible for making sure this check is correct, otherwise the project can access the funds from the terminal by setting distribution limits in a subsequent cycle.

@mejango mejango marked this pull request as ready for review July 24, 2023 20:47
@mejango mejango merged commit b28cd7e into main Aug 11, 2023
2 checks passed
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