feat(run-protocol): distinguish LIQUIDATED from CLOSED vault state #4640
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,17 +28,38 @@ const trace = makeTracer('Vault'); | |
| // collateral, and lending RUN to the borrower | ||
|
|
||
| /** | ||
| * 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 | ||
| * 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 | ||
| * | ||
| * @typedef {VaultPhase[keyof typeof VaultPhase]} VAULT_PHASE | ||
| */ | ||
| export const VaultPhase = /** @type {const} */ ({ | ||
| ACTIVE: 'active', | ||
| LIQUIDATING: 'liquidating', | ||
| CLOSED: 'closed', | ||
| LIQUIDATED: 'liquidated', | ||
| TRANSFER: 'transfer', | ||
| }); | ||
|
|
||
| /** | ||
| * @type {{[K in VAULT_PHASE]: Array<VAULT_PHASE>}} | ||
| */ | ||
| 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 => { | ||
|
|
@@ -102,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 = () => { | ||
| assertPhase(VaultPhase.ACTIVE); | ||
| }; | ||
| // #endregion | ||
|
|
||
| let outerUpdater; | ||
|
|
||
|
|
@@ -239,7 +279,7 @@ export const makeInnerVault = ( | |
| : getCollateralAllocated(vaultSeat); | ||
| }; | ||
|
|
||
| const snapshotState = newPhase => { | ||
| /** @type {VaultUIState} */ | ||
| return harden({ | ||
| // TODO move manager state to a separate notifer https://github.com/Agoric/agoric-sdk/issues/4540 | ||
|
|
@@ -248,38 +288,44 @@ 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, | ||
| }); | ||
| }; | ||
|
|
||
| // call this whenever anything changes! | ||
| const updateUiState = () => { | ||
| if (!outerUpdater) { | ||
| console.warn('updateUiState called after outerUpdater removed'); | ||
| return; | ||
| } | ||
| /** @type {VaultUIState} */ | ||
| const uiState = snapshotState(phase); | ||
| trace('updateUiState', uiState); | ||
|
|
||
| switch (phase) { | ||
| case VaultPhase.ACTIVE: | ||
| case VaultPhase.LIQUIDATING: | ||
|
There was a problem hiding this comment. add If no tests broke, then there aren't any tests for transfering. Please add some tests in that case. There was a problem hiding this comment. There are tests for transferring. "Transfer" is only visible in There was a problem hiding this comment. This is a smell though that the transfer case is so special. Refactored in 29b997 |
||
| outerUpdater.updateState(uiState); | ||
| break; | ||
| case VaultPhase.CLOSED: | ||
| case VaultPhase.LIQUIDATED: | ||
| 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 vault phase: ${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 (phase !== VaultPhase.CLOSED) { | ||
| updateUiState(); | ||
| } | ||
| }, | ||
|
|
@@ -293,15 +339,12 @@ export const makeInnerVault = ( | |
| const liquidated = newDebt => { | ||
| updateDebtSnapshot(newDebt); | ||
|
|
||
| assignPhase(VaultPhase.LIQUIDATED); | ||
| updateUiState(); | ||
| }; | ||
|
|
||
| const liquidating = () => { | ||
| assignPhase(VaultPhase.LIQUIDATING); | ||
| updateUiState(); | ||
| }; | ||
|
|
||
|
|
@@ -342,7 +385,7 @@ export const makeInnerVault = ( | |
| runMint.burnLosses(harden({ RUN: currentDebt }), burnSeat); | ||
| seat.exit(); | ||
| burnSeat.exit(); | ||
| assignPhase(VaultPhase.CLOSED); | ||
| updateDebtSnapshot(AmountMath.makeEmpty(runBrand)); | ||
| updateUiState(); | ||
|
|
||
|
|
@@ -670,7 +713,7 @@ export const makeInnerVault = ( | |
| makeCloseInvitation, | ||
| makeTransferInvitation: () => { | ||
| if (outerUpdater) { | ||
| outerUpdater.finish(snapshotState(VaultPhase.TRANSFER)); | ||
| outerUpdater = null; | ||
| } | ||
| return zcf.makeInvitation(makeTransferInvitationHook, 'TransferVault'); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want a state machine...then we have a library for it, and can potentially integrate that with the
enumproposalThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I was considering a fuller state machine with transition functions and figured this was a good incremental step that wasn't entirely out of scope for the ticket :)
I'd like to hear more about the library we have and this
enumproposal and consider refactoring this in the future to use them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not much:
.../zoe/src/contractSupport/stateMachine.js. It started approximately like this. Just enough for present purposes, with a hope for more when we need it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link. I don't think that version is going to work well for state that need to be serialized into virtual objects because the
stateMachineassumes the state is in its own closure and it's got to be in thestateobject.We could isolate the actual state from the transition mechanics with a design like https://kentcdodds.com/blog/implementing-a-simple-state-machine-library-in-javascript that would allow:
However I think that's out of scope of this PR and the
assignStatemethod closes the gap to a full machine if that proves worthwhile.