vault manager refactorings #5574
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
turadg
commented
Jun 10, 2022
| @@ -80,16 +76,13 @@ const liquidate = async ( | |||
| const runToBurn = AmountMath.min(proceeds.RUN, debt); | |||
| // debt is fully settled, with runToBurn and shortfall | |||
| assert(AmountMath.isEqual(debt, AmountMath.add(runToBurn, shortfall))); | |||
| trace('before burn', { debt, proceeds, overage, shortfall, runToBurn }); | |||
| // TODO why grant this power; we can burn it from the caller | |||
| burnLosses(runToBurn, vaultZcfSeat); | |||
There was a problem hiding this comment.
Now vault is updated with liquidated before the losses are burned, which updates the consumer of the vault. If burnLosses fails now the vault will still be "liquidated" to the user.
As I think this through I realize this could allow a state in which a vault's phase is liquidated and thus settled, but it's still in the vaultManager. Though that possibility true of master, albeit with one fewer case (failure of burnLosses/burnDebt) that could manifest it. We don't have checked exceptions but maybe there's something to document about what failures must be handled? cc @dtribble
Chris-Hibbert
approved these changes
Jun 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs: #4415
Description
Code improvements that came up while working on bug fixes. Left out of those PRs since they're out of scope of fixing the bug and may require additional review.
Review by commit recommended.
Security Considerations
Take
burnLossespower fromvault. GivesvaultManagerthevaultobject instead, but it already had the power to iterate over all unsettled vaults.Documentation Considerations
Testing Considerations