Adding fee calculation for deposit and mint functions #151
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a user calls a deposit function providing assets amount, the fee should be included in that amount. Meaning that the actual deposit will be less, ie deposit(assets - fee, receiverOfShares) When a user calls a mint function providing total shares that should be transferred to his account, a total approved assets should also include fee. A `previewMint(shares)` function can be called to find out how much assets should be approved and transferred in order to receive `shares` amount.
dimpar
commented
Jan 12, 2024
Initializing fee basis point to 5 bps just like in tbtcv2. Ronding fee calculation to Trunc (towards zero) which means truncating any fractions. This amount is so small that should not play any significant role, but it would nicely match the testing results for bigint types that also use they same rounding method.
Inspired by OZ ERC4626 Fee example here we add functionality that calculates fee on entry during deposit and mint functions. Fee configuration will be overriden in Acre contract.
Some of the calculation was moved to a dedicated ERC4626Fees contract that is going to deal with all the fees upon entry and exit. Here we leave only fee configuration.
The treasury address will be used as a destination to transfer fees.
nkuba
reviewed
Jan 16, 2024
Closed
This is needed for correct calculation of the actual staked amount in Acre excluding fees.
By introducing entry fees, the mint() function should also take into account fees. Actual staked amount must not be less than the minimum staked amount param, however a user should deposit assets that also cover fees. Therefore, we should subtract fees from the actual staked amount and then compare it with the min staked param.
A lot of tests broke becuase we introduced fees into the deposit and minting function. This commit fixes some of the current tests and recalcute the numbers. Additional tests were added for the public functions of `previewDeposit()` and `previewMint()`.
nkuba
reviewed
Jan 17, 2024
Once we add upgradability the initialize() function will set the values. Now, this step will help a bit with future upgradability changes.
- Verified the updated process - Verified the deposit functionality when there's no entry fees
nkuba
reviewed
Jan 18, 2024
It turned out that it is better to stick with the original implementation to prevent any potential issues.
We ageed that minimumDepositAmount should also account for treasury fees. This decision has two sides. On the one hand a user deposits an amount and can expect that this is what is actually staked, but in reality the fee is subtracted first and then what's left is being staked. On the other hand minimumDepositAmount is a fixed value (unless changed by the owner) and won't changed if the fee is changed, which makes it consistent value. This is how tbtcv2 works and we mimic this behavior here. Renamed DepositAmountLessThanMin error to LessThanMinDeposit to align how EIP4626 name their custom errors
Aligned feeOnTotal and feeOnRaw functions with how Solidity handles rounding towards infinity. Fixed math when calculating assets and shares when minting stBTC and previewing actual mints.
Added a check with custom error in case a new treasury address is about to be set to zero. Added tests to verify treasury address during the contract deployment and during the update by the governance.
nkuba
reviewed
Jan 25, 2024
Closed
Enforced an inheriting contract to implement entry fee basis points and fee recipient even if it is meant to be set to 0 or 0 address. A dev should do it intentionally.
nkuba
reviewed
Jan 26, 2024
We do not need to take a state snapshot before each test in these cases. It is enough to do it once.
Refactorred and checked some core components that are part of the overall deposit / mint fee calculations. A couple of tests have hardcoded values, but most of the tests use helper functions to check the correctness of fee calculations.
Each deposit collects some fee from a user. Max deposit returns unused tBTC balance in the Acre system, but it does not count for the entry fees. It results in some unsuded dust left. If the dust is below a min amount of deposit, this function will return 0, so that users won't be confused when they actually want to deposit below the min threshold.
nkuba
reviewed
Jan 31, 2024
Extracted tests for these internal functions to a separate describe block.
- There is a Math lib from OZ that is not needed in Acre contract. Removed. - Fixed some spelling in the comments and formatted docs.
Acre address cannot be used for treasury, just like we check it during fee calculation upon deposit.
nkuba
previously approved these changes
Feb 1, 2024
r-czajkowski
previously approved these changes
Feb 1, 2024
We are anticipating more third party abstract classes to be added to acre. This contract can set the pattern so every other abstract third party code can land here.
nkuba
approved these changes
Feb 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds treasury fees upon depositing and minting calls. Entry fees are calculated based on the basis points. Exit fee will have a different value and will be added in the different PR. There is a concept of
minimumAmountDeposit. A user can deposit adeposit(minimumAmountDeposit)which also includes fees. This makes it a more constant value, because if the fees change, this value stays the same.Treasury address functionality were cherry-picked from this PR: #91. Depends of slither fixes.
Inspiration for fees calculation by OpenZeppelin: https://docs.openzeppelin.com/contracts/5.x/erc4626#fees