Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: buy debt and role addition #177

Merged
merged 8 commits into from
Oct 2, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 55 additions & 24 deletions contracts/VaultV3.vy
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,7 @@ def _deposit(sender: address, recipient: address, assets: uint256) -> uint256:
"""
assert self.shutdown == False # dev: shutdown
assert recipient not in [self, empty(address)], "invalid recipient"
assert self._total_assets() + assets <= self.deposit_limit, "exceed deposit limit"
assert self._total_assets() + assets <= self.deposit_limit, "ERC4626: deposit more than max"

# Transfer the tokens to the vault first.
self._erc20_safe_transfer_from(ASSET.address, msg.sender, self, assets)
Expand All @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? this seems to be less useful error msg

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was to try and make the error messages conform to more normal ones used across other vaults/ Erc 20 contracts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we want it to be different though

Copy link
Collaborator

@fp-crypto fp-crypto Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will say i would prefer we stuck with either cannot or can't 😉


log Deposit(sender, recipient, assets, shares)
return shares
Expand All @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, less useful error message

assert self._total_assets() + assets <= self.deposit_limit, "ERC4626: mint more than max"

# Transfer the tokens to the vault first.
self._erc20_safe_transfer_from(ASSET.address, msg.sender, self, assets)
Expand Down Expand Up @@ -668,7 +668,7 @@ def _redeem(
shares: uint256 = shares_to_burn
shares_balance: uint256 = self.balance_of[owner]

assert shares > 0, "no shares to redeem"
assert shares > 0, "ZERO_SHARES"
assert shares_balance >= shares, "insufficient shares to redeem"

if sender != owner:
Expand Down Expand Up @@ -1273,15 +1273,45 @@ def _enforce_role(account: address, role: Roles):
@external
def set_role(account: address, role: Roles):
"""
@notice Set the role of an account.
@notice Set the roles for of an account.
Schlagonia marked this conversation as resolved.
Show resolved Hide resolved
@dev This will fully override an accounts current roles
so it should include all roles the account should hold.
@param account The account to set the role for.
@param role The role to set.
@param role The roles the account should hold.
"""
assert msg.sender == self.role_manager
self.roles[account] = role

log RoleSet(account, role)

@external
def add_role(account: address, role: Roles):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this and remove_role to be the only way to set roles? seems less error prone

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
@notice Add a new role to an address.
@dev This will add a new role to the account
without effecting any of the previously held roles.
@param account The account to add a role to.
@param role The new role to add to account.
"""
assert msg.sender == self.role_manager
self.roles[account] = self.roles[account] | role

log RoleSet(account, self.roles[account])

@external
def remove_role(account: address, role: Roles):
"""
@notice Remove a single role from an account.
@dev This will leave all other roles for the
account unchanged.
@param account The account to remove a Role from.
@param role The Role to remove.
"""
assert msg.sender == self.role_manager
self.roles[account] = self.roles[account] & ~role

log RoleSet(account, self.roles[account])

@external
def set_open_role(role: Roles):
"""
Expand Down Expand Up @@ -1390,9 +1420,8 @@ def buy_debt(strategy: address, amount: uint256):
@dev This should only ever be used in an emergency in place
of force revoking a strategy in order to not report a loss.
It allows the DEBT_PURCHASER role to buy the strategies debt
for an equal amount of `asset`. It's important to note that
this does rely on the strategies `convertToShares` function to
determine the amount of shares to buy.
for an equal amount of `asset`.

@param strategy The strategy to buy the debt for
@param amount The amount of debt to buy from the vault.
"""
Expand All @@ -1401,35 +1430,37 @@ def buy_debt(strategy: address, amount: uint256):

# Cache the current debt.
current_debt: uint256 = self.strategies[strategy].current_debt

_amount: uint256 = amount

assert current_debt > 0, "nothing to buy"
assert amount > 0, "nothing to buy with"
assert _amount > 0, "nothing to buy with"

if _amount > current_debt:
_amount = current_debt

# Get the current shares value for the amount.
shares: uint256 = IStrategy(strategy).convertToShares(amount)
# We get the proportion of the debt that is being bought and
# transfer the equivalant shares. We assume this is being used
Schlagonia marked this conversation as resolved.
Show resolved Hide resolved
# due to strategy issues so won't rely on its conversion rates.
shares: uint256 = IStrategy(strategy).balanceOf(self) * _amount / current_debt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree a partial buy could be useful


assert shares > 0, "can't buy 0"
assert shares <= IStrategy(strategy).balanceOf(self), "not enough shares"

self._erc20_safe_transfer_from(ASSET.address, msg.sender, self, amount)

# Adjust if needed to not underflow on math
bought: uint256 = min(current_debt, amount)
self._erc20_safe_transfer_from(ASSET.address, msg.sender, self, _amount)

# Lower strategy debt
self.strategies[strategy].current_debt -= bought
self.strategies[strategy].current_debt -= _amount
# lower total debt
self.total_debt -= bought
self.total_debt -= _amount
# Increase total idle
self.total_idle += bought
self.total_idle += _amount

