-
Notifications
You must be signed in to change notification settings - Fork 208
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
Conversation
c350319
to
fad96bb
Compare
Fail`price must be captured before auction starts`; | ||
assert(capturedPriceForRound); | ||
if (!capturedPriceForRound) { | ||
console.warn( |
There was a problem hiding this comment.
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?
agoric-sdk/packages/inter-protocol/src/vaultFactory/vaultManager.js
Lines 749 to 751 in fb2e7f5
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
now = await E(timer).getCurrentTimestamp(); | ||
if ( | ||
!nextSchedule || | ||
TimeMath.compareAbs(nextSchedule.startTime, now) < 0 | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with the new await that the await null
is redundant. But I'd propose instead only awaiting the timestamp if necessary.
now = await E(timer).getCurrentTimestamp(); | |
if ( | |
!nextSchedule || | |
TimeMath.compareAbs(nextSchedule.startTime, now) < 0 | |
) { | |
if ( | |
!nextSchedule || | |
TimeMath.compareAbs(nextSchedule.startTime, await E(timer).getCurrentTimestamp()) < 0 | |
) { |
or perhaps more readably,
now = await E(timer).getCurrentTimestamp(); | |
if ( | |
!nextSchedule || | |
TimeMath.compareAbs(nextSchedule.startTime, now) < 0 | |
) { | |
const validSchedule = nextSchedule && TimeMath.compareAbs(nextSchedule.startTime, await E(timer).getCurrentTimestamp()) < 0; | |
if (!validSchedule) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't save now
, so I end up making the call to getCurrentTimestamp
inside a conditional.
let repairableSchedule;
if (!nextSchedule) {
repairableSchedule = true;
} else {
now = await E(timer).getCurrentTimestamp();
repairableSchedule =
TimeMath.compareAbs(nextSchedule.startTime, now) < 0;
}
if (repairableSchedule) {
The TimeMath comparison will only be called if we did need to update now
, so it's relatively clean. I tried to use a ternary, but it wasn't pretty (since the else has to be two statements--it's possible with a comma expression, but not great.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't realize now
needs to be saved.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need a regression test. so please change this test to ensure that a missing price feed does not throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I'll add a test for that case. I think this test also needs to be part of the PR, just to show the reproduction scenario in case anyone needs to work through that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG, contingent upon removing the skipped test. (Thanks for including it to facilitate review.)
await driver.advanceTo(220n, 'wait'); | ||
await driver.advanceTo(250n, 'wait'); | ||
|
||
// before the fix for #8296 in scheduler, this didn't repair the problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
…goric#8301) * fix: Make the auction schedule robust when adding collateral types fixes: 8296 * refactor: clean up timestamp await, and add test. * chore: make error message more visible when quote is missing * test: drop skipped test, included for review purposes * chore: restore ts-expect-error that is still needed
closes: #8296
This fixes two problems.
#8307 covers the additional issue of ensuring that all repeating timers in the auction should survive exceptions.
Description
If a new collateral type is added to the auctioneer before its price is available from an oracle, it breaks the schedule of all auctions in a way that is irretrievable.
Security Considerations
Auction shouldn't be this delicate.
Scaling Considerations
None.
Documentation Considerations
None.
Testing Considerations
Adds a test.
Upgrade Considerations
Has to wait for the ability to upgrade governed contracts.