Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: apparent error charging interest during liquidation #5689

Merged
merged 2 commits into from
Jul 1, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Jun 29, 2022

no ticket, discovered while working on #5558

Description

chargeAllVaults when done calls reschedulePriceCheck. If liquidation is in progress then the highestRatio() vault has no collateral allocated because it's in the seat for the liquidation trade. This was resulting in this error logged:

----- IV.2  5 updateUiState 1 {
  interestRate: {
    denominator: { brand: [Object [Alleged: RUN brand]], value: 10000n },
    numerator: { brand: [Object [Alleged: RUN brand]], value: 100n }
  },
  liquidationRatio: {
    denominator: { brand: [Object [Alleged: RUN brand]], value: 100n },
    numerator: { brand: [Object [Alleged: RUN brand]], value: 105n }
  },
  debtSnapshot: {
    debt: { brand: [Object [Alleged: RUN brand]], value: 389n },
    interest: { denominator: [Object], numerator: [Object] }
  },
  locked: { brand: Object [Alleged: aEth brand] {}, value: 400n },
  vaultState: 'liquidating'
}
----- VM.5  16 chargeAllVaults { updateTime: 4n }
----- VM.5  17 chargeAllVaults complete
🚨 vaultManager failed to charge interest (Error#1)
Error#1: First vault had no collateral
  at Alleged: PrioritizedVaults.highestRatio (.../run-protocol/src/vaultFactory/prioritizedVaults.js:88:1)
  at reschedulePriceCheck (.../run-protocol/src/vaultFactory/vaultManager.js:336:42)
  at Alleged: VaultManagerKit helper.method [as reschedulePriceCheck] (packages/SwingSet/src/liveslots/virtualObjectManager.js:478:43)
  at chargeAllVaults (.../run-protocol/src/vaultFactory/vaultManager.js:283:22)
  at Alleged: VaultManagerKit helper.method [as chargeAllVaults] (packages/SwingSet/src/liveslots/virtualObjectManager.js:478:43)
  at Object.updateState (.../run-protocol/src/vaultFactory/vaultManager.js:785:1)

This moves the query for highestRatio() to lower in reschedulePriceCheck so it's after the existing returns.

It also renames liquidationInProgress flag to distinguish from a particular vault being liquidated. The flag just means that the generator is being consumed.

Finally, it cleans up 633ee70 because that new name wasn't entirely accurate, and reduces IO by passing the new higher highest ratio in the callback.

Security Considerations

--

Documentation Considerations

--

Testing Considerations

No tests affected. Errors in chargeInterest are intentionally handled only as logging because we don't (yet) have a way to signal problems in recurring calls. Also this PR doesn't actually change the behavior.

Copy link
Member

@dtribble dtribble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment

@@ -183,9 +186,9 @@ export const makePrioritizedVaults = (highestChanged = () => {}) => {
entriesPrioritizedGTE,
getCount: vaults.getSize,
hasVaultByAttributes,
highestRatio,
highestRatio: firstDebtRatio,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been renamed?

`reschedulePriceCheck` is called after charging interest. If the first vault is being liquidated it will have no collateral and throw an error.
@turadg turadg force-pushed the ta/fix-pricecheck-liquidating branch from 9c3f086 to b347d5a Compare July 1, 2022 14:49
@turadg turadg force-pushed the ta/fix-pricecheck-liquidating branch from b347d5a to 3f13a4e Compare July 1, 2022 14:51
@turadg turadg added automerge:squash Automatically squash merge automerge:rebase Automatically rebase updates, then merge and removed automerge:squash Automatically squash merge labels Jul 1, 2022
@mergify mergify bot merged commit cafa284 into master Jul 1, 2022
@mergify mergify bot deleted the ta/fix-pricecheck-liquidating branch July 1, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants