fix: buy debt and role addition

yearn / yearn-vaults-v3

GNU Affero General Public License v3.0

96 stars 35 forks source link

fix: buy debt and role addition #177

Closed Schlagonia closed 11 months ago

Schlagonia commented 1 year ago

Description

buy debt based on the current_debt not relying on strategy conversion rates
Add an add_role and remove_role option for setting roles.

Fixes # (issue)

Checklist

[ ] I have run vyper and solidity linting
[ ] I have run the tests on my machine
[ ] I have followed commitlint guidelines
[ ] I have rebased my changes to the latest version of the main branch

fp-crypto commented 11 months ago

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?

Schlagonia commented 11 months ago

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.

fp-crypto commented 11 months ago

@Schlagonia rename PR plz

fp-crypto commented 11 months ago

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?

Schlagonia commented 11 months ago

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 https://github.com/yearn/yearn-vaults-v3/pull/177/commits/f68ed1f2fce796ccc4af981369ddd986aeef828e https://github.com/yearn/yearn-vaults-v3/pull/177/commits/e582f0f6e6ea1afe737dca28336efba91a1d75e5

I will consider adding the deposit limit ones to the withdraw limits module PR since that is where they are relevant.