fix: buy debt and role addition #177
Conversation
contracts/VaultV3.vy
Outdated
| @@ -580,7 +580,7 @@ def _deposit(sender: address, recipient: address, assets: uint256) -> uint256: | |||
| # Issue the corresponding shares for assets. | |||
| shares: uint256 = self._issue_shares_for_amount(assets, recipient) | |||
|
|
|||
| assert shares > 0, "cannot mint zero" | |||
| assert shares > 0, "ZERO_SHARES" | |||
There was a problem hiding this comment.
why? this seems to be less useful error msg
There was a problem hiding this comment.
It was to try and make the error messages conform to more normal ones used across other vaults/ Erc 20 contracts.
There was a problem hiding this comment.
perhaps we want it to be different though
There was a problem hiding this comment.
i will say i would prefer we stuck with either cannot or can't 😉
contracts/VaultV3.vy
Outdated
| @@ -597,8 +597,8 @@ def _mint(sender: address, recipient: address, shares: uint256) -> uint256: | |||
|
|
|||
| assets: uint256 = self._convert_to_assets(shares, Rounding.ROUND_UP) | |||
|
|
|||
| assert assets > 0, "cannot mint zero" | |||
| assert self._total_assets() + assets <= self.deposit_limit, "exceed deposit limit" | |||
| assert assets > 0, "ZERO_ASSETS" | |||
There was a problem hiding this comment.
same, less useful error message
| """ | ||
| assert msg.sender == self.role_manager | ||
| self.roles[account] = role | ||
|
|
||
| log RoleSet(account, role) | ||
|
|
||
| @external | ||
| def add_role(account: address, role: Roles): |
There was a problem hiding this comment.
should this and remove_role to be the only way to set roles? seems less error prone
There was a problem hiding this comment.
I think given the number of Roles the vault has, we want to keep the set_role function.
For something like yChad you want to give all the roles to. I dont want to have to loop through each one calling add_role
| # We get the proportion of the debt that is being bought and | ||
| # transfer the equivalant shares. We assume this is being used | ||
| # due to strategy issues so won't rely on its conversion rates. | ||
| shares: uint256 = IStrategy(strategy).balanceOf(self) * _amount / current_debt |
There was a problem hiding this comment.
why would you allow partial debt buy? if the only situation is when there's an issue with the strat, it should be 100% sold, no?
There was a problem hiding this comment.
If governance either doesnt have enough funds, or desire to buy the whole position.
For example debt is 10m but governance is only gonna but 2m cause it doesnt have more.
There was a problem hiding this comment.
i agree a partial buy could be useful
|
Given the immunefi report received, i've been wondering if there is a reason to overpay for debt. For example, a strategy has a loss that is reported to the vault, governance decides it wants to buy the debt and cover the loss. Would it make sense to over pay for the debt or to donate to the strategy and run through a reporting cycle first? |
I think the better way to do it this is either through the accountant with refunds or buy the debt then add a strategy that has funds donated to it and report the donation profit that way. If debt is being purchased i would imagine we dont want to do any more reporting for that strategy. |
|
@Schlagonia rename PR plz |
|
Generally LGTM, although i think combining the buy debt changes and error messages makes this somewhat harder to merge cleanly. Not to create extra busywork, but maybe break out the revert message changes so we can discuss elsewhere? |
Okay changed the error messages back to the original ones I will consider adding the deposit limit ones to the withdraw limits module PR since that is where they are relevant. |
Description
Fixes # (issue)
Checklist