feat(auction): Auction allows vaults to specify an amount to raise #7239

Chris-Hibbert · 2023-03-24T23:13:28Z

closes: #6952

Description

The Auctioneer allows providers of collateral to specify a target amount to raise. It's specified in an offerArg when exercising a deposit invitation as { toRaise: amount }. There's no guarantee that precisely this amount will be raised: there might not be enough bids, or others might have deposited different amounts with different payout ratios. But when it can, the auction will stop when it reaches that amount, and not sell more collateral past that point.

Reviewers might want to scan the tests to see how all the stats are updated.

Security Considerations

Nothing special.

Scaling Considerations

No change in concerns for auction or vaults.

Documentation Considerations

This is visible to the UI for depositors. It doesn't impact bidders in the auction.

Testing Considerations

Tests were added for all the major branches. It will be good to have people exercising the code and trying many variations.

codecov · 2023-03-24T23:17:50Z

Codecov Report

Merging #7239 (3f9059a) into master (5ba5f7b) will increase coverage by 0.05%.
The diff coverage is 79.31%.

❗ Current head 3f9059a differs from pull request most recent head 9bf5b13. Consider uploading reports for the commit 9bf5b13 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7239      +/-   ##
==========================================
+ Coverage   70.94%   70.99%   +0.05%     
==========================================
  Files         451      450       -1     
  Lines       86095    86359     +264     
  Branches        3        3              
==========================================
+ Hits        61081    61313     +232     
- Misses      24948    24978      +30     
- Partials       66       68       +2

Impacted Files	Coverage Δ
packages/inter-protocol/src/auction/auctionBook.js	`69.82% <37.62%> (-3.47%)`	⬇️
packages/inter-protocol/src/auction/auctioneer.js	`61.70% <94.56%> (+25.92%)`	⬆️

... and 3 files with indirect coverage changes

datadog-full-agoric · 2023-03-24T23:22:44Z

Datadog Report

Branch report: 6952-auctionTargeRaise
Commit report: b87f62f

✅ agoric-sdk: 0 Failed, 0 New Flaky, 3490 Passed, 47 Skipped, 24m 32.59s Wall Time

new tests.

turadg

Did a pass for readability. I'll take another review the distributeProportionalSharesWithLimits logic closely

packages/inter-protocol/src/auction/auctionBook.js

turadg · 2023-03-27T18:44:56Z

packages/inter-protocol/src/auction/auctionBook.js

         */
