feat(run-protocol): interest charging O(1) for all vaults in a manager #4527

turadg · 2022-02-10T23:21:28Z

closes: #4341
refs: #4345

Description

Vault need to be able to persist to disk (#4511). On the path to that is making them store as virtual objects in the new Collections API. While doing this refactor though we design for the durable case and take as a requirement that when interest is charged each day that it does not require disk IO for each vault (O(n)).

To make charging interest O(1) we virtualize the debt that a vault owes to be a function of stable vault attributes and values that change in the vault manager when it charges interest. Specifically,

a compoundedInterest value on the manager that keeps track of interest accrual since its launch
a debtSnapshot on the vault by which one can calculate the actual debt

To maintain that the keys of vaults to liquidate are stable requires that its keys are also time-independent so they're recorded as a "normalized collateralization ratio", with the actual collateral divided by the normalized debt.

Before merging I plan to,

fix or remove each skipped test
clean up the tracing (either remove or figure out a way to configure what gets output, e.g. like https://www.npmjs.com/package/debug )
turn each new TODO line into a ticket or do it
remove each ??? (hopefully by replacing with an answer)
convert the forEach method with callback to a generator
assess maximum key length limits with Collections API
~~eliminate Eslint-disable lines by updating eslint config (style: loosen some eslint rules #4530)~~

Security Considerations

Now the actual debts of all vaults are manipulated by a single value in the vault manager, making it higher stakes. Of course if the vault manager is compromised there are bigger problems and, on the other hand, this moves that out of the vaults which are more distributed.

An implication of this change is that we'll have to #4540 for which there may be some misleading UI could deceive users. Let's consider that when we come to that design.

Documentation Considerations

Reviewers, how much of the design should I document in the README.md?

Anything to do in https://github.com/Agoric/documentation ?

Testing Considerations

There are new tests for the functionality. I think it maintained the APi that the UI was depending on. @samsiegart anything extra to do?

Chris-Hibbert

I'd like to see tests showing the interest rate and debt calculation when some of the vaults were opened significantly after the vaultManager.

I'm worried that the fact that the debt has to be represented as a back-dated value in order to agree with the compoundedInterest may impose a higher floor on the minimum debt. Currently you can take out a loan as long as the interest each period would be non-negligible. This may make that minimum larger in order to be able to back-date the debt.

packages/run-protocol/README.md

Chris-Hibbert · 2022-02-11T00:35:21Z

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

 *
 * @typedef {BaseVault & VaultMixin} Vault
 * @typedef {Object} VaultMixin
 * @property {() => Promise<Invitation>} makeAdjustBalancesInvitation
 * @property {() => Promise<Invitation>} makeCloseInvitation
 * @property {() => ERef<UserSeat>} getLiquidationSeat
- * @property {() => Promise<string>} getLiquidationPromise


why drop this? The Vault's owner has no other way to get the promise, which they can use to be notified when the vault is liquidated.

I should have explained: #4539

Chris-Hibbert · 2022-02-11T01:07:13Z

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

-  let runDebt = AmountMath.makeEmpty(runBrand);
+
+  // ??? is there a good way to encapsulate these snapshot values so they can only be touched together?
+  // perhaps a hardened DebtSnapshot {debt: Amount, interest: Ratio}


perhaps a hardened DebtSnapshot {debt: Amount, interest: Ratio}

That's what I'd do, together with a single method for updating the bundle, which you already have.

Chris-Hibbert · 2022-02-11T01:15:14Z

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

@@ -1,4 +1,6 @@
 // @ts-check
+// XXX we do this a lot, okay to drop it to warning in eslint config?


I'm in favor. I've treated this restriction as a matter of JavaScript style and followed the rule most of the time. I think I've only disabled the check when circular dependencies made it too painful to comply with.

I'll continue as long as it requires the explicit eslint-disable in each file. If we drop the rule, I'll happily go along.

Will come after a release of endojs/endo#1071

Chris-Hibbert · 2022-02-11T17:27:47Z

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

@@ -78,22 +92,61 @@ export const makeVaultManager = (
    async getCollateralQuote() {
      // get a quote for one unit of the collateral
      const displayInfo = await E(collateralBrand).getDisplayInfo();
-      const decimalPlaces = (displayInfo && displayInfo.decimalPlaces) || 0n;
+      const decimalPlaces = displayInfo?.decimalPlaces || 0n;


TIL about optional chaining. Why is this necessary here?

Not necessary just more clear intent.

Is optional chaining in standard JS yet? Is it in test262? (that would let me check whether it's supported by XS.)

A quick check suggests that it is not supported by XS:

packages/xsnap$ ./src/xsrepl xs> {bob: {size: 2}}?.fred (Error#1) Error#1: Uncaught exception in <unnamed xsnap worker>: SyntaxError: invalid token ?.

false alarm: 76/76 optional-chaining says XS conformance

$ ./src/xsrepl xs> ({a: 1}?.b) === undefined true

turadg · 2022-02-15T00:49:34Z

I'd like to see tests showing the interest rate and debt calculation when some of the vaults were opened significantly after the vaultManager.

I added some in b85cc96 and 162fba6.

I'm worried that the fact that the debt has to be represented as a back-dated value in order to agree with the compoundedInterest may impose a higher floor on the minimum debt. Currently you can take out a loan as long as the interest each period would be non-negligible. This may make that minimum larger in order to be able to back-date the debt.

We discussed offline how the check is currently only against loan fees. We also uncovered that the "ceil" implies at least one of the RUN brand amount will be charged at each interest period. However it's denominated in µRUN, not an usurious full RUN. I've documented that in the README.

Chris-Hibbert · 2022-02-15T18:38:17Z

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

+import { makeOrderedVaultStore } from '../../src/vaultFactory/orderedVaultStore.js';
+import { fromVaultKey } from '../../src/vaultFactory/storeUtils.js';
+
+// XXX shouldn't we have a shared test utils for this kind of thing?


issuers are light weight enough that we usually just create a new issuer whenever we need a brand in a test. What's the advantage of a mock brand over that?

Discovery, I suppose. I know now can simply do const mockBrand = Far('brand') because it's any but I first looked for fakeBrand.

If they don't serve any useful purpose, please replace them with either actual issuers or Far('brand'). If the test doesn't rely on these methods, let's not clutter the test with them. Being consistent with other tests when there's no need to diverge makes reviewing them easier.

Chris-Hibbert · 2022-02-15T18:45:40Z

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

+   * @param {string} key
+   * @returns {VaultKit}
+   */
+  const removeByKey = key => {


is orderedVaultStore going to support removing a range of keys as a bulk operation?

I think the underlying Collections API supports that but there's no need to use it yet since vaults are removed whenever their liquidation completes, not a single bulk operation.

Chris-Hibbert · 2022-02-15T18:49:52Z

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

+  });
+};
+
+/**


orderedVaultStore has methods that aren't tested here: removeByKey, keys, getSize, values. I'd like to see a test of at least removeByKey.

I expected a bulk removal operation. If that's coming, I'd like a test of that as well.

OIC. Those are now in test-prioritizedVaults.js. That's fine.

Chris-Hibbert · 2022-02-15T19:14:44Z

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

+  const collateral50 = AmountMath.make(collaterlBrand, 50n);
  const proposal = harden({
    give: { Collateral: collateral50 },
-    want: { RUN: AmountMath.make(runBrand, 70000n) },
+    want: { RUN: AmountMath.make(runBrand, 70n) },


I'm confused by this change. Is it now charging as much interest on 50n as it used to on 50_000n?

Sorry about that. I blew away that file and made it anew, so ignore the diff on it.

Chris-Hibbert · 2022-02-15T23:06:50Z

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

+    }
+
+    // compoundedInterest *= debtStatus.newDebt / totalDebt;
+    compoundedInterest = multiplyRatios(


We never simplify ratios. This ratio is going to contain bigIntegers that grow by a few digits (!) every compounding period. I think we need to choose a resolution and reduce regularly.

Great catch! 🙏

7c3bff8 to address

Chris-Hibbert · 2022-02-15T23:45:20Z

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

+      assert(
+        !(absDelta > totalDebt.value),
+        'Negative delta greater than total debt',
+      );
+      totalDebt = AmountMath.subtract(
+        totalDebt,
+        AmountMath.make(totalDebt.brand, absDelta),


switching back and forth between Amounts and bigInts seems wrong. Can everything here be done with Amounts? If we're already branching based on whether the result of a subtraction is positive or negative, we could branch on which operand is larger, and have nats on both branches, right?

switching back and forth between Amounts and bigInts seems wrong.

Good call. For anyone following along, we chatted and agreed to use bigint more consistently. I think I mostly had them around to be able to make Ratios, but I should be able to simplify those too.

The utility of Amount is as an abstraction when brand or amount type could be different. In the case of interest calculation the brand is always RUN and the amount type is always bigint.

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

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

Chris-Hibbert · 2022-02-16T01:24:29Z

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

+   */
+  // TODO rename to calculateActualDebtAmount throughout codebase https://github.com/Agoric/agoric-sdk/issues/4540
+  const getDebtAmount = () => {
+    // divide compounded interest by the the snapshot


Suggested change

// divide compounded interest by the the snapshot

// divide compounded interest by the snapshot

michaelfig · 2022-02-16T00:36:20Z

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

+   * Redundant tags until https://github.com/Microsoft/TypeScript/issues/23857
+   *
+   * @param {Ratio} ratio
+   * @yields {[string, VaultKit]>}


Suggested change

* @yields {[string, VaultKit]>}

* @yields {[string, VaultKit]}

Chris-Hibbert

The approach looks sound. There are a few things to clean up.

Isn't your expectation that all the '???' should be removed before merging?

Chris-Hibbert · 2022-02-16T22:51:56Z

packages/run-protocol/README.md

+Anyone can open make a **Vault** by putting up collateral the appropriate VaultManager. Then they can request RUN that is backed by that collateral.
+
+When any vat the ratio of the debt to the collateral exceeds a governed threshold, it is deemed undercollateralized. If the result of a price check shows that a vault is undercollateralized. the VaultManager liquidates it.


Suggested change

Anyone can open make a **Vault** by putting up collateral the appropriate VaultManager. Then they can request RUN that is backed by that collateral.

When any vat the ratio of the debt to the collateral exceeds a governed threshold, it is deemed undercollateralized. If the result of a price check shows that a vault is undercollateralized. the VaultManager liquidates it.

Anyone can make a **Vault** by putting up collateral with the appropriate VaultManager. Then

they can request RUN that is backed by that collateral.

In any vat, when the ratio of the debt to the collateral exceeds a governed threshold, it is

deemed undercollateralized. If the result of a price check shows that a vault is

undercollateralized, the VaultManager liquidates it.

Chris-Hibbert · 2022-02-16T23:04:47Z

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

+  while (
+    numerator > COMPOUNDED_INTEREST_PRECISION &&
+    denominator > COMPOUNDED_INTEREST_PRECISION
+  ) {
+    numerator /= COMPOUNDED_INTEREST_PRECISION;
+    denominator /= COMPOUNDED_INTEREST_PRECISION;
+  }


This seems like it'll do the wrong thing. If as a result of multiplication, the new ratio is 25.929489523*COMPOUNDED_INTEREST_PRECISION / 20.239482873*COMPOUNDED_INTEREST_PRECISION, this will reduce it to 25/20. I think Dean's suggestion to reduce the numerator to numerator/denominator * COMPOUNDED_INTEREST_PRECISION, and the denominator to COMPOUNDED_INTEREST_PRECISION would be a better approach.

and 10**30 is more than we need/want. How about 10**21?

This seems like it'll do the wrong thing

Yep, this was a silly thing whipped up for the design discussion.

and 1030 is more than we need/want. How about 1021?
👍 5d43e74

I think we can cut it down more after #4573

Chris-Hibbert · 2022-02-16T23:12:11Z

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

+    const debt = vault.getDebtAmount();
+    const collateral = vault.getCollateralAmount();
+    const key = toVaultKey(debt, collateral, vaultId);
+    console.log('addVaultKit', {


trace() would probably be better.

Agreed. I ended up removing it.

Chris-Hibbert · 2022-02-16T23:24:18Z

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

+import { makeOrderedVaultStore } from '../../src/vaultFactory/orderedVaultStore.js';
+import { fromVaultKey } from '../../src/vaultFactory/storeUtils.js';
+
+// XXX shouldn't we have a shared test utils for this kind of thing?


If they don't serve any useful purpose, please replace them with either actual issuers or Far('brand'). If the test doesn't rely on these methods, let's not clutter the test with them. Being consistent with other tests when there's no need to diverge makes reviewing them easier.

Chris-Hibbert · 2022-02-17T01:23:39Z

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

+ * Each VaultManager manages a single collateralType.
+ *
+ * It owns an autoswap instance which trades this collateralType against RUN. It
+ * also manages some number of outstanding loans, each called a Vault, for which
+ * the collateral is provided in exchange for borrowed RUN.
+ *


Suggested change

* Each VaultManager manages a single collateralType.

*

* It owns an autoswap instance which trades this collateralType against RUN. It

* also manages some number of outstanding loans, each called a Vault, for which

* the collateral is provided in exchange for borrowed RUN.

*

* Each VaultManager manages a single collateral type.

*

* It manages some number of outstanding loans, each called a Vault, for which

* the collateral is provided in exchange for borrowed RUN.

*

Chris-Hibbert · 2022-02-17T01:24:43Z

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

+  const { updater, notifier } = makeNotifierKit(
+    harden({
+      compoundedInterest: makeRatio(1n, runBrand, 1n, runBrand),
+      latestInterestUpdate: 0n,


Is the identity for interest 1n instead of 0n?

latestInterestUpdate is a timestamp, 0n here because there is no previous update. I'll add a comment.

shouldn't it start out as the current time? Won't it charge interest from the unix epoch to the current time the first time it reaches calculateReportingPeriod.
The tests all run with times around zero

Yes! d2560ba

Chris-Hibbert · 2022-02-17T01:30:09Z

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

+    // eslint-disable-next-line no-plusplus
+    const vaultId = String(vaultCounter++);


If the style guide asks us to avoid ++, there isn't a good reason to violate that rule here. Doing what the style guide wants is at least as concise and understandable as the work to disable it.

Suggested change

// eslint-disable-next-line no-plusplus

const vaultId = String(vaultCounter++);

vaultCounter += 1;

const vaultId = String(vaultCounter);

Agree completely about not violating style rules. I had a PR to remove this as a rule but it's closed and this is the right way.

Chris-Hibbert · 2022-02-17T01:36:33Z

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

+test('calculateCompoundedInterest', t => {
+  const brand = Far('brand');
+  const cases = [
+    [1n, 1n, 1n, 1n, 1n, 1n], // no charge


Isn't this a bug? Shouldn't something complain if the debt doesn't grow at all?

I lost the context for this but the underlying buggy draft code is gone anyway.

…erest

…h non-methods like vault in VaultKit

Chris-Hibbert

I'm not done with this pass, but these are what I've found so far. I'll be away for a bit, so I thought I should update with interim results.

Chris-Hibbert · 2022-02-17T17:19:05Z

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

-  // better performance, use virtual objects.)
+  let vaultCounter = 0;
+
+  // A store for of vaultKits prioritized by their collaterization ratio.


Suggested change

// A store for of vaultKits prioritized by their collaterization ratio.

// A store for vaultKits prioritized by their collaterization ratio.

Chris-Hibbert · 2022-02-17T17:20:11Z

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

  // definition of reschedulePriceCheck, which refers to sortedVaultKits
-  let sortedVaultKits;
+  // XXX mutability and flow control, could be refactored with a listener


Should this be a TODO? I don't understand the point that it's making. Can you clarify?

I chose XXX to say "here's a known issue" but I don't think we necessarily have to schedule addressing it. I've updated the comment to clarify:

// XXX misleading mutability and confusing flow control; could be refactored with a listener

That's clearer, though I guess I'm not familiar with listeners. Could you say a bit more? [in this thread; I doubt it needs to be added to the comment.]

And I don't see the update here.

Yeah, didn't implement it yet. I've got some more refactoring tasks coming and it might fit into one of those.

Gist is that right now we're delaying construction of A until the callback B is available that need A in scope. What we can do instead is wire up the listening after they're both created.

const A = makeA(); const B = makeB(); A.onEventBCaresAbout(B);

Chris-Hibbert · 2022-02-17T17:34:22Z

packages/zoe/src/contractSupport/ratio.js

 export const floorMultiplyBy = (amount, ratio) => {
+  // @ts-ignore cast


I thought we were converting to ts-expect-error. Should this use that convention?

Yes, though unfortunately we can't until #4560

Chris-Hibbert · 2022-02-17T17:37:22Z

packages/zoe/src/contractSupport/ratio.js

@@ -213,8 +244,47 @@ export const oneMinus = ratio => {
  return makeRatio(
    subtract(ratio.denominator.value, ratio.numerator.value),
    ratio.numerator.brand,
-    // @ts-ignore asserts ensure values are Nats
+    // @ts-ignore value can be any AmountValue but makeRatio() supports only bigint


Isn't this addressed by declaring Ratio to use only Nat Amounts?

Yes, when the value is known to be Amount<NatValue>. However this function declaration is only, {Amount} which can be any kind of amount. Really this function should take only Amount<NatValue> but making that change reduces what this function takes and necessitates updates to every upstream declaration. Out of scope.

Chris-Hibbert

A few more things to clean up. Mostly comments and renamings.

Chris-Hibbert · 2022-02-17T21:19:22Z

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

+    [250n, BASIS_POINTS, M, 10, 1280090n, 5], // 2.5% APR over 10 year yields 28%
+    // XXX resolution was 12 with banker's rounding https://github.com/Agoric/agoric-sdk/issues/4573
+    [250n, BASIS_POINTS, M * M, 10, 1280084544199n, 8], // 2.5% APR over 10 year yields 28%
+    [250n, BASIS_POINTS, M, 100, 11813903n, 5], // 2.5% APR over 10 year yields 1181%


Suggested change

[250n, BASIS_POINTS, M, 100, 11813903n, 5], // 2.5% APR over 10 year yields 1181%

[250n, BASIS_POINTS, M, 100, 11813903n, 5], // 2.5% APR over 100 year yields 1181%

Chris-Hibbert · 2022-02-17T21:27:41Z

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

-};
+/** @typedef {import('./vault').VaultKit} VaultKit */
+
+// TODO put this with other ratio math


obsolete comment

Chris-Hibbert · 2022-02-17T21:35:38Z

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

-  let runDebt = AmountMath.makeEmpty(runBrand);
+
+  /**
+   * Snapshot of the debt and compouneded interest when the principal was last changed


Suggested change

* Snapshot of the debt and compouneded interest when the principal was last changed

* Snapshot of the debt and compounded interest when the principal was last changed

Chris-Hibbert · 2022-02-17T21:36:36Z

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

+  };
+
+  /**
+   * Called whenever principal changes.


whenever principal or debt changes? i.e. in adjustBalancesHook.

Chris-Hibbert · 2022-02-17T21:40:11Z

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

@@ -162,17 +262,32 @@ export const makeVaultKit = (
        throw Error(`unreachable vaultState: ${vaultState}`);
    }
  };
+  // XXX Echo notifications from the manager though all vaults


"though" isn't right. Should it be "to" or "through"?

yeah typo for "through". corrected.

Chris-Hibbert · 2022-02-17T21:44:20Z

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

    // get the payout to provide access to the collateral if the
    // contract abandons
    const {
      give: { Collateral: collateralAmount },
      want: { RUN: wantedRun },
    } = seat.getProposal();

+    if (typeof wantedRun.value !== 'bigint') throw new Error();


I'm surprised eslint allows this. All if bodies should have braces.

All Errors should have a message.

Is this necessary? I expected Zoe to ensure the want amount in a proposal should be value for the issuer.

Not necessary. Maybe leftover from a type safety excursion. Removed now.

Chris-Hibbert · 2022-02-17T21:49:17Z

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

@@ -450,7 +598,7 @@ export const makeVaultKit = (
      );
    }

-    runDebt = AmountMath.add(wantedRun, fee);
+    const runDebt = AmountMath.add(wantedRun, fee);


runDebt was the old name for the amount that's now in debtSnapshot. Here it should be newDebt or stagedDebt to reflect that it won't be updated in debtSnapshot until we perform a couple of checks.

Chris-Hibbert · 2022-02-17T21:54:36Z

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

  // definition of reschedulePriceCheck, which refers to sortedVaultKits
-  let sortedVaultKits;
+  // XXX mutability and flow control, could be refactored with a listener


That's clearer, though I guess I'm not familiar with listeners. Could you say a bit more? [in this thread; I doubt it needs to be added to the comment.]

And I don't see the update here.

Chris-Hibbert · 2022-02-17T21:57:07Z

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

-  /** @type {InnerVaultManager} */
-  const innerFacet = harden({
+  /** @type {Parameters<typeof makeVaultKit>[1]} */
+  const managerFacade = harden({


Is this something other than what we call a facet?

Suggested change

const managerFacade = harden({

const managerFacet = harden({

dtribble · 2022-02-17T20:12:03Z

packages/run-protocol/README.md

+Anyone can make a **Vault** by putting up collateral with the appropriate VaultManager. Then
+they can request RUN that is backed by that collateral.
+
+In any vat, when the ratio of the debt to the collateral exceeds a governed threshold, it is


"in any vault"

dtribble · 2022-02-17T20:33:29Z

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

+      ratioGTE(debtToCollateral, oracleQueryThreshold)
+    ) {
+      // don't call reschedulePriceCheck, but do reset the highest.
+      oracleQueryThreshold = firstDebtRatio();


This could be expensive if we delete individual entries in order. It would be too much to make it computed lazily I expect until we see the actual perf though. So just a note for the future

dtribble · 2022-02-17T20:40:51Z

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

-    }
-    highestDebtToCollateral = highestRatio();
+  /**
+   * @param {Amount<NatValue>} oldDebt


could use a comment about the method :)

dtribble · 2022-02-17T21:30:07Z

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

-  const assertSufficientCollateral = async (collateralAmount, wantedRun) => {
+  const assertSufficientCollateral = async (
+    collateralAmount,
+    proposedRunDebt,


the old name seems better since it aligns with the offer "give/want" terminology.

I agree give/want are terms to use reliably, which is why I think the use of "want" here is an error. "Want" is for a particular transaction and this proposedRunDebt is cumulative. I.e. the total debt if wanted RUN were to be provided.

dtribble · 2022-02-17T22:59:02Z

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

@@ -196,31 +311,30 @@ export const makeVaultKit = (
    // you must pay off the entire remainder but if you offer too much, we won't
    // take more than you owe
    assert(
-      AmountMath.isGTE(runReturned, runDebt),
-      X`You must pay off the entire debt ${runReturned} > ${runDebt}`,
+      AmountMath.isGTE(runReturned, getDebtAmount()),


this should define a local constant for the runDebt. It's both less churn and properly makes it clear that the value is expected to be the same.

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

dtribble · 2022-02-18T17:25:19Z

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

+
+    if (delta > 0n) {
+      // add the amount
+      totalDebt += delta;


Why isn't totalDebt an Amount? There's no reason not to have it be a checked value where our units math is doing the right thing.

(I don't think that's blocking but something we should discuss)

Let's do discuss. This is where @Chris-Hibbert and I arrived. My position for why to have it a bigint here is this makes the arithmetic easier to follow and the Amount wrapper didn't provide any extra assurances within this module because debt brand is invariant.

Totally open to changing back, just want to save it for the virtual objects refactor.

The bug fixed in in d2560ba I think wouldn't have happened if not for wrapping amount values for totalDebt. So I'm convinced you're right that it's better to keep it as an amount: e71db04

dtribble · 2022-02-18T17:28:02Z

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

+      return;
+    }
+
+    if (delta > 0n) {


why the funky absolute value?

const newTotal = totalDebt += delta; assert(newTotal > 0n), 'Negative delta greater than total debt'); // TODO here we will call the factory machine to apply the delta to it totalDebt = newTotal;

Should be fixed?

I agree with the simplification. I don't recall why it's like this and have simplified to,

totalDebt += delta; assert(totalDebt >= 0n, 'Negative delta greater than total debt');

I think the "call the factory machine" element is out of scope.

dtribble · 2022-02-18T17:30:04Z

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

@@ -218,11 +345,14 @@ export const makeVaultManager = (

  observeNotifier(periodNotifier, timeObserver);

-  /** @type {InnerVaultManager} */
-  const innerFacet = harden({


I was trying to be clear that it's closely held (inner), but YMMV

Punting on this topic as it's going to get refactored more by the vault transfer and virtual objects work.

dtribble · 2022-02-18T17:34:37Z

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

+   * @see getNormalizedDebt
+   * @returns {Amount<NatValue>}
+   */
+  // TODO rename to calculateActualDebtAmount throughout codebase https://github.com/Agoric/agoric-sdk/issues/4540


It's perfectly reasonable for this to remain getDebtAmount. The caller doesn't care that it's calculated. the only reason to rename is if it was doing side-effects, which it's not.

Agree that caller doesn't (and shouldn't) care. But I do think that since there is getNormalizedDebt exists it would help the caller to distinguish this as actual debt.

dtribble

LGTM except the one change

dtribble · 2022-02-18T20:49:47Z

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

-      totalDebt -= absDelta;
-    }
+    totalDebt += delta;
+    assert(totalDebt >= 0n, 'Negative delta greater than total debt');


This assert needs to be before the totalDebt gets updated, I think. Hence compute a temporary, assert, then assign.

Agree the lines commented have the bug that if the assert fails totalDebt will have been updated. I thought the thrown error would break more things but that's not a valid assumption; something could catch it and believe the totalDebt that was left.

However, the lines highlighted were removed in e71db04 . That amountMath call will throw if the negative delta was greater than total debt (because the result wouldn't be a natural number, which is really what this is enforcing). totalDebt won't update.

Chris-Hibbert · 2022-02-18T21:28:05Z

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

-  /** @type {NatValue} */
-  let totalDebt = 0n;
+  /** @type {Amount<NatValue>} */
+  let totalDebt = AmountMath.make(runBrand, 0n);


makeEmpty is preferred, even when we know it's a Nat.

I'll get that into #4581 when I rebase it onto master.

turadg requested review from dtribble and Chris-Hibbert February 10, 2022 23:21

turadg changed the title ~~refactor vault storage and liquidation for durable collections~~ redesign vault storage and interest charging for durable collections Feb 11, 2022

turadg force-pushed the ta/4341-durable-prioritizedVaults branch from 7c2b647 to 421a2d9 Compare February 11, 2022 18:48

Chris-Hibbert reviewed Feb 11, 2022

View reviewed changes

This was referenced Feb 12, 2022

Ability for vault observers to know liquidated vs closed #4539

Closed

Refactor vault notifications to not cascade interest charge events #4540

Closed

turadg force-pushed the ta/4341-durable-prioritizedVaults branch from 421a2d9 to 73fc082 Compare February 12, 2022 03:10

turadg changed the title ~~redesign vault storage and interest charging for durable collections~~ redesign vault storage and interest charging for virtual collections Feb 14, 2022

turadg mentioned this pull request Feb 14, 2022

make vault collections durable #4550

Closed

turadg force-pushed the ta/4341-durable-prioritizedVaults branch from 241907d to 5af883b Compare February 14, 2022 22:29

turadg marked this pull request as ready for review February 15, 2022 00:46

turadg force-pushed the ta/4341-durable-prioritizedVaults branch from 1159228 to 3e84c99 Compare February 15, 2022 18:04

Chris-Hibbert reviewed Feb 16, 2022

View reviewed changes

michaelfig reviewed Feb 16, 2022

View reviewed changes

turadg force-pushed the ta/4341-durable-prioritizedVaults branch 3 times, most recently from 59d86db to 7c3bff8 Compare February 16, 2022 19:27

turadg requested a review from Chris-Hibbert February 16, 2022 20:19

Chris-Hibbert reviewed Feb 17, 2022

View reviewed changes

turadg mentioned this pull request Feb 17, 2022

Adopt banker's rounding for interest calculations #4573

Closed

turadg added 7 commits February 16, 2022 21:23

types

bdc0c19

runtime changes for type clarity (safe?)

5cae71a

types

5101d54

make accrued debt a function of prior debt and current compounded int…

9d1b818

…erest

stub for orderedVaultStore

68c894f

progress to hurdle: collectionManager cannot serialize Remotables wit…

3757dc8

…h non-methods like vault in VaultKit

green test-OrderedVaultStore

07cd9f1

turadg added 4 commits February 16, 2022 21:23

factor out and test calculateCompoundedInterest()

754c085

arbitrary precision in compound interest state

5d43e74

refactor(zoe): move ratioGTE

5e4cc5f

cleanup

d3a746f

turadg force-pushed the ta/4341-durable-prioritizedVaults branch from 7c3bff8 to d3a746f Compare February 17, 2022 05:52

turadg requested a review from Chris-Hibbert February 17, 2022 16:15

Chris-Hibbert reviewed Feb 17, 2022

View reviewed changes

docs: code review feedback

9b3d4c2

turadg changed the title ~~redesign vault storage and interest charging for virtual collections~~ refactor(run-protocol): redesign vault storage and interest charging for virtual collections Feb 18, 2022

turadg changed the title ~~refactor(run-protocol): redesign vault storage and interest charging for virtual collections~~ feat(run-protocol): interest charging O(1) for all vaults in a manager Feb 18, 2022

turadg added the automerge:squash Automatically squash merge label Feb 18, 2022

Merge branch 'master' into ta/4341-durable-prioritizedVaults

9c56712

dtribble reviewed Feb 18, 2022

View reviewed changes

turadg removed the automerge:squash Automatically squash merge label Feb 18, 2022

turadg mentioned this pull request Feb 18, 2022

Add limits for amounts between vault and vault manager #4599

Closed

turadg added 3 commits February 18, 2022 10:23

doc: code review improvements

ceb95a2

correct initial latestInterestUpdate

d2560ba

store totalDebt as Amount

e71db04

Chris-Hibbert approved these changes Feb 18, 2022

View reviewed changes

dtribble requested changes Feb 18, 2022

View reviewed changes

Chris-Hibbert reviewed Feb 18, 2022

View reviewed changes

dtribble approved these changes Feb 18, 2022

View reviewed changes

turadg added the automerge:squash Automatically squash merge label Feb 18, 2022

Merge branch 'master' into ta/4341-durable-prioritizedVaults

d10e0e2

mergify bot merged commit 58103ac into master Feb 18, 2022

mergify bot deleted the ta/4341-durable-prioritizedVaults branch February 18, 2022 22:45

turadg mentioned this pull request Mar 5, 2022

refactor(run-protocol): extract a vaultKit module #4743

Merged

turadg mentioned this pull request Apr 19, 2022

fix(run-protocol): vault debt ratio ordering after interest charges #5149

Merged

mhofman mentioned this pull request May 30, 2022

non-deterministic crash of single node under load #5031

Closed

		@@ -1,4 +1,6 @@
		// @ts-check
		// XXX we do this a lot, okay to drop it to warning in eslint config?

+                });
+              };
+              /**

	// divide compounded interest by the the snapshot
	// divide compounded interest by the snapshot

	* @yields {[string, VaultKit]>}
	* @yields {[string, VaultKit]}

		Anyone can open make a Vault by putting up collateral the appropriate VaultManager. Then they can request RUN that is backed by that collateral.

		When any vat the ratio of the debt to the collateral exceeds a governed threshold, it is deemed undercollateralized. If the result of a price check shows that a vault is undercollateralized. the VaultManager liquidates it.

		// eslint-disable-next-line no-plusplus
		const vaultId = String(vaultCounter++);

	// A store for of vaultKits prioritized by their collaterization ratio.
	// A store for vaultKits prioritized by their collaterization ratio.

		export const floorMultiplyBy = (amount, ratio) => {
		// @ts-ignore cast

	[250n, BASIS_POINTS, M, 100, 11813903n, 5], // 2.5% APR over 10 year yields 1181%
	[250n, BASIS_POINTS, M, 100, 11813903n, 5], // 2.5% APR over 100 year yields 1181%

	* Snapshot of the debt and compouneded interest when the principal was last changed
	* Snapshot of the debt and compounded interest when the principal was last changed

feat(run-protocol): interest charging O(1) for all vaults in a manager #4527

feat(run-protocol): interest charging O(1) for all vaults in a manager #4527

Conversation

turadg commented Feb 10, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

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

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

dckc Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

turadg commented Feb 15, 2022

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 Feb 16, 2022 • edited Loading

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

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 Feb 17, 2022 • edited Loading

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

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

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

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 Feb 10, 2022 •

edited

Loading

dckc Feb 14, 2022 •

edited

Loading

turadg Feb 16, 2022 •

edited

Loading

turadg Feb 17, 2022 •

edited

Loading