feat(vaultManager): expose liquidation metrics #5393

turadg · 2022-05-18T23:44:56Z

closes: #5300

Description

Include liquidation metrics in VaultManager's notifications.

Security Considerations

Exposes more data, but only that we want to be public.

Documentation Considerations

The metrics and their descriptions:

/**
 * @typedef {object} MetricsNotification
 *
 * @property {number}         numVaults        present count of vaults
 * @property {Amount<'nat'>}  totalCollateral  present sum of collateral across all vaults
 * @property {Amount<'nat'>}  totalDebt        present sum of debt across all vaults
 *
 * @property {Amount<'nat'>}  totalCollateralSold       running sum of collateral sold in liquidation // totalCollateralSold
 * @property {Amount<'nat'>}  totalOverageReceived      running sum of overages, central received greater than debt
 * @property {Amount<'nat'>}  totalProceedsReceived     running sum of central received from liquidation
 * @property {Amount<'nat'>}  totalShortfallReceived    running sum of shortfalls, central received less than debt
 * @property {number}         numLiquidationsCompleted  running count of liquidations
 */

It wasn't clear how to distinguish metrics for "now" vs "up to now". This This introduces a naming scheme: nouns are present values; past-participles are summative. It's included as a comment in vaultManager.js. Are there other places it should go? (assuming reviewer agrees to the scheme)

Testing Considerations

New tests and new harness for expressing diffs from one subscription state to the next.

I want to have a test validating the arithmetic among debt, proceeds, overage, and sales. I'm waiting on #5366 to be able to query the latest state. If that's done by the time this needs to land I'll include it; if not I'll change the FIXME to a TODO.

…n-metrics

turadg · 2022-05-23T22:31:48Z

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

@@ -85,9 +85,9 @@
 * @property {() => Ratio} getLoanFee
 * @property {() => Promise<PriceQuote>} getCollateralQuote
 * @property {() => Ratio} getInterestRate - The annual interest rate on a loan
- * @property {() => RelativeTime} getChargingPeriod - The period (in seconds) at
+ * @property {() => NatValue} getChargingPeriod - The period (in seconds) at


these are specifically seconds, not arbitrary RelativeTime. they also can't be relative; they must be positive.

RelativeTime should perhaps be renamed to Duration and declared to be Nat. I don't think it can be negative.

For the moment, I think the RelativeTime used by the TimerService is actually what we want here.

sgtm. done.

turadg · 2022-05-23T22:40:43Z

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

-      state.totalCollateral = AmountMath.add(
-        state.totalCollateral,
-        vaultKit.vault.getCollateralAmount(),
-      );


this didn't previously account for changes in collateral after a vault was created. it's handled now in updateVaultAccounting

Chris-Hibbert · 2022-05-23T23:50:59Z

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

@@ -85,9 +85,9 @@
 * @property {() => Ratio} getLoanFee
 * @property {() => Promise<PriceQuote>} getCollateralQuote
 * @property {() => Ratio} getInterestRate - The annual interest rate on a loan
- * @property {() => RelativeTime} getChargingPeriod - The period (in seconds) at
+ * @property {() => NatValue} getChargingPeriod - The period (in seconds) at


RelativeTime should perhaps be renamed to Duration and declared to be Nat. I don't think it can be negative.

For the moment, I think the RelativeTime used by the TimerService is actually what we want here.

Chris-Hibbert

I think my only substantive comment is about whether t.pass() is necessary.

Chris-Hibbert · 2022-05-25T19:52:28Z

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