-        addAssets(assetAmount, sourceSeat) {
+        addAssets(assetAmount, sourceSeat, toRaise) {


"raise" is ambiguous between between "increase a quantity" (raise the bet) and "transact to reach a capital threshold" (raise $1m for charity).

Is there another term that's unambiguous? Maybe "collect". Also, I see the "total" in state goes down, which seems less like a total and more like "remaining".

There's also the matter of the optionality of the state. If it's not defined, it means "collect/raise as much as possible", yes?

Consider toCollect for the param and collectionLimit for the state. The limit goes up by toCollect and down with each sell.

I kind of like remainingCollectionTarget, though remainingTarget is a close second. I couldn't find a good replacement for "Collection", and I agree that "remaining" is critical. "target" seems important, to remind readers that it's only a goal.

I ended up with totalProceedsGoal, saving a few characters here and there, with losing clarity, I think.

packages/inter-protocol/src/auction/auctionBook.js

packages/inter-protocol/src/auction/auctioneer.js

packages/inter-protocol/src/vaultFactory/vaultManager.js

packages/inter-protocol/test/auction/test-auctionContract.js

test-proportionalDist covers all of distributeProportionalSharesWithLimits

Other miscellaneous minor renamings and refactorings.

turadg

looking good.

turadg · 2023-03-29T18:11:29Z

packages/inter-protocol/test/auction/test-proportionalDist.js

+    deposits.push({
+      seat,
+      amount: collateral.make(BigInt(rawDeposits[i].deposit)),
+      goal: collect ? currency.make(BigInt(collect)) : null,


collect was to match what I had proposed for the property of the deposit element type. To match the type now:

Suggested change

goal: collect ? currency.make(BigInt(collect)) : null,

goal: goal ? currency.make(BigInt(goal)) : null,

turadg · 2023-03-29T18:12:30Z

packages/inter-protocol/test/auction/test-proportionalDist.js

+);
+
+// rounding happens because the value of collateral is negligible.
+const transferSharing2000 = /** @type {Array<[bigint, bigint]>} */ ([


these one-off fixtures could be inlined. The reason I made the const was to convey that each of the cases using it has the same transfers.

turadg · 2023-03-29T18:15:16Z

packages/inter-protocol/test/auction/test-auctionContract.js

+  await driver.advanceTo(170n);
+  await eventLoopIteration();


I forget, if we make advanceTo also await the event loop something else breaks?

WDYT of having an option like…

Suggested change

await driver.advanceTo(170n);

await eventLoopIteration();

await driver.advanceTo(170n, 'andTurn');

incidental, out of scope

turadg · 2023-03-29T18:17:14Z

packages/inter-protocol/test/auction/test-auctionContract.js

@@ -175,7 +175,7 @@ const makeAuctionDriver = async (t, customTerms, params = defaultParams) => {
    return E(zoe).offer(bidInvitation, proposal, payment, harden(offerArgs));
  };

-  const depositCollateral = async (collateralAmount, issuerKit) => {
+  const depositCollateral = async (collateralAmount, issuerKit, limit) => {


limit name is confusing since it contains {goal}. I think this is also more correct,

Suggested change

const depositCollateral = async (collateralAmount, issuerKit, limit) => {

const depositCollateral = async (collateralAmount, issuerKit, offerArgs) => {

turadg · 2023-03-29T18:17:26Z

packages/inter-protocol/test/auction/test-auctionContract.js

+  /**
+   * @param {Pick<IssuerKit<'nat'>, 'brand' | 'issuer' | 'mint'>} issuerKit
+   * @param {Amount<'nat'>} collateralAmount
+   * @param {undefined | { goal: Amount<'nat'> }} limit


ditto re: name and our style is to express optionality with brackets (per JSDoc)

Suggested change

* @param {undefined | { goal: Amount<'nat'> }} limit

* @param {{ goal: Amount<'nat'> }} [offerArgs]

turadg · 2023-03-29T19:03:04Z

packages/inter-protocol/src/auction/auctionBook.js

+          const currencyLimit = totalProceedsGoal
+            ? AmountMath.min(totalProceedsGoal, initialCurrencyTarget)
+            : initialCurrencyTarget;
+          const isRaiseBounded =


"bounded" might not be worth a new term.

Suggested change

const isRaiseBounded =

const isRaiseLimited =

turadg · 2023-03-29T19:04:55Z

packages/inter-protocol/src/auction/auctionBook.js

+          if (proceedsGoal || totalProceedsGoal) {
+            this.state.totalProceedsGoal = ceilMultiplyBy(
+              AmountMath.add(curCollateral, assetAmount),
+              // @ts-expect-error calcTargetRatio always returns a ratio here


let's ensure that statically so this suppression isn't needed. I think a throw Fail at the end of calcTargetRatio would do it

turadg · 2023-03-29T19:07:53Z

packages/inter-protocol/src/auction/auctioneer.js

+      collateralValue,
+    );
+    // The share for those who specified a limit is proportional to their
+    // collateral. ceiling because it's a lower limit on the restrictive branch


thx for documenting why ceiling

turadg · 2023-03-29T19:13:34Z

packages/inter-protocol/src/vaultFactory/vaultManager.js

@@ -1071,6 +1037,14 @@ export const prepareVaultManagerKit = (
            /** @type {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart[]} */
            const transfers = [];
            let liquidated = 0;
+            /** @type {MapStore<string, Vault>} */
+            const vaultsToReinstate = makeScalarMapStore();


between here and when these entries are read, there can be failure that exits the function. That in-between should be in a try/catch so execution continues to the reinstating and publishing.

turadg · 2023-03-29T19:16:32Z

packages/inter-protocol/src/vaultFactory/vaultManager.js

@@ -862,11 +856,11 @@ export const prepareVaultManagerKit = (
            userSeatPromise,
          ]);

-          trace(`VM LiqV after long wait`, proceeds);
+          trace(`LiqV after long wait`, proceeds);


I notice that this function is async but the bulk of it is sync from here on. It would be easier to maintain and follow if here down was in its own sync function. e.g. helper.liquidateFromAuction.

not a blocker

turadg

Approving based on discussion about what's pending push

Chris-Hibbert added Inter-protocol Overarching Inter Protocol Vaults VaultFactor (née Treasury) labels Mar 24, 2023

Chris-Hibbert self-assigned this Mar 24, 2023

Chris-Hibbert requested a review from turadg March 24, 2023 23:13

feat(auction): Auction allows vaults to specify an amount to raise

7e6fb40

new tests.

Chris-Hibbert force-pushed the 6952-auctionTargeRaise branch from 767699e to 7e6fb40 Compare March 25, 2023 00:10

turadg reviewed Mar 27, 2023

View reviewed changes

turadg and others added 5 commits March 27, 2023 14:59

docs(auction): reorder distribution cases

eefe45f

test(auction): distributeProportionalSharesWithLimits

e105529

test: add tests showing that reserve gets funds from liquidation.

374362b

refactor: reduce redundancy in calculating next totalToRaise

689c9a6

refactor: improved distribution algo and tests based on coverage

dbc0fd4

test-proportionalDist covers all of distributeProportionalSharesWithLimits

Chris-Hibbert force-pushed the 6952-auctionTargeRaise branch from 17e5f41 to dbc0fd4 Compare March 28, 2023 20:59

Chris-Hibbert added 2 commits March 28, 2023 15:52

chore: rename toRaise to goal/proceedsGoal/TotalProceedsGoal

1b5f439

Other miscellaneous minor renamings and refactorings.

chore: renamings and whitespace

1b28b83

Chris-Hibbert requested a review from turadg March 28, 2023 22:58

turadg reviewed Mar 29, 2023

View reviewed changes

turadg approved these changes Mar 29, 2023

View reviewed changes

chore: renaming, extracting functions for readability, and cleanups

04d5b3c

Chris-Hibbert added the automerge:squash Automatically squash merge label Mar 29, 2023

turadg approved these changes Mar 29, 2023

View reviewed changes

fix: correct an obvious mistake. vaultData isn't in state

3f9059a

Chris-Hibbert force-pushed the 6952-auctionTargeRaise branch from 9eb4b93 to 3f9059a Compare March 30, 2023 00:30

Merge branch 'master' into 6952-auctionTargeRaise

9bf5b13

mergify bot merged commit bf35d83 into master Mar 30, 2023

mergify bot deleted the 6952-auctionTargeRaise branch March 30, 2023 01:47

Chris-Hibbert mentioned this pull request Apr 26, 2023

Updated Title: Too much excess collateral returned to vaults in Liquidation Flow 1 scenario #7346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auction): Auction allows vaults to specify an amount to raise #7239

feat(auction): Auction allows vaults to specify an amount to raise #7239

Chris-Hibbert commented Mar 24, 2023

codecov bot commented Mar 24, 2023 •

edited

Loading

datadog-full-agoric bot commented Mar 24, 2023 •

edited

Loading

turadg left a comment

turadg Mar 27, 2023

Chris-Hibbert Mar 27, 2023

Chris-Hibbert Mar 28, 2023

turadg left a comment

turadg Mar 29, 2023

turadg Mar 29, 2023

turadg Mar 29, 2023

turadg Mar 29, 2023

turadg Mar 29, 2023

turadg Mar 29, 2023

turadg Mar 29, 2023

turadg Mar 29, 2023

turadg Mar 29, 2023

turadg Mar 29, 2023

turadg left a comment

	goal: collect ? currency.make(BigInt(collect)) : null,
	goal: goal ? currency.make(BigInt(goal)) : null,

	await driver.advanceTo(170n);
	await eventLoopIteration();
	await driver.advanceTo(170n, 'andTurn');

	const depositCollateral = async (collateralAmount, issuerKit, limit) => {
	const depositCollateral = async (collateralAmount, issuerKit, offerArgs) => {

	* @param {undefined \| { goal: Amount<'nat'> }} limit
	* @param {{ goal: Amount<'nat'> }} [offerArgs]

feat(auction): Auction allows vaults to specify an amount to raise #7239

feat(auction): Auction allows vaults to specify an amount to raise #7239

Conversation

Chris-Hibbert commented Mar 24, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

codecov bot commented Mar 24, 2023 • edited Loading

Codecov Report

datadog-full-agoric bot commented Mar 24, 2023 • edited Loading

Datadog Report

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 24, 2023 •

edited

Loading

datadog-full-agoric bot commented Mar 24, 2023 •

edited

Loading