Staking limits #68
Merged
Staking limits #68
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
Store staking-related parameters in struct such as minimum amount for a single deposit operation and maximum total amount og tBTC token held by Acre. In the future, we should add other staking-related parametrs here.
Only deposits where the amount is greater than or equal to the minimum amount for a single deposit should be possible.
We want to control how many tBTC tokens the Acre contract can hold. Here we override the `maxDeposit` function that takes into account the staking param, maximum total assets, which determines the total amount of tBTC token held by Acre.
In ERC4626 standard users can deposit using 2 functions `mint` and `deposit` so we need to take into account staking limits in `mint` function to not allow deposits that break staking limts rules.
Add function that updates parameters of staking. Only owner can update these parameters so here we also use the `Ownable` contract from `OZ` library to use the `onlyOwner` modifier. Requirements: - Minimum deposit amount must be greater than zero, - Minimum deposit amount must be greater than or equal to the minimum deposit amount in the tBTC system, - Maximum total assets must be greater than zero.
We added a new role `owner` to the test fixture and we need to make sure we call functions with correct staker account.
r-czajkowski
force-pushed
the
staking-limits
branch
from
0088734 to
a5d022c
Compare
Fix too many digits error by using `ether` suffix as recomended in slither docs.
Since the `Acre` contract inherits from `Ownable` contract from OZ lib we need to pass `initialOwner` to the Acre contract constructor.
nkuba
reviewed
Dec 8, 2023
There is no need to assign value from `super.deposit` to the new variable. We can return directly.
nkuba
reviewed
Dec 11, 2023
dimpar
reviewed
Dec 11, 2023
dimpar
reviewed
Dec 11, 2023
dimpar
reviewed
Dec 11, 2023
We can pass `msg.sender` to the `Ownable` constructor and later in the deployment scripts transfer the ownership at the end of the deployment scenario.
Cover a case where `totalAssets()` value is greater than `stakingParameters.maxiumumTotalAssets`. This can happen when vaults generated yield or `maximumTotalAssets` has been updated to a lower value.
Remove unnecessary requirement. `0.01` BTC is tBTC minting process limitation which is expected to be invoked before `Acre.stake` function.
Leave a TODO to remember about introducing a governable parameters update process.
The ERC4626 already uses custom errors, so it would be nice to have them here for consistency.
nkuba
reviewed
Dec 21, 2023
Use `convertToShares` instead of `previewDeposit` in tests - `convertToShares` does not take fees into account.
For the first staker, shares and stkaed amount will be the same number, we don't need to call `previewDeposit`.
We should allow to set the maximum total assets value to `0` so the `maxDeposit` function can return `0` when, for example, deposits are disabled.
We need a way to remove the limit for deposits. In such cases, we will set the value of `maximumTotalAssets` to the maximum (`type(uint256).max`).
We need to handle a case where there is no maximum limit for deposits.
We overridden `deposit` function and now it takes into account the minimum deposit amount parameter - here we add unit test for this function to cover a cases where the deposit amount is less than minimum and equal to the minimum amount. We indirectly cover this check's test in `stake` tests but we want to add an explicit `deposit` function test that covers it as well.
r-czajkowski
force-pushed
the
staking-limits
branch
3 times, most recently
from
4dcefe9 to
43074ab
Compare
nkuba
reviewed
Dec 29, 2023
r-czajkowski
force-pushed
the
staking-limits
branch
from
43074ab to
771553b
Compare
`parametrs` -> `parameters` Co-authored-by: Jakub Nowakowski <[email protected]>
Add a case where total tBTC amount after staking would be equal the max amount, to confirm the transaction will succeed.
nkuba
approved these changes
Dec 29, 2023
dimpar
reviewed
Dec 29, 2023
| /// Acre. | ||
| /// @return The maximum amount of the tBTC token. | ||
| function maxDeposit(address) public view override returns (uint256) { | ||
| if (maximumTotalAssets == type(uint256).max) { |
There was a problem hiding this comment.
It says that
// Maximum total amount of tBTC token held by Acre.
maximumTotalAssets
tBTC is pegged to BTC. As of now, max total supply of BTC is capped to 21M. No way BTC will reach max of uint256, unless something really ground breaking will happen to the whole crypto space. I don't think this check is necessary.
There was a problem hiding this comment.
The max uint256 value is used to disable the limit. If the maximumTotalAssets is set to the maxuint256, the function won't bother with further calculations and immediately returns the max uint256 value.
There was a problem hiding this comment.
Ah, but this is a view function that doesn't cost anything even if we do the calculation. I think it's a bit confusing and I know that uint256 is a big number, but logically it's not 100% correct, because if there are already assets in Acre, then maxDeposit should return maxuint256 - totalAssets, not just maxuint256. I don't think it can bite us though. Up to you guys.
nkuba
reviewed
Dec 29, 2023
dimpar
reviewed
Dec 29, 2023
nkuba
added a commit
that referenced
this pull request
Dec 29, 2023
Ref: #68 (comment) Add missing snapshot restorer to `deposit` tests according to the existing pattern.
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.
Closes: #59
This PR implements staking limits. Here we add staking parameters that define the maximum total assets(tBTC) that the Acre contract can hold and the minimum deposit amount for a single deposit operation. Each staking parameter can be updated by the owner.