refactor(run-protocol): store liquidation cohort before liquidating #4715
Conversation
| @@ -107,7 +108,10 @@ export const makeVaultManager = ( | |||
| // definition of reschedulePriceCheck, which refers to sortedVaultKits | |||
| // XXX misleading mutability and confusing flow control; could be refactored with a listener | |||
| /** @type {ReturnType<typeof makePrioritizedVaults>=} */ | |||
| let prioritizedVaults; | |||
| let illiquidVaults; | |||
There was a problem hiding this comment.
I think these are the same prioritizedVaults as before. 'illiquid' sounds to me like they have insufficient capitalization and should be liquidated. Would you mind renaming it back to prioritizedVaults, or something representing them as loans in good standing?
There was a problem hiding this comment.
Yeah, it's a poor name choice. I was looking for something that described how they're prioritized because vaultsToLiquidate are also ordered. But I'll leave that for #4415
| @@ -129,30 +133,43 @@ export const makeVaultManager = ( | |||
| ); | |||
|
|
|||
| /** | |||
| * | |||
| * @param {[key: string, vaultKit: InnerVault]} record | |||
| * @param {Iterable<[key: string, vaultKit: InnerVault]>} recordEntries | |||
There was a problem hiding this comment.
recordEntries is confusing to me. I alternate between reading record as a noun and a verb. Can they be vaultEntries or something like that?
|
|
||
| vaultsToLiquidate.delete(key); | ||
| } catch (e) { | ||
| // XXX should notify interested parties |
There was a problem hiding this comment.
If the vault is now in the liquidating state, should we try again to remove it from vaultsToLiquidate?
There was a problem hiding this comment.
Good question. My thought was it still needs to be liquidated. This way it'll keep retrying. I don't know of failure modes in which you wouldn't want to retry. I figure we'll come back to that when "notify interested parties" is addressed.
closes: #4571
Description
Populates a
vaultsToLiquidatequeue before beginning liquidation.Separated in commits for easier review. I'll squash the whole PR for merge.
Security Considerations
Doesn't change API.
Documentation Considerations
Internal implementation.
Testing Considerations
No change in behavior. Eventually we'll want these stores to persist durably which will need testing, but that's down the road.