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

feat(vaults)!: refactor Vaults to use an Auction for Liquidation #7047

Merged
merged 12 commits into from
Mar 17, 2023

Conversation

Chris-Hibbert
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert commented Feb 22, 2023

closes: #6692
refs: #6641
fixes: #6951

Description

Refactor Vaults to use an Auction for liquidation in place of the previous reliance on the AMM.

The AMM and Reserve are tangled in bootstrap, and this PR doesn't succeed in untangling them completely. Some tests fail at this time, but will be repaired before it is merged.

There are two commits here. Please focus on the first, which is the meat of the refactoring. The second is the beginning of an attempt to clean up the entanglement with the AMM. We may end up removing the AMM entirely as the simplest way to clean up the loose ends.

Security Considerations

The concerns here are mostly economic, and not much related to software security.

Scaling Considerations

There may be many users of the Auction, though presumably significantly fewer than of the Vaults.

Documentation Considerations

Documentation of the AMM for end users and high-investment users will be required.

Testing Considerations

Tests show that the normal paths through vault liquidation work correctly. There many not be enough testing of edge cases.

@Chris-Hibbert Chris-Hibbert added Inter-protocol Overarching Inter Protocol AMM Vaults VaultFactor (née Treasury) MUST-HAVE labels Feb 22, 2023
@Chris-Hibbert Chris-Hibbert self-assigned this Feb 22, 2023
@Chris-Hibbert Chris-Hibbert marked this pull request as draft February 22, 2023 18:20
packages/inter-protocol/src/proposals/startPSM.js Outdated Show resolved Hide resolved
* @returns {string} lexically sortable string in which highest
* debt-to-collateral is earliest
*/
const toCollateralizationRatioKey = (normalizedDebt, collateral) => {
Copy link
Member

Choose a reason for hiding this comment

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

this is the same as toVaultKey but without the vaultId differentiator. I don't see it used anywhere except tests. What's the purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The vault keys have two parts, the collateralizationRatio and a key to uniquify vaults. When retrieving values from a store using M.lte() or M.gte(), the entire key is compared.

If I want everything above some collateralization ratio, I don't want to include a specific key or any vault with that exact CR, but a smaller key would be missed. So the right query is one that doesn't specifies the vaultId. normalizedCRKey() does that, but it takes a normalized CR.

Creating a normalized ratio would have taken more work than I wanted, so I used this function to create keys with just the CR part. I've now removed this function and had the tests use a key that is just 'id.'.

packages/inter-protocol/src/vaultFactory/storeUtils.js Outdated Show resolved Hide resolved
packages/inter-protocol/src/vaultFactory/vaultDirector.js Outdated Show resolved Hide resolved
packages/inter-protocol/src/vaultFactory/vaultManager.js Outdated Show resolved Hide resolved
packages/inter-protocol/src/vaultFactory/vaultManager.js Outdated Show resolved Hide resolved
packages/inter-protocol/src/vaultFactory/vaultManager.js Outdated Show resolved Hide resolved
@ivanlei ivanlei requested a review from dtribble February 22, 2023 20:49
Chris-Hibbert added a commit that referenced this pull request Feb 24, 2023
The most visible consequence is to vault liquidation. Some pieces are
left hanging, and will be repaired by #7047.
Chris-Hibbert added a commit that referenced this pull request Feb 24, 2023
The most visible consequence is to vault liquidation. Some pieces are
left hanging, and will be repaired by #7047.
@Chris-Hibbert Chris-Hibbert force-pushed the 6641-auctioneer branch 3 times, most recently from 6e6baea to d45a03d Compare February 28, 2023 20:29
Chris-Hibbert added a commit that referenced this pull request Feb 28, 2023
The most visible consequence is to vault liquidation. Some pieces are
left hanging, and will be repaired by #7047.
Chris-Hibbert added a commit that referenced this pull request Feb 28, 2023
It will be introduced with #7047 when vaultfactory makes use of the
auction for liquidation
mergify bot pushed a commit that referenced this pull request Mar 1, 2023
…7074)

* feat(AMM)!:  remove the AMM and cleanup bootstrap etc. dependencies

The most visible consequence is to vault liquidation. Some pieces are
left hanging, and will be repaired by #7047.

* chore: first follow-ups to initial comments

Remove a few more things, mostly doc comments.

* chore: clean-ups and todos to mark unfinished transition steps

* chore: use unknown rather than any in interim liquidation contracts

* chore: remove more stuff related to AMM

remove interchainPool and other bootstrap stuff connected to AMM
Simplify liquidation contracts so they don't refer to dead code.

other clean ups

* chore: lint cleanups and revert retainedCollateral

* test: update integration test to use faucet instead of amm & vault
Chris-Hibbert added a commit that referenced this pull request Mar 2, 2023
It will be introduced with #7047 when vaultfactory makes use of the
auction for liquidation
Chris-Hibbert added a commit that referenced this pull request Mar 5, 2023
It will be introduced with #7047 when vaultfactory makes use of the
auction for liquidation
Base automatically changed from 6992-auctioneer to master March 6, 2023 01:24
@Chris-Hibbert Chris-Hibbert removed the automerge:rebase Automatically rebase updates, then merge label Mar 16, 2023
@turadg turadg added automerge:no-update (expert!) Automatically merge without updates and removed automerge:no-update (expert!) Automatically merge without updates labels Mar 16, 2023
@dckc
Copy link
Member

