From 4e12571426425dca4be39b2270a12cad318b0d67 Mon Sep 17 00:00:00 2001 From: Turadg Aleahmad Date: Tue, 22 Feb 2022 07:44:26 -0800 Subject: [PATCH 1/4] style(run-protocol): rename VaultState to VaultPhase --- .../run-protocol/src/vaultFactory/vault.js | 47 +++++++++---------- .../test/vaultFactory/test-vaultFactory.js | 12 ++--- 2 files changed, 29 insertions(+), 30 deletions(-) diff --git a/packages/run-protocol/src/vaultFactory/vault.js b/packages/run-protocol/src/vaultFactory/vault.js index 4eb313cc794..7c1289bc467 100644 --- a/packages/run-protocol/src/vaultFactory/vault.js +++ b/packages/run-protocol/src/vaultFactory/vault.js @@ -28,18 +28,16 @@ const trace = makeTracer('Vault'); // collateral, and lending RUN to the borrower /** - * Constants for vault state. + * Constants for vault phase. * - * @typedef {'active' | 'liquidating' | 'closed' | 'transfer'} VAULT_STATE - * - * @type {{ ACTIVE: 'active', LIQUIDATING: 'liquidating', CLOSED: 'closed', TRANSFER: 'transfer' }} + * @typedef {VaultPhase[keyof typeof VaultPhase]} VAULT_PHASE */ -export const VaultState = { +export const VaultPhase = /** @type {const} */ ({ ACTIVE: 'active', LIQUIDATING: 'liquidating', CLOSED: 'closed', TRANSFER: 'transfer', -}; +}); const makeOuterKit = inner => { const { updater: uiUpdater, notifier } = makeNotifierKit(); @@ -102,11 +100,11 @@ export const makeInnerVault = ( const { zcfSeat: liquidationZcfSeat, userSeat: liquidationSeat } = zcf.makeEmptySeatKit(undefined); - /** @type {VAULT_STATE} */ - let vaultState = VaultState.ACTIVE; + /** @type {VAULT_PHASE} */ + let phase = VaultPhase.ACTIVE; const assertVaultIsOpen = () => { - assert(vaultState === VaultState.ACTIVE, X`vault must still be active`); + assert(phase === VaultPhase.ACTIVE, X`vault must still be active`); }; let outerUpdater; @@ -239,7 +237,7 @@ export const makeInnerVault = ( : getCollateralAllocated(vaultSeat); }; - const snapshotState = vstate => { + const snapshotState = newPhase => { /** @type {VaultUIState} */ return harden({ // TODO move manager state to a separate notifer https://github.com/Agoric/agoric-sdk/issues/4540 @@ -249,8 +247,9 @@ export const makeInnerVault = ( locked: getCollateralAmount(), debt: getDebtAmount(), // TODO state distinct from CLOSED https://github.com/Agoric/agoric-sdk/issues/4539 - liquidated: vaultState === VaultState.CLOSED, - vaultState: vstate, + // XXX uses closure value instead of argument + liquidated: phase === VaultPhase.CLOSED, + vaultState: newPhase, }); }; @@ -260,26 +259,26 @@ export const makeInnerVault = ( return; } /** @type {VaultUIState} */ - const uiState = snapshotState(vaultState); + const uiState = snapshotState(phase); trace('updateUiState', uiState); - switch (vaultState) { - case VaultState.ACTIVE: - case VaultState.LIQUIDATING: + switch (phase) { + case VaultPhase.ACTIVE: + case VaultPhase.LIQUIDATING: outerUpdater.updateState(uiState); break; - case VaultState.CLOSED: + case VaultPhase.CLOSED: outerUpdater.finish(uiState); break; default: - throw Error(`unreachable vaultState: ${vaultState}`); + throw Error(`unreachable vaultState: ${phase}`); } }; // XXX Echo notifications from the manager through all vaults // TODO move manager state to a separate notifer https://github.com/Agoric/agoric-sdk/issues/4540 observeNotifier(managerNotifier, { updateState: () => { - if (vaultState !== VaultState.CLOSED) { + if (phase !== VaultPhase.CLOSED) { updateUiState(); } }, @@ -293,15 +292,15 @@ export const makeInnerVault = ( const liquidated = newDebt => { updateDebtSnapshot(newDebt); - vaultState = VaultState.CLOSED; + phase = VaultPhase.CLOSED; updateUiState(); }; const liquidating = () => { - if (vaultState === VaultState.LIQUIDATING) { + if (phase === VaultPhase.LIQUIDATING) { throw new Error('Vault already liquidating'); } - vaultState = VaultState.LIQUIDATING; + phase = VaultPhase.LIQUIDATING; updateUiState(); }; @@ -342,7 +341,7 @@ export const makeInnerVault = ( runMint.burnLosses(harden({ RUN: currentDebt }), burnSeat); seat.exit(); burnSeat.exit(); - vaultState = VaultState.CLOSED; + phase = VaultPhase.CLOSED; updateDebtSnapshot(AmountMath.makeEmpty(runBrand)); updateUiState(); @@ -670,7 +669,7 @@ export const makeInnerVault = ( makeCloseInvitation, makeTransferInvitation: () => { if (outerUpdater) { - outerUpdater.finish(snapshotState(VaultState.TRANSFER)); + outerUpdater.finish(snapshotState(VaultPhase.TRANSFER)); outerUpdater = null; } return zcf.makeInvitation(makeTransferInvitationHook, 'TransferVault'); diff --git a/packages/run-protocol/test/vaultFactory/test-vaultFactory.js b/packages/run-protocol/test/vaultFactory/test-vaultFactory.js index b109c8ba7dc..abad1d03a8e 100644 --- a/packages/run-protocol/test/vaultFactory/test-vaultFactory.js +++ b/packages/run-protocol/test/vaultFactory/test-vaultFactory.js @@ -25,7 +25,7 @@ import { import { makeTracer } from '../../src/makeTracer.js'; import { SECONDS_PER_YEAR } from '../../src/vaultFactory/interest.js'; -import { VaultState } from '../../src/vaultFactory/vault.js'; +import { VaultPhase } from '../../src/vaultFactory/vault.js'; import { CHARGING_PERIOD_KEY, RECORDING_PERIOD_KEY, @@ -525,7 +525,7 @@ test('price drop', async t => { notification = await E(uiNotifier).getUpdateSince(notification.updateCount); trace('price changed to liquidate', notification.value.vaultState); t.falsy(notification.value.liquidated); - t.is(notification.value.vaultState, VaultState.LIQUIDATING); + t.is(notification.value.vaultState, VaultPhase.LIQUIDATING); await manualTimer.tick(); notification = await E(uiNotifier).getUpdateSince(notification.updateCount); @@ -1270,14 +1270,14 @@ test('transfer vault', async t => { const aliceFinish = await E(aliceNotifier).getUpdateSince(); t.deepEqual( aliceFinish.value.vaultState, - VaultState.TRANSFER, + VaultPhase.TRANSFER, 'transfer closed old notifier', ); const transferStatus = await E(transferNotifier).getUpdateSince(); t.deepEqual( transferStatus.value.vaultState, - VaultState.ACTIVE, + VaultPhase.ACTIVE, 'new notifier is active', ); @@ -1322,14 +1322,14 @@ test('transfer vault', async t => { const transferFinish = await E(transferNotifier).getUpdateSince(); t.deepEqual( transferFinish.value.vaultState, - VaultState.TRANSFER, + VaultPhase.TRANSFER, 't2 closed old notifier', ); const t2Status = await E(t2Notifier).getUpdateSince(); t.deepEqual( t2Status.value.vaultState, - VaultState.ACTIVE, + VaultPhase.ACTIVE, 'new notifier is active', ); }); From 26593e4eca7aa7997d56470c7892c30d6d9b6f93 Mon Sep 17 00:00:00 2001 From: Turadg Aleahmad Date: Tue, 22 Feb 2022 14:05:32 -0800 Subject: [PATCH 2/4] feat(run-protocol): distinct vault phase for liquidated --- .../run-protocol/src/vaultFactory/types.js | 2 +- .../run-protocol/src/vaultFactory/vault.js | 25 ++++++++++++++++--- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/packages/run-protocol/src/vaultFactory/types.js b/packages/run-protocol/src/vaultFactory/types.js index 38d2dddb4fb..a4f5e6455ff 100644 --- a/packages/run-protocol/src/vaultFactory/types.js +++ b/packages/run-protocol/src/vaultFactory/types.js @@ -66,7 +66,7 @@ * @property {Ratio} interestRate Annual interest rate charge * @property {Ratio} liquidationRatio * @property {boolean} liquidated boolean showing whether liquidation occurred - * @property {'active' | 'liquidating' | 'closed'} vaultState + * @property {'active' | 'liquidating' | 'liquidated' | 'closed' | 'transfer'} vaultState */ /** diff --git a/packages/run-protocol/src/vaultFactory/vault.js b/packages/run-protocol/src/vaultFactory/vault.js index 7c1289bc467..99a653aaa58 100644 --- a/packages/run-protocol/src/vaultFactory/vault.js +++ b/packages/run-protocol/src/vaultFactory/vault.js @@ -30,12 +30,29 @@ const trace = makeTracer('Vault'); /** * Constants for vault phase. * + * ACTIVE - vault is in use and can be changed + * LIQUIDATING - vault is being liquidated by the vault manager, and cannot be changed by the user + * TRANSFER - vault is released from the manager and able to be transferred + * CLOSED - vault was closed by the user and all assets have been paid out + * LIQUIDATED - vault was closed by the manager, with remaining assets paid to owner + * + * These are the valid state transitions: + * Active -> Liquidating + * Active -> Transferrable + * Active -> Closed + * Liquidating -> Liquidated + * Transferrable -> Active + * Transferrable -> Liquidating + * + * (Liquidated and Closed cannot be changed) + * * @typedef {VaultPhase[keyof typeof VaultPhase]} VAULT_PHASE */ export const VaultPhase = /** @type {const} */ ({ ACTIVE: 'active', LIQUIDATING: 'liquidating', CLOSED: 'closed', + LIQUIDATED: 'liquidated', TRANSFER: 'transfer', }); @@ -246,9 +263,8 @@ export const makeInnerVault = ( debtSnapshot, locked: getCollateralAmount(), debt: getDebtAmount(), - // TODO state distinct from CLOSED https://github.com/Agoric/agoric-sdk/issues/4539 // XXX uses closure value instead of argument - liquidated: phase === VaultPhase.CLOSED, + liquidated: phase === VaultPhase.LIQUIDATED, vaultState: newPhase, }); }; @@ -256,6 +272,7 @@ export const makeInnerVault = ( // call this whenever anything changes! const updateUiState = () => { if (!outerUpdater) { + console.warn('updateUiState called after outerUpdater removed'); return; } /** @type {VaultUIState} */ @@ -268,7 +285,9 @@ export const makeInnerVault = ( outerUpdater.updateState(uiState); break; case VaultPhase.CLOSED: + case VaultPhase.LIQUIDATED: outerUpdater.finish(uiState); + outerUpdater = null; break; default: throw Error(`unreachable vaultState: ${phase}`); @@ -292,7 +311,7 @@ export const makeInnerVault = ( const liquidated = newDebt => { updateDebtSnapshot(newDebt); - phase = VaultPhase.CLOSED; + phase = VaultPhase.LIQUIDATED; updateUiState(); }; From 6456af25e154f01820efbdc1afb343902e385384 Mon Sep 17 00:00:00 2001 From: Turadg Aleahmad Date: Wed, 23 Feb 2022 12:36:46 -0800 Subject: [PATCH 3/4] chore(run-protocol)!: remove liquidated flag from vault notifications --- .../run-protocol/src/vaultFactory/types.js | 1 - .../run-protocol/src/vaultFactory/vault.js | 2 - .../test/vaultFactory/test-vaultFactory.js | 37 +++++++++++-------- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/packages/run-protocol/src/vaultFactory/types.js b/packages/run-protocol/src/vaultFactory/types.js index a4f5e6455ff..6451e86841b 100644 --- a/packages/run-protocol/src/vaultFactory/types.js +++ b/packages/run-protocol/src/vaultFactory/types.js @@ -65,7 +65,6 @@ * @typedef {Object} LiquidationUIMixin * @property {Ratio} interestRate Annual interest rate charge * @property {Ratio} liquidationRatio - * @property {boolean} liquidated boolean showing whether liquidation occurred * @property {'active' | 'liquidating' | 'liquidated' | 'closed' | 'transfer'} vaultState */ diff --git a/packages/run-protocol/src/vaultFactory/vault.js b/packages/run-protocol/src/vaultFactory/vault.js index 99a653aaa58..8fc59699b9b 100644 --- a/packages/run-protocol/src/vaultFactory/vault.js +++ b/packages/run-protocol/src/vaultFactory/vault.js @@ -263,8 +263,6 @@ export const makeInnerVault = ( debtSnapshot, locked: getCollateralAmount(), debt: getDebtAmount(), - // XXX uses closure value instead of argument - liquidated: phase === VaultPhase.LIQUIDATED, vaultState: newPhase, }); }; diff --git a/packages/run-protocol/test/vaultFactory/test-vaultFactory.js b/packages/run-protocol/test/vaultFactory/test-vaultFactory.js index abad1d03a8e..6c70c9da8c2 100644 --- a/packages/run-protocol/test/vaultFactory/test-vaultFactory.js +++ b/packages/run-protocol/test/vaultFactory/test-vaultFactory.js @@ -25,7 +25,6 @@ import { import { makeTracer } from '../../src/makeTracer.js'; import { SECONDS_PER_YEAR } from '../../src/vaultFactory/interest.js'; -import { VaultPhase } from '../../src/vaultFactory/vault.js'; import { CHARGING_PERIOD_KEY, RECORDING_PERIOD_KEY, @@ -44,6 +43,15 @@ const trace = makeTracer('TestST'); const BASIS_POINTS = 10000n; +// Define locally to test that vaultFactory uses these values +export const Phase = /** @type {const} */ ({ + ACTIVE: 'active', + LIQUIDATING: 'liquidating', + CLOSED: 'closed', + LIQUIDATED: 'liquidated', + TRANSFER: 'transfer', +}); + async function makeBundle(sourceRoot) { const url = await importMetaResolve(sourceRoot, import.meta.url); const path = new URL(url).pathname; @@ -508,7 +516,7 @@ test('price drop', async t => { let notification = await E(uiNotifier).getUpdateSince(); trace('got notificaation', notification); - t.falsy(notification.value.liquidated); + t.is(notification.value.vaultState, Phase.ACTIVE); t.deepEqual(await notification.value.debt, AmountMath.add(loanAmount, fee)); const { RUN: lentAmount } = await E(loanSeat).getCurrentAllocation(); t.truthy(AmountMath.isEqual(lentAmount, loanAmount), 'received 470 RUN'); @@ -519,22 +527,21 @@ test('price drop', async t => { ); await manualTimer.tick(); notification = await E(uiNotifier).getUpdateSince(); - t.falsy(notification.value.liquidated); + t.is(notification.value.vaultState, Phase.ACTIVE); await manualTimer.tick(); notification = await E(uiNotifier).getUpdateSince(notification.updateCount); trace('price changed to liquidate', notification.value.vaultState); - t.falsy(notification.value.liquidated); - t.is(notification.value.vaultState, VaultPhase.LIQUIDATING); + t.is(notification.value.vaultState, Phase.LIQUIDATING); await manualTimer.tick(); notification = await E(uiNotifier).getUpdateSince(notification.updateCount); t.falsy(notification.updateCount); - t.truthy(notification.value.liquidated); + t.is(notification.value.vaultState, Phase.LIQUIDATED); const debtAmountAfter = await E(vault).getDebtAmount(); const finalNotification = await E(uiNotifier).getUpdateSince(); - t.truthy(finalNotification.value.liquidated); + t.is(finalNotification.value.vaultState, Phase.LIQUIDATED); t.truthy(AmountMath.isEmpty(debtAmountAfter)); t.deepEqual(await E(vaultFactory).getRewardAllocation(), { @@ -1270,14 +1277,14 @@ test('transfer vault', async t => { const aliceFinish = await E(aliceNotifier).getUpdateSince(); t.deepEqual( aliceFinish.value.vaultState, - VaultPhase.TRANSFER, + Phase.TRANSFER, 'transfer closed old notifier', ); const transferStatus = await E(transferNotifier).getUpdateSince(); t.deepEqual( transferStatus.value.vaultState, - VaultPhase.ACTIVE, + Phase.ACTIVE, 'new notifier is active', ); @@ -1322,14 +1329,14 @@ test('transfer vault', async t => { const transferFinish = await E(transferNotifier).getUpdateSince(); t.deepEqual( transferFinish.value.vaultState, - VaultPhase.TRANSFER, + Phase.TRANSFER, 't2 closed old notifier', ); const t2Status = await E(t2Notifier).getUpdateSince(); t.deepEqual( t2Status.value.vaultState, - VaultPhase.ACTIVE, + Phase.ACTIVE, 'new notifier is active', ); }); @@ -1657,7 +1664,7 @@ test('mutable liquidity triggers and interest', async t => { await waitForPromisesToSettle(); aliceUpdate = await E(aliceNotifier).getUpdateSince(aliceUpdate.updateCount); bobUpdate = await E(bobNotifier).getUpdateSince(); - t.truthy(aliceUpdate.value.liquidated); + t.is(aliceUpdate.value.vaultState, Phase.LIQUIDATED); for (let i = 0; i < 5; i += 1) { manualTimer.tick(); @@ -1665,7 +1672,7 @@ test('mutable liquidity triggers and interest', async t => { await waitForPromisesToSettle(); bobUpdate = await E(bobNotifier).getUpdateSince(); - t.truthy(bobUpdate.value.liquidated); + t.is(bobUpdate.value.vaultState, Phase.LIQUIDATED); }); test('bad chargingPeriod', async t => { @@ -2159,7 +2166,7 @@ test('mutable liquidity triggers and interest sensitivity', async t => { await waitForPromisesToSettle(); aliceUpdate = await E(aliceNotifier).getUpdateSince(aliceUpdate.updateCount); bobUpdate = await E(bobNotifier).getUpdateSince(); - t.falsy(aliceUpdate.value.liquidated); + t.is(aliceUpdate.value.vaultState, Phase.ACTIVE); for (let i = 0; i < 5; i += 1) { manualTimer.tick(); @@ -2167,5 +2174,5 @@ test('mutable liquidity triggers and interest sensitivity', async t => { await waitForPromisesToSettle(); bobUpdate = await E(bobNotifier).getUpdateSince(); - t.truthy(bobUpdate.value.liquidated); + t.is(bobUpdate.value.vaultState, Phase.LIQUIDATED); }); From 59ee9b03f4c40810d91d5a691c83d5d1c59d8918 Mon Sep 17 00:00:00 2001 From: Turadg Aleahmad Date: Wed, 23 Feb 2022 14:57:09 -0800 Subject: [PATCH 4/4] refactor(run-protocol): validate vault phase transitions --- .../run-protocol/src/vaultFactory/vault.js | 63 +++++++++++++------ 1 file changed, 45 insertions(+), 18 deletions(-) diff --git a/packages/run-protocol/src/vaultFactory/vault.js b/packages/run-protocol/src/vaultFactory/vault.js index 8fc59699b9b..3e91e3419f4 100644 --- a/packages/run-protocol/src/vaultFactory/vault.js +++ b/packages/run-protocol/src/vaultFactory/vault.js @@ -33,19 +33,10 @@ const trace = makeTracer('Vault'); * ACTIVE - vault is in use and can be changed * LIQUIDATING - vault is being liquidated by the vault manager, and cannot be changed by the user * TRANSFER - vault is released from the manager and able to be transferred + * TRANSFER - vault is able to be transferred (payments and debits frozen until it has a new owner) * CLOSED - vault was closed by the user and all assets have been paid out * LIQUIDATED - vault was closed by the manager, with remaining assets paid to owner * - * These are the valid state transitions: - * Active -> Liquidating - * Active -> Transferrable - * Active -> Closed - * Liquidating -> Liquidated - * Transferrable -> Active - * Transferrable -> Liquidating - * - * (Liquidated and Closed cannot be changed) - * * @typedef {VaultPhase[keyof typeof VaultPhase]} VAULT_PHASE */ export const VaultPhase = /** @type {const} */ ({ @@ -56,6 +47,21 @@ export const VaultPhase = /** @type {const} */ ({ TRANSFER: 'transfer', }); +/** + * @type {{[K in VAULT_PHASE]: Array}} + */ +const validTransitions = { + [VaultPhase.ACTIVE]: [ + VaultPhase.LIQUIDATING, + VaultPhase.TRANSFER, + VaultPhase.CLOSED, + ], + [VaultPhase.LIQUIDATING]: [VaultPhase.LIQUIDATED], + [VaultPhase.TRANSFER]: [VaultPhase.ACTIVE, VaultPhase.LIQUIDATING], + [VaultPhase.LIQUIDATED]: [], + [VaultPhase.CLOSED]: [], +}; + const makeOuterKit = inner => { const { updater: uiUpdater, notifier } = makeNotifierKit(); @@ -117,12 +123,31 @@ export const makeInnerVault = ( const { zcfSeat: liquidationZcfSeat, userSeat: liquidationSeat } = zcf.makeEmptySeatKit(undefined); + // #region Phase state /** @type {VAULT_PHASE} */ let phase = VaultPhase.ACTIVE; + /** + * @param {VAULT_PHASE} newPhase + */ + const assignPhase = newPhase => { + const validNewPhases = validTransitions[phase]; + if (!validNewPhases.includes(newPhase)) + throw new Error(`Vault cannot transition from ${phase} to ${newPhase}`); + phase = newPhase; + }; + + const assertPhase = allegedPhase => { + assert( + phase === allegedPhase, + X`vault must be ${allegedPhase}, not ${phase}`, + ); + }; + const assertVaultIsOpen = () => { - assert(phase === VaultPhase.ACTIVE, X`vault must still be active`); + assertPhase(VaultPhase.ACTIVE); }; + // #endregion let outerUpdater; @@ -263,6 +288,8 @@ export const makeInnerVault = ( debtSnapshot, locked: getCollateralAmount(), debt: getDebtAmount(), + // newPhase param is so that makeTransferInvitation can finish without setting the vault's phase + // TODO refactor https://github.com/Agoric/agoric-sdk/issues/4415 vaultState: newPhase, }); }; @@ -287,8 +314,11 @@ export const makeInnerVault = ( outerUpdater.finish(uiState); outerUpdater = null; break; + case VaultPhase.TRANSFER: + // Transfer handles finish()/null itself + throw Error('no UI updates from transfer state'); default: - throw Error(`unreachable vaultState: ${phase}`); + throw Error(`unreachable vault phase: ${phase}`); } }; // XXX Echo notifications from the manager through all vaults @@ -309,15 +339,12 @@ export const makeInnerVault = ( const liquidated = newDebt => { updateDebtSnapshot(newDebt); - phase = VaultPhase.LIQUIDATED; + assignPhase(VaultPhase.LIQUIDATED); updateUiState(); }; const liquidating = () => { - if (phase === VaultPhase.LIQUIDATING) { - throw new Error('Vault already liquidating'); - } - phase = VaultPhase.LIQUIDATING; + assignPhase(VaultPhase.LIQUIDATING); updateUiState(); }; @@ -358,7 +385,7 @@ export const makeInnerVault = ( runMint.burnLosses(harden({ RUN: currentDebt }), burnSeat); seat.exit(); burnSeat.exit(); - phase = VaultPhase.CLOSED; + assignPhase(VaultPhase.CLOSED); updateDebtSnapshot(AmountMath.makeEmpty(runBrand)); updateUiState();