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: Make the auction schedule robust when adding collateral types #8301

Merged
merged 6 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions packages/inter-protocol/src/auction/auctionBook.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

from throw to warn… is it not an error to start the auction without a price feed?

console.error(
`⚠️ Excess collateral remaining sent to reserve. Expected ${q(
plan.collatRemaining,

maybe not. I suppose we have decided it is part of the spec that the auction skips if there's no feed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an error, but it should only affect the auction for that collateral. Throwing is an over-reaction.

Copy link
Member

Choose a reason for hiding this comment

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

Certainly throwing is an over-reaction. My question was about how to log.

It's an error

Then should it be console.error? Consider also ⚠️ in the message to draw attention to it in log output. I think "🚨" would be too severe as this is a known possible state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with error, though I haven't seen any evidence that it increases visibility.

I had intended to add a glyph, but forgot. Thanks for the reminder.

`No price for ${this.state.collateralBrand}, skipping auction.`,
);
return;
}

state.curAuctionPrice = multiplyRatios(
reduction,
Expand Down
1 change: 0 additions & 1 deletion packages/inter-protocol/src/auction/auctioneer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
10 changes: 10 additions & 0 deletions packages/inter-protocol/src/auction/scheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,17 @@ export const makeScheduler = async (
async updateState(_newState) {
trace('received param update', _newState);
await null;

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();
Expand Down
165 changes: 165 additions & 0 deletions packages/inter-protocol/test/auction/test-auctionContract.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -1408,3 +1423,153 @@ 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
test.serial('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 scheduleTracker.assertChange({
nextDescendingStepTime: { absValue: 185n },
});

await driver.advanceTo(185n, 'wait');

await scheduleTracker.assertChange({
nextDescendingStepTime: { absValue: 190n },
});

t.true(await E(seat).hasExited());
await assertPayouts(t, seat, bid, collateral, 0n, 200n);

await driver.advanceTo(210n, 'wait');

t.true(await E(liqSeat).hasExited());
await assertPayouts(t, liqSeat, bid, collateral, 231n, 800n);

await scheduleTracker.assertChange({
activeStartTime: null,
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.
Copy link
Member

Choose a reason for hiding this comment

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

ACK

driver.setGovernedParam('DiscountStep', 2000n);

await driver.advanceTo(300n, 'wait');

await scheduleTracker.assertChange({
nextDescendingStepTime: { absValue: 330n },
nextStartTime: { absValue: 330n },
});

await driver.advanceTo(330n, 'wait');
},
);
Loading