fix: deposit flow #209
fix: deposit flow #209
Conversation
Schlagonia
force-pushed
the
deposit_flow
branch
2 times, most recently
from
be419b9
to
36757ef
Compare
Schlagonia
force-pushed
the
deposit_flow
branch
from
1e0895f
to
411cae3
Compare
Schlagonia
force-pushed
the
deposit_flow
branch
from
d796b65
to
60524bd
Compare
|
|
||
| assert assets > 0, "cannot deposit zero" | ||
| assert shares > 0, "cannot mint zero" | ||
|
|
||
| # Transfer the tokens to the vault first. | ||
| self._erc20_safe_transfer_from(self.asset, msg.sender, self, assets) |
There was a problem hiding this comment.
Why not use the input parameter sender instead of msg.sender?
Below in the event Deposit, parameter sender is used.
I would either remove the sender parameter and go with msg.sender in both places or use the sender address for both, to limit confusion.
There was a problem hiding this comment.
good point
I believe it is actually cheaper to use msg.sender than using a memory variable. And clearer so will remove the variable
tests/unit/vault/test_shares.py
Outdated
| with ape.reverts("cannot mint zero"): | ||
| vault.deposit(amount, fish, sender=fish) | ||
|
|
||
| # shares should not be |
| assert vault.pricePerShare() == 0 | ||
| assert vault.convertToShares(amount) == 0 | ||
| assert vault.convertToAssets(amount) == 0 | ||
| # assert vault.maxDeposit(fish) == 0 |
There was a problem hiding this comment.
maxDeposit() won't return 0 but uint256.max?
yearn-vaults-v3/contracts/VaultV3.vy
Line 529 in 60524bd
There was a problem hiding this comment.
yes, maxDeposit and maxMint wont be accurate currently in this scenario
Not sure any good way to fix it though without adding way to much complexity and extra gas.
Seems like such a minor edge case that it is not worth it.
There was a problem hiding this comment.
Realistically if this happens no-one should be trying to deposit and a manager should shutdown the vault anyway which will reduce the deposit limit.
tests/unit/vault/test_shares.py
Outdated
| # shares should not be | ||
| assert vault.balanceOf(fish) == amount | ||
| assert vault.pricePerShare() == 0 | ||
| assert vault.convertToShares(amount) == 0 | ||
| assert vault.convertToAssets(amount) == 0 | ||
| # assert vault.maxMint(fish) == 0 |
There was a problem hiding this comment.
The same questions as above, for deposit flow.
There was a problem hiding this comment.
maxMint will be accurate if there is a non-max uint deposit limit, since it will then do the full conversion
Description
issue_shares_for_amountin order to use consistent logic for both routes.This can happen due to the profit unlocking if all shares are redeemed while some profit is still unlocking, at the end of the unlock time totalAssets >0 but totalSupply == 0.
Fixes # (issue)
Checklist