feat(run-protocol): liquidate serially #4931
Conversation
| quoteRatioPlusMargin, | ||
| )) { | ||
| // eslint-disable-next-line no-await-in-loop -- We want each liquidation to happen in a separate turn | ||
| await liquidateAndRemove([key, vault]); |
There was a problem hiding this comment.
This is actually exactly the scenario that the await rule is designed to prevent, so I don't think this is an allowable exception. If entriesPrioritizedGTE returns an empty iteration, then line 229 will execute in the same turn, whereas if it does not, it would execute in a different turn.
turadg
force-pushed
the
4750-simplify-liquidation-queuing
branch
from
2f64e5e to
3f15daf
Compare
| // expect Alice to be liquidated because her collateral is too low. | ||
| aliceUpdate = await E(aliceNotifier).getUpdateSince(); |
There was a problem hiding this comment.
@Chris-Hibbert can you confirm these test changes are legit?
There was a problem hiding this comment.
This update isn't used. It seems fine to drop it.
| ) | ||
| .then(() => { | ||
| assert(prioritizedVaults); | ||
| prioritizedVaults?.removeVault(key); |
There was a problem hiding this comment.
Why does this need optional chaining when it has just asserted that prioritizedVaults is non-null?
There was a problem hiding this comment.
probably a result of refactoring. corrected.
| // expect Alice to be liquidated because her collateral is too low. | ||
| aliceUpdate = await E(aliceNotifier).getUpdateSince(); |
There was a problem hiding this comment.
This update isn't used. It seems fine to drop it.
| t.is(bobUpdate.value.vaultState, Phase.LIQUIDATED); | ||
|
|
||
| // No change for Alice | ||
| aliceUpdate = await E(aliceNotifier).getUpdateSince(); // can't use updateCount because there's no newer update |
There was a problem hiding this comment.
updateCount is optional. You don't need to justify not including it. Not including it means "I want the current state, even if it hasn't changed."
There was a problem hiding this comment.
I had to include it in the other places or the update would come back before the liquidation processor settled. There are more promise resolutions now with "liquidate one at a time".
I actually think we should make a habit of including the updateCount so that you have a guarantee that you're getting a new state. If you don't expect a new state then don't ask for it and if you do then the test should fail if it's not available.
I even think it might be worth a helper wrapper around this notifier/response pair so you could have,
const aliceAfterLiquidation = await alicesUpdates.next();
(it would do the updateCount stuff itself to ensure you're getting a new update)
| t.deepEqual(aliceUpdate.value.debtSnapshot.debt, aliceRunDebtLevel); | ||
|
|
||
| await manualTimer.tick(); | ||
| // price levels changed and interest was charged. | ||
|
|
||
| // expect Alice to be liquidated because her collateral is too low. | ||
| aliceUpdate = await E(aliceNotifier).getUpdateSince(); | ||
| aliceUpdate = await E(aliceNotifier).getUpdateSince(aliceUpdate.updateCount); |
There was a problem hiding this comment.
I think adding updateCount to these calls is acceptable, though I'm not sure it's an improvement.
Using getUpdateSince without an argument can be more robust in a test because the test won't wait in vain when something changes. It'll return the existing state, which should be tested.
There was a problem hiding this comment.
I spoke to this above but for review hygiene: it's better to be more explicit about whether a new state is expected, especially due to the interleaving of async calls that are common in our code.
closes: #4750
refs: #XXXX
Description
Restores
liquidateAndRemovepromise from #4715 but changes the high-water-mark case to liquidate serially instead of starting them all at once in parallel.For the
liquidateAllcase it continues start them all at once in parallel, assuming that that would maximize the throughput and efficiency is not a concern for that case.Security Considerations
Reduces dependence on durable state.
Documentation Considerations
--
Testing Considerations
Existing unit tests.