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 |
There was a problem hiding this comment.
Shares should not be minted?
| 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
* chore: updgrade ape * build: allow max uint (#204) * build: allow max uint * fix: lint version * forge install: openzeppelin-contracts v4.9.5 * chore: oz sub module * forge install: tokenized-strategy v3.0.2 * test: fix foundry tests * chore: bump version * fix: workflow (#207) * chore: update wf version * chore: add statemind report * feat: update name and symbol (#206) * feat: set name and symbol * chore: spech * test: fix emergency * fix: test workflow (#208) * fix: config override * chore: manual setup * fix: requirements * fix: ape version * chore: rebase * chore: dont double pull from storage (#212) * feat: report on self (#205) * feat: report on self * chore: comment * chore: add back * build: auto allocate (#203) * build: auto allocate * build: dont revert debt increase * chore: remove decrease revert * chore: update spech * chore: spech * chore: comments * fix: event * fix: comments Co-authored-by: spalen0 <[email protected]> * feat: pack pf config (#211) * feat: pack pf config * chore: remove event * chore: formatting * test: interface updates * chore: comments * fix: deposit flow (#209) * forge install: openzeppelin-contracts v4.9.5 * chore: oz sub module * test: fix foundry tests * fix: deposit flow * fix: zero total assets * fix: flow * test: full loss * chore: comment * test: add invariants * fix: comments * fix: user msg sender * fix: comments * fix: comment * chore: add to interfaces * fix: comments * chore: match gov abi (#213) * chore: deployment * chore: deployed --------- Co-authored-by: FP <[email protected]> Co-authored-by: spalen0 <[email protected]>
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