feat(run-protocol): distinguish LIQUIDATED from CLOSED vault state #4640
Conversation
| // TODO state distinct from CLOSED https://github.com/Agoric/agoric-sdk/issues/4539 | ||
| liquidated: vaultState === VaultState.CLOSED, | ||
| vaultState: vstate, | ||
| liquidated: phase === VaultPhase.LIQUIDATED, |
There was a problem hiding this comment.
I suspect the UI actually wants to know "is the vault closed or liquidated?". "closed" is a better term for that than "liquidated", but maybe the UI needs to distinguish [ [Active], [Closed, Liquidated], [Transferring]]. I think Liquidating is most like Closed and Liquidating among those, but we should probably check with @samsiegart.
There was a problem hiding this comment.
I'm assuming the end user would want to know all of these states. I'd really like to remove this flag entirely since it's redundant with the richer option, but I was trying to maintain backwards compatibility for now. To that end, I suppose CLOSED achieves that more!
@samsiegart is this backwards compatibility necessary or can we drop the liquidated flag in this PR? If not now then when would be good?
There was a problem hiding this comment.
from convo today, decision was to drop this boolean and rely just on the phase/state value 797a9cf
| case VaultState.LIQUIDATING: | ||
| switch (vaultPhase) { | ||
| case VaultPhase.ACTIVE: | ||
| case VaultPhase.LIQUIDATING: |
There was a problem hiding this comment.
add TRANSFERABLE here.
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 finish event that is generated when the transfer is started
There was a problem hiding this comment.
This is a smell though that the transfer case is so special. Refactored in 29b997
| * @typedef {'active' | 'liquidating' | 'closed' | 'transfer'} VAULT_STATE | ||
| * 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 | ||
| * TRANSFERABLE - vault is released from the manager and able to be transferred |
There was a problem hiding this comment.
What does "released from the manager" mean?
There was a problem hiding this comment.
I like "transfering" better than "tranferable" I think
There was a problem hiding this comment.
"released from manager" was when I thought the vault freezes for transfer and can't be liquidated, but that's wrong so I'll fix this. What's the best way to describe this? "Able to be transferred (payments and debits frozen until it has a new owner)" ?
Regarding the term, "transferable" feels clunky but "transfering" isn't necessarily true, right? It could sit inert in a transferable state indefinitely. How about SEALED? It can be sealed for transfer and unsealed upon reception.
There was a problem hiding this comment.
back to TRANSFER but I don't like that it's a verb and the rest are adjectives 792c388
| /** | ||
| * @param {VAULT_PHASE} newPhase | ||
| */ | ||
| const assignPhase = newPhase => { |
There was a problem hiding this comment.
If we want a state machine...then we have a library for it, and can potentially integrate that with the enum proposal
There was a problem hiding this comment.
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 enum proposal and consider refactoring this in the future to use them.
There was a problem hiding this comment.
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.
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 stateMachine assumes the state is in its own closure and it's got to be in the state object.
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:
state.phase = machine.transition(state.phase, 'liquidate')
However I think that's out of scope of this PR and the assignState method closes the gap to a full machine if that proves worthwhile.
| @@ -258,7 +290,7 @@ export const makeInnerVault = ( | |||
| return makeRatioFromAmounts(collateralValueInRun, getDebtAmount()); | |||
| }; | |||
|
|
|||
| const snapshotState = (vstate, collateralizationRatio) => { | |||
| const snapshotState = (phase, collateralizationRatio) => { | |||
There was a problem hiding this comment.
not sure I like "phase" when it's really the state for a state machine
There was a problem hiding this comment.
The motivation is for when we're on virtual objects. Writing state.state = VaultState.ACTIVE was worse :)
| // TODO state distinct from CLOSED https://github.com/Agoric/agoric-sdk/issues/4539 | ||
| liquidated: vaultState === VaultState.CLOSED, | ||
| vaultState: vstate, | ||
| liquidated: phase === VaultPhase.LIQUIDATED, |
There was a problem hiding this comment.
this should be named "isClosed"
| @@ -285,26 +316,27 @@ export const makeInnerVault = ( | |||
| // [https://github.com/Agoric/dapp-token-economy/issues/123] | |||
| const collateralizationRatio = await getCollateralizationRatio(); | |||
| /** @type {VaultUIState} */ | |||
| const uiState = snapshotState(vaultState, collateralizationRatio); | |||
| const uiState = snapshotState(vaultPhase, collateralizationRatio); | |||
There was a problem hiding this comment.
I'd use "phase" rather than "vaultPhase" if we are using it
| case VaultState.LIQUIDATING: | ||
| switch (vaultPhase) { | ||
| case VaultPhase.ACTIVE: | ||
| case VaultPhase.LIQUIDATING: |
There was a problem hiding this comment.
There are tests for transferring. "Transfer" is only visible in finish event that is generated when the transfer is started
turadg
force-pushed
the
ta/4539-liquidated-vs-closed
branch
from
797a9cf
to
3b4843e
Compare
turadg
force-pushed
the
ta/4539-liquidated-vs-closed
branch
2 times, most recently
from
5a0ed49
to
546f638
Compare
| @@ -1657,15 +1657,15 @@ 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, 'liquidated'); | |||
There was a problem hiding this comment.
| t.is(aliceUpdate.value.vaultState, 'liquidated'); | |
| t.is(aliceUpdate.value.vaultState, VaultPhase.LIQUIDATED); |
There was a problem hiding this comment.
Agree it's better to use a constant instead of the literal. I'll define the constant in this test so that if the actual string VaultFactory returns changes this test will break.
|
|
||
| for (let i = 0; i < 5; i += 1) { | ||
| manualTimer.tick(); | ||
| } | ||
| await waitForPromisesToSettle(); | ||
| bobUpdate = await E(bobNotifier).getUpdateSince(); | ||
|
|
||
| t.truthy(bobUpdate.value.liquidated); | ||
| t.is(bobUpdate.value.vaultState, 'liquidated'); |
There was a problem hiding this comment.
| t.is(bobUpdate.value.vaultState, 'liquidated'); | |
| t.is(bobUpdate.value.vaultState, VaultPhase.LIQUIDATED); |
| @@ -2159,13 +2159,13 @@ 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, 'active'); | |||
There was a problem hiding this comment.
| t.is(aliceUpdate.value.vaultState, 'active'); | |
| t.is(aliceUpdate.value.vaultState, VaultPhase.ACTIVE); |
|
|
||
| for (let i = 0; i < 5; i += 1) { | ||
| manualTimer.tick(); | ||
| } | ||
| await waitForPromisesToSettle(); | ||
| bobUpdate = await E(bobNotifier).getUpdateSince(); | ||
|
|
||
| t.truthy(bobUpdate.value.liquidated); | ||
| t.is(bobUpdate.value.vaultState, 'liquidated'); |
There was a problem hiding this comment.
| t.is(bobUpdate.value.vaultState, 'liquidated'); | |
| t.is(bobUpdate.value.vaultState, VaultPhase.LIQUIDATED); |
|
|
||
| const debtAmountAfter = await E(vault).getDebtAmount(); | ||
| const finalNotification = await E(uiNotifier).getUpdateSince(); | ||
| t.truthy(finalNotification.value.liquidated); | ||
| t.is(finalNotification.value.vaultState, 'liquidated'); |
There was a problem hiding this comment.
| t.is(finalNotification.value.vaultState, 'liquidated'); | |
| t.is(finalNotification.value.vaultState, VaultPhase.LIQUIDATED); |
|
|
||
| await manualTimer.tick(); | ||
| notification = await E(uiNotifier).getUpdateSince(notification.updateCount); | ||
| t.falsy(notification.updateCount); | ||
| t.truthy(notification.value.liquidated); | ||
| t.is(notification.value.vaultState, 'liquidated'); |
There was a problem hiding this comment.
| t.is(notification.value.vaultState, 'liquidated'); | |
| t.is(notification.value.vaultState, VaultPhase.LIQUIDATED); |
|
|
||
| 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, VaultState.LIQUIDATING); | ||
| t.is(notification.value.vaultState, 'liquidating'); |
There was a problem hiding this comment.
| t.is(notification.value.vaultState, 'liquidating'); | |
| t.is(notification.value.vaultState, VaultPhase.LIQUIDATING); |
| @@ -519,22 +519,22 @@ test('price drop', async t => { | |||
| ); | |||
| await manualTimer.tick(); | |||
| notification = await E(uiNotifier).getUpdateSince(); | |||
| t.falsy(notification.value.liquidated); | |||
| t.is(notification.value.vaultState, 'active'); | |||
There was a problem hiding this comment.
| t.is(notification.value.vaultState, 'active'); | |
| t.is(notification.value.vaultState, VaultPhase.ACTIVE); |
| @@ -508,7 +508,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, 'active'); | |||
There was a problem hiding this comment.
| t.is(notification.value.vaultState, 'active'); | |
| t.is(notification.value.vaultState, VaultPhase.ACTIVE); |
turadg
force-pushed
the
ta/4539-liquidated-vs-closed
branch
from
546f638
to
7fdf359
Compare
turadg
added
the
automerge:no-update
turadg
force-pushed
the
ta/4539-liquidated-vs-closed
branch
from
918964f
to
59ee9b0
Compare
closes: #4539
Description
Security Considerations
--
Documentation Considerations
--
Testing Considerations
Tests updated.