From fad96bbbcb1bd6fb47170f496219d123cc9c0799 Mon Sep 17 00:00:00 2001 From: Chris Hibbert Date: Fri, 1 Sep 2023 17:26:33 -0700 Subject: [PATCH 1/5] fix: Make the auction schedule robust when adding collateral types fixes: 8296 --- .../inter-protocol/src/auction/auctionBook.js | 9 +- .../inter-protocol/src/auction/scheduler.js | 7 +- .../test/auction/test-auctionContract.js | 90 +++++++++++++++++++ 3 files changed, 102 insertions(+), 4 deletions(-) diff --git a/packages/inter-protocol/src/auction/auctionBook.js b/packages/inter-protocol/src/auction/auctionBook.js index 548be5f0943..17ce9be001e 100644 --- a/packages/inter-protocol/src/auction/auctionBook.js +++ b/packages/inter-protocol/src/auction/auctionBook.js @@ -555,9 +555,12 @@ export const prepareAuctionBook = (baggage, zcf, makeRecorderKit) => { trace(this.state.collateralBrand, 'settleAtNewRate', reduction); const { capturedPriceForRound, priceBook, scaledBidBook } = state; - capturedPriceForRound !== null || - Fail`price must be captured before auction starts`; - assert(capturedPriceForRound); + if (!capturedPriceForRound) { + console.warn( + `No price for ${this.state.collateralBrand}, skipping auction.`, + ); + return; + } state.curAuctionPrice = multiplyRatios( reduction, diff --git a/packages/inter-protocol/src/auction/scheduler.js b/packages/inter-protocol/src/auction/scheduler.js index c1d05b18b5f..6d418247da5 100644 --- a/packages/inter-protocol/src/auction/scheduler.js +++ b/packages/inter-protocol/src/auction/scheduler.js @@ -323,7 +323,12 @@ export const makeScheduler = async ( async updateState(_newState) { trace('received param update', _newState); await null; - if (!nextSchedule) { + + now = await E(timer).getCurrentTimestamp(); + if ( + !nextSchedule || + TimeMath.compareAbs(nextSchedule.startTime, now) < 0 + ) { trace('repairing nextSchedule and restarting'); ({ nextSchedule } = await initializeNextSchedule()); startSchedulingFromScratch(); diff --git a/packages/inter-protocol/test/auction/test-auctionContract.js b/packages/inter-protocol/test/auction/test-auctionContract.js index 16960075028..053786fa8bb 100644 --- a/packages/inter-protocol/test/auction/test-auctionContract.js +++ b/packages/inter-protocol/test/auction/test-auctionContract.js @@ -18,6 +18,7 @@ import { assertPayoutAmount } from '@agoric/zoe/test/zoeTestHelpers.js'; import { makeManualPriceAuthority } from '@agoric/zoe/tools/manualPriceAuthority.js'; import { providePriceAuthorityRegistry } from '@agoric/zoe/tools/priceAuthorityRegistry.js'; import { E } from '@endo/eventual-send'; +import { NonNullish } from '@agoric/assert'; import { setupReserve, @@ -110,6 +111,11 @@ export const setupServices = async (t, params = defaultParams) => { const space = await setupBootstrap(t, timer); installPuppetGovernance(zoe, space.installation.produce); + t.context.puppetGovernors = { + auctioneer: E.get(space.consume.auctioneerKit).governorCreatorFacet, + vaultFactory: E.get(space.consume.vaultFactoryKit).governorCreatorFacet, + }; + // @ts-expect-error not all installs are needed for auctioneer. produceInstallations(space, t.context.installs); @@ -309,6 +315,15 @@ const makeAuctionDriver = async (t, params = defaultParams) => { await eventLoopIteration(); } }, + + setGovernedParam: (name, newValue) => { + trace(t, 'setGovernedParam', name); + const auctionGov = NonNullish(t.context.puppetGovernors.auctioneer); + return E(auctionGov).changeParams( + harden({ changes: { [name]: newValue } }), + ); + }, + async updatePriceAuthority(newPrice) { priceAuthorities.get(newPrice.denominator.brand).setPrice(newPrice); await eventLoopIteration(); @@ -1408,3 +1423,78 @@ test.serial('time jumps forward', async t => { t.is(schedules.liveAuctionSchedule?.startTime.absValue, 1530n); t.is(schedules.liveAuctionSchedule?.endTime.absValue, 1550n); }); + +// serial because dynamicConfig is shared across tests +// This test reproduced bug #8296. Now its expectations don't match the behavior +test.serial.skip('add collateral type during auction', async t => { + const { collateral, bid } = t.context; + const driver = await makeAuctionDriver(t); + const asset = withAmountUtils(makeIssuerKit('Asset')); + const timerBrand = await E(driver.getTimerService()).getTimerBrand(); + + const liqSeat = await driver.setupCollateralAuction( + collateral, + collateral.make(1000n), + ); + await driver.updatePriceAuthority( + makeRatioFromAmounts(bid.make(11n), collateral.make(10n)), + ); + + const result = await E(liqSeat).getOfferResult(); + t.is(result, 'deposited'); + + await driver.advanceTo(167n); + const seat = await driver.bidForCollateralSeat( + // 1.1 * 1.05 * 200 + bid.make(231n), + collateral.make(200n), + ); + t.is(await E(seat).getOfferResult(), 'Your bid has been accepted'); + t.false(await E(seat).hasExited()); + + const scheduleTracker = await driver.getScheduleTracker(); + await scheduleTracker.assertInitial({ + activeStartTime: TimeMath.coerceTimestampRecord(170n, timerBrand), + nextDescendingStepTime: TimeMath.coerceTimestampRecord(170n, timerBrand), + nextStartTime: TimeMath.coerceTimestampRecord(210n, timerBrand), + }); + + await driver.advanceTo(170n, 'wait'); + await scheduleTracker.assertChange({ + nextDescendingStepTime: { absValue: 175n }, + }); + + await driver.advanceTo(175n, 'wait'); + await scheduleTracker.assertChange({ + nextDescendingStepTime: { absValue: 180n }, + }); + + // before the fix for #8296 in AuctionBook, this broke the ongoing auction. + await driver.setupCollateralAuction(asset, asset.make(500n)); + + await driver.advanceTo(180n, 'wait'); + await driver.advanceTo(185n, 'wait'); + t.true(await E(seat).hasExited()); + + await assertPayouts(t, seat, bid, collateral, 0n, 200n); + t.false(await E(liqSeat).hasExited()); + await E(liqSeat).tryExit(); + + await assertPayouts(t, liqSeat, bid, collateral, 0n, 0n); + + await driver.advanceTo(210n, 'wait'); + await driver.advanceTo(220n, 'wait'); + await driver.advanceTo(250n, 'wait'); + + // before the fix for #8296 in scheduler, this didn't repair the problem. + driver.setGovernedParam('DiscountStep', 2000n); + + await driver.advanceTo(300n, 'wait'); + + await scheduleTracker.assertChange({ + nextDescendingStepTime: { absValue: 330n }, + nextStartTime: { absValue: 330n }, + }); + + await driver.advanceTo(330n, 'wait'); +}); From 59a537005faa09655d555074d6a3e72bd40fe5db Mon Sep 17 00:00:00 2001 From: Chris Hibbert Date: Wed, 6 Sep 2023 10:08:30 -0700 Subject: [PATCH 2/5] refactor: clean up timestamp await, and add test. --- .../inter-protocol/src/auction/auctioneer.js | 1 - .../inter-protocol/src/auction/scheduler.js | 15 ++- .../test/auction/test-auctionContract.js | 109 +++++++++++++++--- 3 files changed, 102 insertions(+), 23 deletions(-) diff --git a/packages/inter-protocol/src/auction/auctioneer.js b/packages/inter-protocol/src/auction/auctioneer.js index 7a705398dc3..f2592ad7454 100644 --- a/packages/inter-protocol/src/auction/auctioneer.js +++ b/packages/inter-protocol/src/auction/auctioneer.js @@ -679,7 +679,6 @@ export const start = async (zcf, privateArgs, baggage) => { makeScheduler( driver, timer, - // @ts-expect-error types are correct. How to convince TS? params, timerBrand, scheduleRecorder, diff --git a/packages/inter-protocol/src/auction/scheduler.js b/packages/inter-protocol/src/auction/scheduler.js index 6d418247da5..d1b5c1466bf 100644 --- a/packages/inter-protocol/src/auction/scheduler.js +++ b/packages/inter-protocol/src/auction/scheduler.js @@ -324,11 +324,16 @@ export const makeScheduler = async ( trace('received param update', _newState); await null; - now = await E(timer).getCurrentTimestamp(); - if ( - !nextSchedule || - TimeMath.compareAbs(nextSchedule.startTime, now) < 0 - ) { + let fixableSchedule; + if (!nextSchedule) { + fixableSchedule = true; + } else { + now = await E(timer).getCurrentTimestamp(); + fixableSchedule = + TimeMath.compareAbs(nextSchedule.startTime, now) < 0; + } + + if (fixableSchedule) { trace('repairing nextSchedule and restarting'); ({ nextSchedule } = await initializeNextSchedule()); startSchedulingFromScratch(); diff --git a/packages/inter-protocol/test/auction/test-auctionContract.js b/packages/inter-protocol/test/auction/test-auctionContract.js index 053786fa8bb..d272aab1a7e 100644 --- a/packages/inter-protocol/test/auction/test-auctionContract.js +++ b/packages/inter-protocol/test/auction/test-auctionContract.js @@ -1425,8 +1425,7 @@ test.serial('time jumps forward', async t => { }); // serial because dynamicConfig is shared across tests -// This test reproduced bug #8296. Now its expectations don't match the behavior -test.serial.skip('add collateral type during auction', async t => { +test.serial('add collateral type during auction', async t => { const { collateral, bid } = t.context; const driver = await makeAuctionDriver(t); const asset = withAmountUtils(makeIssuerKit('Asset')); @@ -1473,28 +1472,104 @@ test.serial.skip('add collateral type during auction', async t => { await driver.setupCollateralAuction(asset, asset.make(500n)); await driver.advanceTo(180n, 'wait'); + await scheduleTracker.assertChange({ + nextDescendingStepTime: { absValue: 185n }, + }); + await driver.advanceTo(185n, 'wait'); - t.true(await E(seat).hasExited()); - await assertPayouts(t, seat, bid, collateral, 0n, 200n); - t.false(await E(liqSeat).hasExited()); - await E(liqSeat).tryExit(); + await scheduleTracker.assertChange({ + nextDescendingStepTime: { absValue: 190n }, + }); - await assertPayouts(t, liqSeat, bid, collateral, 0n, 0n); + t.true(await E(seat).hasExited()); + await assertPayouts(t, seat, bid, collateral, 0n, 200n); await driver.advanceTo(210n, 'wait'); - await driver.advanceTo(220n, 'wait'); - await driver.advanceTo(250n, 'wait'); - // before the fix for #8296 in scheduler, this didn't repair the problem. - driver.setGovernedParam('DiscountStep', 2000n); - - await driver.advanceTo(300n, 'wait'); + t.true(await E(liqSeat).hasExited()); + await assertPayouts(t, liqSeat, bid, collateral, 231n, 800n); await scheduleTracker.assertChange({ - nextDescendingStepTime: { absValue: 330n }, - nextStartTime: { absValue: 330n }, + activeStartTime: null, + nextDescendingStepTime: { absValue: 210n }, }); - - await driver.advanceTo(330n, 'wait'); }); + +// serial because dynamicConfig is shared across tests +// This test reproduces bug #8296. Now its expectations don't match the behavior +test.serial.skip( + 'late priceAuthority breaks auction; cannot recover via governance', + async t => { + const { collateral, bid } = t.context; + const driver = await makeAuctionDriver(t); + const asset = withAmountUtils(makeIssuerKit('Asset')); + const timerBrand = await E(driver.getTimerService()).getTimerBrand(); + + const liqSeat = await driver.setupCollateralAuction( + collateral, + collateral.make(1000n), + ); + await driver.updatePriceAuthority( + makeRatioFromAmounts(bid.make(11n), collateral.make(10n)), + ); + + const result = await E(liqSeat).getOfferResult(); + t.is(result, 'deposited'); + + await driver.advanceTo(167n); + const seat = await driver.bidForCollateralSeat( + // 1.1 * 1.05 * 200 + bid.make(231n), + collateral.make(200n), + ); + t.is(await E(seat).getOfferResult(), 'Your bid has been accepted'); + t.false(await E(seat).hasExited()); + + const scheduleTracker = await driver.getScheduleTracker(); + await scheduleTracker.assertInitial({ + activeStartTime: TimeMath.coerceTimestampRecord(170n, timerBrand), + nextDescendingStepTime: TimeMath.coerceTimestampRecord(170n, timerBrand), + nextStartTime: TimeMath.coerceTimestampRecord(210n, timerBrand), + }); + + await driver.advanceTo(170n, 'wait'); + await scheduleTracker.assertChange({ + nextDescendingStepTime: { absValue: 175n }, + }); + + await driver.advanceTo(175n, 'wait'); + await scheduleTracker.assertChange({ + nextDescendingStepTime: { absValue: 180n }, + }); + + // before the fix for #8296 in AuctionBook, this broke the ongoing auction. + await driver.setupCollateralAuction(asset, asset.make(500n)); + + await driver.advanceTo(180n, 'wait'); + await driver.advanceTo(185n, 'wait'); + t.true(await E(seat).hasExited()); + + await assertPayouts(t, seat, bid, collateral, 0n, 200n); + t.false(await E(liqSeat).hasExited()); + await E(liqSeat).tryExit(); + + await assertPayouts(t, liqSeat, bid, collateral, 0n, 0n); + + await driver.advanceTo(210n, 'wait'); + await driver.advanceTo(220n, 'wait'); + await driver.advanceTo(250n, 'wait'); + + // before the fix for #8296 in scheduler, this didn't repair the problem. + driver.setGovernedParam('DiscountStep', 2000n); + + await driver.advanceTo(300n, 'wait'); + + await scheduleTracker.assertChange({ + nextDescendingStepTime: { absValue: 330n }, + nextStartTime: { absValue: 330n }, + }); + + await driver.advanceTo(330n, 'wait'); + }, +); From 88083f94cf3055e7a96a7c4618c19c3ee0d36311 Mon Sep 17 00:00:00 2001 From: Chris Hibbert Date: Wed, 6 Sep 2023 14:50:08 -0700 Subject: [PATCH 3/5] chore: make error message more visible when quote is missing --- packages/inter-protocol/src/auction/auctionBook.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/inter-protocol/src/auction/auctionBook.js b/packages/inter-protocol/src/auction/auctionBook.js index 17ce9be001e..672c3ce7fe9 100644 --- a/packages/inter-protocol/src/auction/auctionBook.js +++ b/packages/inter-protocol/src/auction/auctionBook.js @@ -556,8 +556,8 @@ export const prepareAuctionBook = (baggage, zcf, makeRecorderKit) => { trace(this.state.collateralBrand, 'settleAtNewRate', reduction); const { capturedPriceForRound, priceBook, scaledBidBook } = state; if (!capturedPriceForRound) { - console.warn( - `No price for ${this.state.collateralBrand}, skipping auction.`, + console.error( + `⚠️No price for ${this.state.collateralBrand}, skipping auction.`, ); return; } From cb813c2e313967076afcff365398150167fa7ee4 Mon Sep 17 00:00:00 2001 From: Chris Hibbert Date: Wed, 6 Sep 2023 14:52:04 -0700 Subject: [PATCH 4/5] test: drop skipped test, included for review purposes --- .../test/auction/test-auctionContract.js | 78 ------------------- 1 file changed, 78 deletions(-) diff --git a/packages/inter-protocol/test/auction/test-auctionContract.js b/packages/inter-protocol/test/auction/test-auctionContract.js index d272aab1a7e..f953b605098 100644 --- a/packages/inter-protocol/test/auction/test-auctionContract.js +++ b/packages/inter-protocol/test/auction/test-auctionContract.js @@ -1495,81 +1495,3 @@ test.serial('add collateral type during auction', async t => { nextDescendingStepTime: { absValue: 210n }, }); }); - -// serial because dynamicConfig is shared across tests -// This test reproduces bug #8296. Now its expectations don't match the behavior -test.serial.skip( - 'late priceAuthority breaks auction; cannot recover via governance', - async t => { - const { collateral, bid } = t.context; - const driver = await makeAuctionDriver(t); - const asset = withAmountUtils(makeIssuerKit('Asset')); - const timerBrand = await E(driver.getTimerService()).getTimerBrand(); - - const liqSeat = await driver.setupCollateralAuction( - collateral, - collateral.make(1000n), - ); - await driver.updatePriceAuthority( - makeRatioFromAmounts(bid.make(11n), collateral.make(10n)), - ); - - const result = await E(liqSeat).getOfferResult(); - t.is(result, 'deposited'); - - await driver.advanceTo(167n); - const seat = await driver.bidForCollateralSeat( - // 1.1 * 1.05 * 200 - bid.make(231n), - collateral.make(200n), - ); - t.is(await E(seat).getOfferResult(), 'Your bid has been accepted'); - t.false(await E(seat).hasExited()); - - const scheduleTracker = await driver.getScheduleTracker(); - await scheduleTracker.assertInitial({ - activeStartTime: TimeMath.coerceTimestampRecord(170n, timerBrand), - nextDescendingStepTime: TimeMath.coerceTimestampRecord(170n, timerBrand), - nextStartTime: TimeMath.coerceTimestampRecord(210n, timerBrand), - }); - - await driver.advanceTo(170n, 'wait'); - await scheduleTracker.assertChange({ - nextDescendingStepTime: { absValue: 175n }, - }); - - await driver.advanceTo(175n, 'wait'); - await scheduleTracker.assertChange({ - nextDescendingStepTime: { absValue: 180n }, - }); - - // before the fix for #8296 in AuctionBook, this broke the ongoing auction. - await driver.setupCollateralAuction(asset, asset.make(500n)); - - await driver.advanceTo(180n, 'wait'); - await driver.advanceTo(185n, 'wait'); - t.true(await E(seat).hasExited()); - - await assertPayouts(t, seat, bid, collateral, 0n, 200n); - t.false(await E(liqSeat).hasExited()); - await E(liqSeat).tryExit(); - - await assertPayouts(t, liqSeat, bid, collateral, 0n, 0n); - - await driver.advanceTo(210n, 'wait'); - await driver.advanceTo(220n, 'wait'); - await driver.advanceTo(250n, 'wait'); - - // before the fix for #8296 in scheduler, this didn't repair the problem. - driver.setGovernedParam('DiscountStep', 2000n); - - await driver.advanceTo(300n, 'wait'); - - await scheduleTracker.assertChange({ - nextDescendingStepTime: { absValue: 330n }, - nextStartTime: { absValue: 330n }, - }); - - await driver.advanceTo(330n, 'wait'); - }, -); From e43c818653063207add45e0f12a7adabc76f74f4 Mon Sep 17 00:00:00 2001 From: Chris Hibbert Date: Wed, 6 Sep 2023 15:22:51 -0700 Subject: [PATCH 5/5] chore: restore ts-expect-error that is still needed --- packages/inter-protocol/src/auction/auctioneer.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/inter-protocol/src/auction/auctioneer.js b/packages/inter-protocol/src/auction/auctioneer.js index f2592ad7454..7a705398dc3 100644 --- a/packages/inter-protocol/src/auction/auctioneer.js +++ b/packages/inter-protocol/src/auction/auctioneer.js @@ -679,6 +679,7 @@ export const start = async (zcf, privateArgs, baggage) => { makeScheduler( driver, timer, + // @ts-expect-error types are correct. How to convince TS? params, timerBrand, scheduleRecorder,