# log debt change
log DebtUpdated(strategy, current_debt, current_debt - bought)
log DebtUpdated(strategy, current_debt, current_debt - _amount)

# Transfer the strategies shares out.
self._erc20_safe_transfer(strategy, msg.sender, shares)

log DebtPurchased(strategy, bought)
log DebtPurchased(strategy, _amount)

## STRATEGY MANAGEMENT ##
@external
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/test_deposit_withdraw_flow.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def test_deposit_and_withdraw(asset, gov, fish, fish_amount, create_vault):
# set deposit limit to half_amount and max deposit to test deposit limit
vault.set_deposit_limit(half_amount, sender=gov)

with ape.reverts("exceed deposit limit"):
with ape.reverts("ERC4626: deposit more than max"):
vault.deposit(amount, fish.address, sender=fish)

vault.deposit(quarter_amount, fish.address, sender=fish)
Expand Down
56 changes: 51 additions & 5 deletions tests/unit/vault/test_buy_debt.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def test_buy_debt__no_amount__reverts(
vault.buy_debt(strategy, 0, sender=gov)


def test_buy_debt__to_many_shares__reverts(
def test_buy_debt__more_than_available__withdraws_current_debt(
gov,
asset,
vault,
Expand All @@ -94,8 +94,30 @@ def test_buy_debt__to_many_shares__reverts(
asset.mint(gov.address, amount, sender=gov)
asset.approve(vault.address, amount, sender=gov)

with ape.reverts("not enough shares"):
vault.buy_debt(strategy, amount * 2, sender=gov)
before_balance = asset.balanceOf(gov)
before_shares = strategy.balanceOf(gov)

tx = vault.buy_debt(strategy, amount * 2, sender=gov)

logs = list(tx.decode_logs(vault.DebtPurchased))[0]

assert logs.strategy == strategy.address
assert logs.amount == amount

logs = list(tx.decode_logs(vault.DebtUpdated))

assert len(logs) == 1
assert logs[0].strategy == strategy.address
assert logs[0].current_debt == amount
assert logs[0].new_debt == 0

assert vault.totalIdle() == amount
assert vault.totalDebt() == 0
assert vault.pricePerShare() == 10 ** asset.decimals()
assert vault.strategies(strategy)["current_debt"] == 0
# assert shares got moved
assert asset.balanceOf(gov) == before_balance - amount
assert strategy.balanceOf(gov) == before_shares + amount


def test_buy_debt__full_debt(
Expand All @@ -120,7 +142,19 @@ def test_buy_debt__full_debt(
before_balance = asset.balanceOf(gov)
before_shares = strategy.balanceOf(gov)

vault.buy_debt(strategy, amount, sender=gov)
tx = vault.buy_debt(strategy, amount, sender=gov)

logs = list(tx.decode_logs(vault.DebtPurchased))[0]

assert logs.strategy == strategy.address
assert logs.amount == amount

logs = list(tx.decode_logs(vault.DebtUpdated))

assert len(logs) == 1
assert logs[0].strategy == strategy.address
assert logs[0].current_debt == amount
assert logs[0].new_debt == 0

assert vault.totalIdle() == amount
assert vault.totalDebt() == 0
Expand Down Expand Up @@ -155,7 +189,19 @@ def test_buy_debt__half_debt(
before_balance = asset.balanceOf(gov)
before_shares = strategy.balanceOf(gov)

vault.buy_debt(strategy, to_buy, sender=gov)
tx = vault.buy_debt(strategy, to_buy, sender=gov)

logs = list(tx.decode_logs(vault.DebtPurchased))[0]

assert logs.strategy == strategy.address
assert logs.amount == to_buy

logs = list(tx.decode_logs(vault.DebtUpdated))

assert len(logs) == 1
assert logs[0].strategy == strategy.address
assert logs[0].current_debt == amount
assert logs[0].new_debt == amount - to_buy

assert vault.totalIdle() == to_buy
assert vault.totalDebt() == amount - to_buy
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/vault/test_profit_unlocking.py
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ def test_gain_no_fees_with_refunds_no_buffer(
assert asset.balanceOf(fish) == fish_amount + first_profit + total_refunds

# Accountant redeems shares
with reverts("no shares to redeem"):
with reverts("ZERO_SHARES"):
vault.redeem(
vault.balanceOf(accountant), accountant, accountant, sender=accountant
)
Expand Down Expand Up @@ -421,7 +421,7 @@ def test_gain_no_fees_with_refunds_with_buffer(
)

# Accountant redeems shares
with reverts("no shares to redeem"):
with reverts("ZERO_SHARES"):
vault.redeem(
vault.balanceOf(accountant), accountant, accountant, sender=accountant
)
Expand Down Expand Up @@ -1629,7 +1629,7 @@ def test_loss_no_fees_with_refunds_with_buffer(
)

# Accountant redeems shares
with reverts("no shares to redeem"):
with reverts("ZERO_SHARES"):
vault.redeem(
vault.balanceOf(accountant), accountant, accountant, sender=accountant
)
Expand Down
Loading