Skip to content

Commit

Permalink
fix: giving collateral should not change totalDebt (#9873)
Browse files Browse the repository at this point in the history
closes: #XXXX
refs: #XXXX

## Description

Updating `totalDebt` for 1 `vault` was doing `totalDebt += vault.getCurrentDebt() - old` where current debt includes compounded interest but old did not.

### Security Considerations

This (in combination with another PR to reset totalDebt to the sum over all vaults) should address a confusion risk for metrics clients.

### Scaling Considerations

The separate work to reset totalDebt to the sum over all vaults may have some, but I don't see any here.

### Testing Considerations

A straightforward unit test is included.

### Upgrade Considerations

This change takes effect with the next upgrade to the `vaultFactory`.

### Documentation Considerations

As part of the governance decision to upgrade `vaultFactory`, metrics consumers should be notified of this fix.
  • Loading branch information
mergify[bot] authored Aug 20, 2024
2 parents aaa0def + e29a42c commit a18dbbf
Show file tree
Hide file tree
Showing 7 changed files with 91 additions and 29 deletions.
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/interest.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ export const chargeInterest = (powers, params, prior, accruedUntil) => {
}

// NB: This method of inferring the compounded rate from the ratio of debts
// acrrued suffers slightly from the integer nature of debts. However in
// accrued suffers slightly from the integer nature of debts. However in
// testing with small numbers there's 5 digits of precision, and with large
// numbers the ratios tend towards ample precision.
// TODO adopt banker's rounding https://github.com/Agoric/agoric-sdk/issues/4573
Expand Down
4 changes: 3 additions & 1 deletion packages/inter-protocol/src/vaultFactory/vaultManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {
TopicsRecordShape,
} from '@agoric/zoe/src/contractSupport/index.js';
import { PriceQuoteShape, SeatShape } from '@agoric/zoe/src/typeGuards.js';
import { multiplyBy } from '@agoric/zoe/src/contractSupport/ratio.js';
import {
checkDebtLimit,
makeNatAmountShape,
Expand Down Expand Up @@ -993,8 +994,9 @@ export const prepareVaultManagerKit = (
);
state.totalDebt = AmountMath.subtract(
AmountMath.add(state.totalDebt, vault.getCurrentDebt()),
oldDebtNormalized,
multiplyBy(oldDebtNormalized, state.compoundedInterest),
);

void facets.helper.writeMetrics();
},
},
Expand Down
21 changes: 20 additions & 1 deletion packages/inter-protocol/test/vaultFactory/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ const getRunFromFaucet = async (t, amt) => {
* @param {Amount} priceBase
*/
const setupServices = async (t, initialPrice, priceBase) => {
const timer = buildZoeManualTimer(t.log);
const timer = buildZoeManualTimer(t.log, 0n, { timeStep: 60n * 60n });
const { zoe, run, aeth, interestTiming, minInitialDebt, rates } = t.context;
t.context.timer = timer;

Expand Down Expand Up @@ -336,6 +336,10 @@ export const makeManagerDriver = async (
publicTopics.asset.subscriber,
);
let managerNotification = await E(managerNotifier).getUpdateSince();
const metricsNotifier = await makeNotifierFromSubscriber(
publicTopics.metrics.subscriber,
);
let metricsNotification = await E(metricsNotifier).getUpdateSince();

/** @type {UserSeat} */
let currentSeat;
Expand Down Expand Up @@ -559,6 +563,21 @@ export const makeManagerDriver = async (
}
return managerNotification;
},
/**
* @param {object} [likeExpected]
* @param {AT_NEXT | number} [optSince] AT_NEXT is an alias for updateCount
* of the last update, forcing to wait for another
*/
metricsNotified: async (likeExpected, optSince) => {
metricsNotification = await E(metricsNotifier).getUpdateSince(
optSince === AT_NEXT ? metricsNotification.updateCount : optSince,
);
trace(t, 'metrics notifier', metricsNotification);
if (likeExpected) {
t.like(metricsNotification.value, likeExpected);
}
return managerNotification;
},
checkReserveAllocation: async stableValue => {
const { reserveCreatorFacet } = t.context;
const reserveAllocations = await E(reserveCreatorFacet).getAllocations();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ Generated by [AVA](https://avajs.dev).
},
totalDebt: {
brand: Object @Alleged: IST brand {},
value: 1634n,
value: 1608n,
},
totalOverageReceived: {
brand: Object @Alleged: IST brand {},
Expand Down
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import { test as unknownTest } from '@agoric/zoe/tools/prepare-test-env-ava.js';

import { makeTracer } from '@agoric/internal';
import { E } from '@endo/eventual-send';
import { makeDriverContext, makeManagerDriver } from './driver.js';
import { eventLoopIteration } from '@agoric/internal/src/testing-utils.js';
import { AT_NEXT, makeDriverContext, makeManagerDriver } from './driver.js';

/** @typedef {import('./driver.js').DriverContext & {}} Context */
/** @type {import('ava').TestFn<Context>} */
Expand All @@ -15,6 +16,46 @@ test.before(async t => {
trace(t, 'CONTEXT');
});

test('totalDebt calculation includes compoundedInterest', async t => {
const { aeth, run } = t.context;
const md = await makeManagerDriver(t);
const timer = md.timer();

t.log('charge 2% per day to simulate lower rates over longer times');
const aethKey = { collateralBrand: aeth.brand };
const twoPctPerDay = run.makeRatio(2n * 360n, 100n);
await md.setGovernedParam('InterestRate', twoPctPerDay, { key: aethKey });
const plausibleDebtLimit = run.units(500000);
await md.setGovernedParam('DebtLimit', plausibleDebtLimit, { key: aethKey });

t.log('mint 100 IST against 25 AETH');
const vd = await md.makeVaultDriver(aeth.units(25), run.units(100));
const v1debtAfterMint = await E(vd.vault()).getCurrentDebt();
t.deepEqual(v1debtAfterMint, run.units(100 + 5));
// totalDebt is debit of this 1 vault
await md.metricsNotified({ totalDebt: v1debtAfterMint }, AT_NEXT);

await E(timer).tickN(40, 'interest accumulates over 40hrs');
await eventLoopIteration(); // let all timer-induced promises settle
const v1debtAfterDay = await E(vd.vault()).getCurrentDebt();
t.deepEqual(v1debtAfterDay, run.make(106_008_000n));

// Checking that totalDebt here matches v1debtAfterDay would be handy,
// but totalDebt isn't updated when calculating interest.
const expectedCompoundedInterest = {
denominator: { value: 100000000000000000000n },
numerator: { value: 100960000000000000000n },
};
await md.managerNotified({ compoundedInterest: expectedCompoundedInterest });

t.log('give some collateral (adjustBalances); no change to debt');
await E(vd).giveCollateral(50n, aeth);
const v1debtAfterGive = await E(vd.vault()).getCurrentDebt();
t.deepEqual(v1debtAfterGive, v1debtAfterDay, 'no debt change');

await md.metricsNotified({ totalDebt: v1debtAfterDay }, AT_NEXT);
});

test('excessive loan', async t => {
const { aeth, run } = t.context;
const md = await makeManagerDriver(t);
Expand Down
48 changes: 24 additions & 24 deletions packages/inter-protocol/test/vaultFactory/vaultFactory.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1838,6 +1838,13 @@ test('manager notifiers, with snapshot', async t => {
}),
);
({ vault } = await E(vaultSeat).getOfferResult());
totalCollateral += ENOUGH;
totalDebt += DEBT2;
await m.assertChange({
numActiveVaults: 4,
totalCollateral: { value: totalCollateral },
totalDebt: { value: totalDebt },
});

trace('6. Loan interest');
vaultSeat = await E(services.zoe).offer(
Expand All @@ -1851,18 +1858,21 @@ test('manager notifiers, with snapshot', async t => {
}),
);
({ vault } = await E(vaultSeat).getOfferResult());
totalCollateral += ENOUGH;
totalDebt += DEBT2;
totalCollateral += AMPLE;
totalDebt += DEBT1;
await m.assertChange({
numActiveVaults: 4,
numActiveVaults: 5,
totalCollateral: { value: totalCollateral },
totalDebt: { value: totalDebt },
});
m.addDebt(DEBT2);

await manualTimer.tickN(5);
// This is interest for a single vault.
const interestAccrued = (await E(vault).getCurrentDebt()).value - DEBT1;
m.addDebt(interestAccrued);
t.is(interestAccrued, 9n);

t.is(interestAccrued, 9n); // interest on OPEN1 for 5 periods
totalDebt += 30n; // interest on 270_000 for 5 periods

trace('7. make another loan to trigger a publish');
vaultSeat = await E(services.zoe).offer(
Expand All @@ -1876,10 +1886,10 @@ test('manager notifiers, with snapshot', async t => {
}),
);
({ vault } = await E(vaultSeat).getOfferResult());
totalCollateral += AMPLE;
totalDebt += DEBT1;
totalCollateral += ENOUGH;
totalDebt += DEBT2;
await m.assertChange({
numActiveVaults: 5,
numActiveVaults: 6,
totalCollateral: { value: totalCollateral },
totalDebt: { value: totalDebt },
});
Expand All @@ -1898,11 +1908,10 @@ test('manager notifiers, with snapshot', async t => {
}),
);
({ vault } = await E(vaultSeat).getOfferResult());
totalCollateral += ENOUGH;
totalDebt += DEBT2 + 30n; // XXX ??
m.addDebt(DEBT2);
totalCollateral += AMPLE;
totalDebt += DEBT1;
await m.assertChange({
numActiveVaults: 6,
numActiveVaults: 7,
totalCollateral: { value: totalCollateral },
totalDebt: { value: totalDebt },
});
Expand All @@ -1925,20 +1934,11 @@ test('manager notifiers, with snapshot', async t => {
}),
);
await E(vaultOpSeat).getOfferResult();
totalCollateral += AMPLE;
totalDebt += DEBT1;
await m.assertChange({
numActiveVaults: 7,
totalDebt: { value: totalDebt },
totalCollateral: { value: totalCollateral },
});

totalCollateral += given.value;
totalDebt += WANT_EXTRA + 29n; // magic number is fees

totalDebt += WANT_EXTRA + 20n;
await m.assertChange({
totalCollateral: { value: totalCollateral },
totalDebt: { value: totalDebt },
totalCollateral: { value: totalCollateral },
});

trace('10. Close vault');
Expand All @@ -1955,7 +1955,7 @@ test('manager notifiers, with snapshot', async t => {
await E(vaultOpSeat).getOfferResult();

totalCollateral -= AMPLE + given.value;
totalDebt -= DEBT1_EXTRA - 17n; // magic number is fees
totalDebt -= DEBT1_EXTRA;
await m.assertChange({
numActiveVaults: 6,
totalCollateral: { value: totalCollateral },
Expand Down

0 comments on commit a18dbbf

Please sign in to comment.