Merge pull request #5689 from Agoric/ta/fix-pricecheck-liquidating

fix: apparent error charging interest during liquidation

packages/run-protocol/src/vaultFactory/prioritizedVaults.js

@@ -43,22 +43,26 @@ export const currentDebtToCollateral = vault =>
   );
 
 /**
- * Vaults, ordered by their liquidation ratio so that all the
- * vaults below a threshold can be quickly found and liquidated.
+ * Vaults, ordered by their debt ratio so that all the vaults below a threshold
+ * can be quickly found and liquidated.
  *
- * @param {() => void} highestChanged called when there is a new
- * `highestRatio` (least-collateralized vault)
+ * @param {(highestRatio: Ratio) => void} higherHighestCb called when a new
+ * vault has a higher debt ratio than the previous highest
  */
-export const makePrioritizedVaults = (highestChanged = () => {}) => {
+export const makePrioritizedVaults = (higherHighestCb = () => {}) => {
   const vaults = makeOrderedVaultStore();
 
   /**
-   * Set the callback for when there is a new least-collateralized vault
+   * Called back when there's a new highestRatio and it's higher than the previous.
    *
-   * @param {() => void} callback
+   * In other words, when a new highestRatio results from adding a vault. Removing the
+   * first vault also changes the highest, but necessarily to a lower highest,
+   * for which there are no use cases requiring notification.
+   *
+   * @param {(highestRatio: Ratio) => void} callback
    */
-  const onHighestRatioChanged = callback => {
-    highestChanged = callback;
+  const onHigherHighest = callback => {
+    higherHighestCb = callback;
   };
 
   // To deal with fluctuating prices and varying collateralization, we schedule a
@@ -76,6 +80,7 @@ export const makePrioritizedVaults = (highestChanged = () => {}) => {
   /** @param {Ratio} collateralToDebt */
 
   /**
+   * Ratio of the least-collateralized vault, if there is one.
    *
    * @returns {Ratio=} actual debt over collateral
    */
@@ -109,9 +114,6 @@ export const makePrioritizedVaults = (highestChanged = () => {}) => {
    */
   const removeVault = key => {
     const vault = vaults.removeByKey(key);
-    // don't call reschedulePriceCheck, but do reset the highest.
-    // This could be expensive if we delete individual entries in
-    // order. Will know once we have perf data.
     trace('removeVault', key, fromVaultKey(key), 'when first:', firstKey);
     if (keyEQ(key, firstKey)) {
       const [secondKey] = vaults.keys();
@@ -145,7 +147,7 @@ export const makePrioritizedVaults = (highestChanged = () => {}) => {
     trace('addVault', key, 'when first:', firstKey);
     if (!firstKey || keyLT(key, firstKey)) {
       firstKey = key;
-      highestChanged();
+      higherHighestCb(currentDebtToCollateral(vault));
     }
     return key;
   };
@@ -186,6 +188,6 @@ export const makePrioritizedVaults = (highestChanged = () => {}) => {
     highestRatio,
     removeVault,
     removeVaultByAttributes,
-    onHighestRatioChanged,
+    onHigherHighest,
   });
 };

packages/run-protocol/src/vaultFactory/vaultManager.js

@@ -1,3 +1,4 @@
+/* eslint-disable consistent-return */
 // @ts-check
 /**
  * @file Vault Manager object manages vault-based debts for a collateral type.
@@ -127,7 +128,7 @@ const trace = makeTracer('VM');
  */
 
 // FIXME https://github.com/Agoric/agoric-sdk/issues/5622
-let liquidationInProgress = false;
+let liquidationQueueing = false;
 let outstandingQuote = null;
 
 /**
@@ -280,6 +281,7 @@ const helperBehavior = {
 
     facets.helper.assetNotify();
     trace('chargeAllVaults complete');
+    // price to check against has changed
     return facets.helper.reschedulePriceCheck();
   },
 
@@ -329,28 +331,21 @@ const helperBehavior = {
    * level.
    *
    * @param {MethodContext} context
+   * @param {Ratio} [highestRatio]
    * @returns {Promise<void>}
    */
-  reschedulePriceCheck: async ({ state, facets }) => {
-    const { prioritizedVaults } = state;
-    const highestDebtRatio = prioritizedVaults.highestRatio();
-    trace('reschedulePriceCheck', { highestDebtRatio });
-    if (!highestDebtRatio) {
-      // if there aren't any open vaults, we don't need an outstanding RFQ.
-      trace('no open vaults');
-      return;
-    }
-
+  reschedulePriceCheck: async ({ state, facets }, highestRatio) => {
+    trace('reschedulePriceCheck', { liquidationQueueing });
     // INTERLOCK: the first time through, start the activity to wait for
     // and process liquidations over time.
-    if (!liquidationInProgress) {
-      liquidationInProgress = true;
+    if (!liquidationQueueing) {
+      liquidationQueueing = true;
       // eslint-disable-next-line consistent-return
       return facets.helper
         .processLiquidations()
         .catch(e => console.error('Liquidator failed', e))
         .finally(() => {
-          liquidationInProgress = false;
+          liquidationQueueing = false;
         });
     }
 
@@ -359,6 +354,14 @@ const helperBehavior = {
       return;
     }
 
+    const { prioritizedVaults } = state;
+    const highestDebtRatio = highestRatio || prioritizedVaults.highestRatio();
+    if (!highestDebtRatio) {
+      // if there aren't any open vaults, we don't need an outstanding RFQ.
+      trace('no open vaults');
+      return;
+    }
+
     // There is already an activity processing liquidations. It may be
     // waiting for the oracle price to cross a threshold.
     // Update the current in-progress quote.
@@ -774,7 +777,7 @@ const selfBehavior = {
 
 /** @param {MethodContext} context */
 const finish = ({ state, facets: { helper } }) => {
-  state.prioritizedVaults.onHighestRatioChanged(helper.reschedulePriceCheck);
+  state.prioritizedVaults.onHigherHighest(helper.reschedulePriceCheck);
 
   // push initial state of metrics
   helper.updateMetrics();

packages/run-protocol/test/vaultFactory/test-prioritizedVaults.js

@@ -41,7 +41,8 @@ function makeCollector() {
 function makeRescheduler() {
   let called = false;
 
-  async function fakeReschedule() {
+  async function fakeReschedule(ratio) {
+    assert(ratio);
     called = true;
     return called;
   }
@@ -216,7 +217,8 @@ test('removals', async t => {
 
 test('highestRatio', async t => {
   const reschedulePriceCheck = makeRescheduler();
-  const vaults = makePrioritizedVaults(reschedulePriceCheck.fakeReschedule);
+  const vaults = makePrioritizedVaults();
+  vaults.onHigherHighest(reschedulePriceCheck.fakeReschedule);
 
   vaults.addVault(
     'id-fakeVault1',