dckc commented Mar 16, 2023

E(auctioneerCreator).addBrand() missing

While working on the CLI (#7132) in a running chain, I got result: "Your offer was refused" from all my bid offers. Looks like this is missing:

@@ -204,7 +204,7 @@ export const registerScaledPriceAuthority = async (
  */
 export const addAssetToVault = async (
   {
-    consume: { vaultFactoryKit, agoricNamesAdmin },
+    consume: { vaultFactoryKit, agoricNamesAdmin, auctioneerKit },
     brand: {
       consume: { [Stable.symbol]: stableP },
     },
@@ -239,6 +239,8 @@ export const addAssetToVault = async (
     liquidationPenalty: makeRatio(1n, stable),
     interestRate: makeRatio(1n, stable),
   });
+  const auctioneerCreator = E.get(auctioneerKit).creatorFacet;
+  await E(auctioneerCreator).addBrand(interchainIssuer, keyword);
 };
@@ -288,7 +290,8 @@ export const getManifestForAddAssetToVault = (
       },
       [addAssetToVault.name]: {
         consume: {
-          vaultFactoryKit: true,
+          auctioneerKit: 'auctioneer',
+          vaultFactoryKit: 'vaultFactory',
           agoricNamesAdmin: true,
         },
         brand: {

@dckc
Copy link
Member

dckc commented Mar 16, 2023

scaledBidBook not durable?

2023-03-16T16:39:35.036Z SwingSet: ls: v39: Error#1: value is not durable: Object [Alleged: scaledBidBook ] { add: [Function: add], offersAbove: [Function: offersAbove], hasOrders: [Function: hasOrders], delete: [Function: delete], updateReceived: [Function: updateReceived], exitAllSeats: [Function: exitAllSeats] } at slot 0 of #"$0.Alleged: scaledBidBook "
2023-03-16T16:39:35.036Z SwingSet: ls: v39: Error: value is not durable: (an object) at slot 0 of (a string)
 at construct ()
 at Error (/bundled-source/.../node_modules/ses/src/error/tame-error-constructor.js:56)
 at makeError (/bundled-source/.../node_modules/ses/src/error/assert.js:242)
 at fail (/bundled-source/.../node_modules/ses/src/error/assert.js:370)
 at throwNotDurable (/bundled-source/.../packages/swingset-liveslots/src/collectionManager.js:47)
 at (/bundled-source/.../packages/swingset-liveslots/src/collectionManager.js:327)
 at forEach ()
 at init (/bundled-source/.../packages/swingset-liveslots/src/collectionManager.js:325)
 at provideLazy (.../store/src/stores/store-utils.js:98)
 at makeAuctionBook (.../inter-protocol/src/auction/auctionBook.js:130)
 at ()
 at ()

@@ -175,7 +193,12 @@ export const makeAuctionBook = async (
}
};

// Settle with seat. The caller is responsible for updating the book, if any.
/**
*e Settle with seat. The caller is responsible for updating the book, if any.
Copy link
Member

Choose a reason for hiding this comment

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

stray e

@@ -262,7 +276,12 @@ export const start = async (zcf, privateArgs, baggage) => {

const publicFacet = augmentPublicFacet(
harden({
/** @param {Brand<'nat'>} collateralBrand */
getBidInvitation(collateralBrand) {
Copy link
Member

Choose a reason for hiding this comment

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

should be makeBidInvitation, right? it returns a fresh Invitation each time.

Also: mustMatch(collateralBrand, BrandShape) seems in order.

getBidInvitation(collateralBrand) {
/**
* @param {ZCFSeat} zcfSeat
* @param {import('./auctionBook.js').BidSpec} bidSpec
Copy link
Member

Choose a reason for hiding this comment

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

bidSpec comes from outside the contract (i.e. from a presumed attacker), and bidProposalShape doesn't guarantee that bidSpec is of type BidSpec. It only checks {}, whatever that means.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

good stuff.

I'm still a little nervous about module level state for getting the next seqNum. Here's hoping for time to think about that a bit more.

@@ -912,3 +915,16 @@ test('deposit unregistered collateral', async t => {
message: /no ordinal/,
});
});

test('bid on unregistered collateral', async t => {
Copy link
Member

Choose a reason for hiding this comment

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

👏

arirubinstein and others added 10 commits March 16, 2023 13:32
All tests pass with minor modifications.

split test-vaultFactory. liquidation-related tests go to
test-vaultLiquidation.js, others remain.

chore: lint cleanups and other minor issues

refactor: revise liquidation payout approach to match #7123

chore: changes requested in review

chore: permit auctioneerKit for startVaultFactory

chore: more changes suggested in review

fixup: remove getDepositInvitation from creatorFacet
chore(types): fixup auction types
@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Mar 16, 2023
@mergify mergify bot merged commit a2fa2fb into master Mar 17, 2023
@mergify mergify bot deleted the 6641-auctioneer branch March 17, 2023 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMM automerge:no-update (expert!) Automatically merge without updates Inter-protocol Overarching Inter Protocol Vaults VaultFactor (née Treasury)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impose liquidation penalty on liquidated vaults intermittent CI failure in vat-admin terminate test
4 participants