+ * @param {Amount<'nat'>} debt
+ * @param {Amount<'nat'>} proceeds
+ */
+const calcMetrics = (debt, proceeds) => {


I think we're now consistently using 'metrics' for the data that gets published by subscriptions. As long as it's not exposed outside this helper, it doesn't matter. I think the naming is inconsequential, since the return value is { proceeds: proceeds.RUN, overage, shortfall }.

Chris-Hibbert · 2022-05-25T19:55:32Z

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

@@ -33,6 +33,21 @@ const { details: X } = assert;

 const trace = makeTracer('VM', false);

+// Metrics naming scheme: nouns are present values; past-participles are summative.


Is 'summative' an actual word? How about 'running totals' or 'accumulations'? If you prefer summative, I'll be fine with it. Its meaning is obvious.

It is indeed a word. Primed in my brain summative assessments. Accumulative is much better though because they grow monotonically. Changing to that.

Chris-Hibbert · 2022-05-25T19:58:21Z

packages/run-protocol/test/metrics.js

@@ -19,13 +21,14 @@ export const subscriptionTracker = async (t, subscription) => {
    const prevNotif = notif;
    notif = await metrics.getUpdateSince(notif.updateCount);
    const actualDelta = diff(prevNotif.value, notif.value);
+    console.log({ actualDelta });


shouldn't this have a label?

ideally. this is just a test so I don't feel strongly. I'll remove though and people can add it whip it up if they need to debug a failure.

Chris-Hibbert · 2022-05-25T20:02:34Z

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

@@ -2398,60 +2400,264 @@ test('director notifiers', async t => {

  // Not testing rewardPoolAllocation contents because those are simply those values.
  // We could refactor the tests of those allocations to use the data now exposed by a notifier.
+
+  t.pass();


Isn't it a bug if no assertions get called on t?

Yeah, this was a holdover from when assertChange was asserting without t, in which case Ava needn't to be told that if we get to this line we passed. No longer necessary so I'll remove.

Chris-Hibbert

Looking good!

dtribble

LGTM. My only objection is in the partialAssign usage, which predates this PR. Please eitehr fix that or file a ticket for it.

dtribble · 2022-05-25T20:48:46Z

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

+ * @param {Amount<'nat'>} proceeds
+ */
+const calcMetrics = (debt, proceeds) => {
+  if (AmountMath.isGTE(debt, proceeds)) {


This should be a conditional expression?

you mean return AmountMath.isGTE(debt, proceeds) ? or something else?

dtribble · 2022-05-25T22:44:37Z

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

    debtBrand: fixed.debtBrand,
-    vaultCounter: 0,
+    latestInterestUpdate,


"latest" here seems redundant. All state is "latest"

will follow up

@dtribble this is not about the state of "interest update", it's specifying the last in a stream of "interest update" values. What would be a better name?

this is sent as part of the AssetNotification so renaming would be a breaking change. If you think the new name idea is worth that lemme know.

dtribble · 2022-05-25T22:46:41Z

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

@@ -243,7 +255,7 @@ const helperBehavior = {
      },
      updateTime,
    );
-    Object.assign(state, stateUpdates);
+    partialAssign(state, stateUpdates);


I don't like this pattern. It makes the entire state of the vault vulnerable to the interest computation. Can we replace with explicit assign of the allowed fields?

Good point. I'll let this one merge since it's on the train and follow up in the morning with another.

dtribble · 2022-05-25T22:54:56Z

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

+      oldCollateral,
+    );
+    // debt accounting managed through minting and burning
+    facets.helper.updateMetrics();


will this trigger two updates, one for collateral and one for debt? For an adjustBalances there should just be one.

I think I'd see that in tests.

dtribble · 2022-05-25T22:56:56Z

packages/run-protocol/test/metrics.js

@@ -19,13 +21,14 @@ export const subscriptionTracker = async (t, subscription) => {
    const prevNotif = notif;
    notif = await metrics.getUpdateSince(notif.updateCount);
    const actualDelta = diff(prevNotif.value, notif.value);
+    console.log({ actualDelta });


comment out now?

or should be using trace

He did drop it.

was removed in last push

dtribble · 2022-05-25T22:58:04Z

packages/run-protocol/test/metrics.js

@@ -10,6 +10,8 @@ import { diff } from 'deep-object-diff';
 export const subscriptionTracker = async (t, subscription) => {
  const metrics = makeNotifierFromAsyncIterable(subscription);
  let notif;
+  const getLastNotif = () => notif;


That's a strange name. Isn't it the last state snapshot?

(oh it's in test. ok nvm :)

turadg added 13 commits May 19, 2022 09:50

refactor(vaultManager): updateMetrics in finish()

dd29fde

feat(vaultManager): expose metrics on liquidation

f2b4404

fixup! feat(vaultManager): expose metrics on liquidation

de31950

test: helper for metrics state changes

28c46fc

deepDifference has bugs; import open source

fa61375

fixup test logging

34ae779

chore: more detailed error msg

0e7efea

fixup! test: helper for metrics state changes

e5011d9

fixup! deepDifference has bugs; import open source

54d7fed

hack: amount deltas (allow negative)

d42691e

fixup! deepDifference has bugs; import open source

297d8a5

test: metrics subscription testing support

76e8190

test: metrics subscription testing support

f27aed9

turadg mentioned this pull request May 19, 2022

test: metrics subscription testing support #5402

Merged

turadg added 3 commits May 19, 2022 16:14

Merge branch '5300-metrics-testing-support' into 5300-liquidation-eco…

499264a

…n-metrics

Merge branch 'master' into 5300-liquidation-econ-metrics

bb3b5a3

document metric properties

a6a4045

turadg force-pushed the 5300-liquidation-econ-metrics branch from 6955665 to 23c4d30 Compare May 20, 2022 23:14

turadg added 3 commits May 20, 2022 16:32

totalDebtTracker

e534c68

fixup

62c73b8

feat(vaultManager): liquidation overage and shortfall metrics

07d7b31

turadg force-pushed the 5300-liquidation-econ-metrics branch from 23c4d30 to 07d7b31 Compare May 23, 2022 14:48

turadg changed the title ~~feat(liquidation): expose econ metrics~~ feat(vaultManager): expose liquidation metrics May 23, 2022

turadg added 4 commits May 23, 2022 14:36

fixup

53c17d7

improve metric names

33d9242

fixup superfluous updateMetrics()

4e2a47e

Merge branch 'master' into 5300-liquidation-econ-metrics

ca76d87

turadg commented May 23, 2022

View reviewed changes

turadg added 2 commits May 23, 2022 15:35

cleanup

42ac0d8

updateVaultPriority -> updateVaultAccounting

0405621

turadg commented May 23, 2022

View reviewed changes

turadg requested a review from Chris-Hibbert May 23, 2022 22:41

turadg marked this pull request as ready for review May 23, 2022 22:41

turadg requested a review from dtribble as a code owner May 23, 2022 22:41

Chris-Hibbert requested changes May 24, 2022

View reviewed changes

turadg added 2 commits May 24, 2022 07:14

Merge branch 'master' into 5300-liquidation-econ-metrics

0c4fe50

validate sums from liquidation vs total debt ever

0897a66

turadg force-pushed the 5300-liquidation-econ-metrics branch from 72b4696 to 0897a66 Compare May 24, 2022 16:49

turadg requested a review from Chris-Hibbert May 24, 2022 16:52

turadg added 2 commits May 24, 2022 15:43

Merge branch 'master' into 5300-liquidation-econ-metrics

78f0c22

comments

b97db67

turadg added the automerge:squash Automatically squash merge label May 25, 2022

Chris-Hibbert reviewed May 25, 2022

View reviewed changes

cr

8d7858a

turadg requested a review from Chris-Hibbert May 25, 2022 21:41

Chris-Hibbert approved these changes May 25, 2022

View reviewed changes

Merge branch 'master' into 5300-liquidation-econ-metrics

2ade506

dtribble approved these changes May 25, 2022

View reviewed changes

mergify bot merged commit 47d4823 into master May 25, 2022

mergify bot deleted the 5300-liquidation-econ-metrics branch May 25, 2022 23:39

turadg mentioned this pull request May 27, 2022

vault security improvements #5440

Merged

turadg mentioned this pull request Oct 17, 2022

rationalize Inter Protocol storage node paths #6111

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vaultManager): expose liquidation metrics #5393

feat(vaultManager): expose liquidation metrics #5393

turadg commented May 18, 2022 •

edited

Loading

turadg May 23, 2022

Chris-Hibbert May 23, 2022

turadg May 24, 2022

turadg May 23, 2022

Chris-Hibbert May 23, 2022

Chris-Hibbert left a comment

Chris-Hibbert May 25, 2022

Chris-Hibbert May 25, 2022

turadg May 25, 2022 •

edited

Loading

Chris-Hibbert May 25, 2022

turadg May 25, 2022

Chris-Hibbert May 25, 2022

turadg May 25, 2022

Chris-Hibbert left a comment

dtribble left a comment

dtribble May 25, 2022

turadg May 25, 2022

turadg May 26, 2022

dtribble May 25, 2022

turadg May 25, 2022

turadg May 26, 2022 •

edited

Loading

dtribble May 25, 2022

turadg May 25, 2022

turadg May 26, 2022

dtribble May 25, 2022

turadg May 25, 2022

dtribble May 25, 2022

dtribble May 25, 2022

Chris-Hibbert May 25, 2022

turadg May 25, 2022

dtribble May 25, 2022

dtribble May 25, 2022

		@@ -33,6 +33,21 @@ const { details: X } = assert;

		const trace = makeTracer('VM', false);

		// Metrics naming scheme: nouns are present values; past-participles are summative.

feat(vaultManager): expose liquidation metrics #5393

feat(vaultManager): expose liquidation metrics #5393

Conversation

turadg commented May 18, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert left a comment

Choose a reason for hiding this comment

dtribble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg May 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented May 18, 2022 •

edited

Loading

turadg May 25, 2022 •

edited

Loading

turadg May 26, 2022 •

edited

Loading