-
Notifications
You must be signed in to change notification settings - Fork 212
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(auction): add an auctioneer to manage vault liquidation #7000
Conversation
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.
Partial pass. Back soon.
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.
Another pass. Not a totally thorough review but it's draft. I'll look again when re-requested.
Quite an achievement already!
// until DB supports composite keys, copy its method for turning numbers to DB | ||
// entry keys |
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 comment. @warner is there an issue for DB supporting composite keys?
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 should have searched: #5263
Please include the issue in the comment
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.
After a refactoring of sortedOffer keys suggested by MarkM, I'm getting composite keys by using encodeData
on an array of the key components.
); | ||
}; | ||
|
||
const discountRatioFromFloat = ( |
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 don't understand why there are parallel functions for price/discount that do the same thing.
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.
They're conceptually different types. Prices are currency/collateral, while discounts are currency/currency. I've dropped denomBrand from discounts, and made the assertions more precise.
// prices in this book are expressed as percentage of full price. .4 is 60% off. | ||
// 1.1 is 10% above par. |
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 comment is helpful. I think it suggests choosing a different word than "Discount" for this functionality. It also sounds like "more discount" is "smaller price" but it's the other way around.
brainstorming,
- Coefficient
- Coeff
- Proportion
- Prop
- Premium
- Multiplier
- QuoteFactor
None seem obviously better on balance than Discount.
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.
Your point about the directionality is persuasive. priceScalingFactor
is the most explicit description I've come up with. coeff
is okay, but I prefer factor. opinions about 'scaling' vs. 'scale'?
Multiplier
seems less on-point than factor.proportion
has the same drawback asdiscount
in sounding like it's restricted to less than one.premium
has the opposite problem, likemarkup
.quoteFactor
has a subtle appeal. It says that it's markup or markdown that will be applied to the quote, but doesn't say that it's part of a bid.
How about bidScaleFactor
or bidScaling
?
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.
"bid" definitely clarifies. Slight preference for Scaling over ScaleFactor. ScaleFactor is a bit of Hungarian notation and is longer without adding clarity. So 👍 to bidScaling
.
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.
done. Fairly pervasively. I didn't change parameter names, which are visible to EC governance, etc. I think they'll be happier with 'discount', and if not, PM can say what term to use instead.
@Chris-Hibbert @turadg in reading the conversations you two have above, to help my review process, I'll Resolve those conversations that seem resolved, to help focus attention on the remainder. These aren't my conversations, so please let me know if this interferes with the way you two are coordinating, and unresolve as needed. Thanks. |
// This will work just fine for at least 250 years. And notice that these | ||
// timestamps are used for sorting during an auction and don't need to be stored | ||
// long-term. We could safely subtract from a timestamp that's now + 1 month. | ||
const FarFuture = 10000000000000n; |
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.
const FarFuture = 10000000000000n; | |
const FarFuture = 10_000_000_000_000n; |
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.
Switched to sequence numbers so this was no longer needed. Then you made me realize that sorting in the store never needs to care about times, so it's gone.
/** | ||
* @param {ZCF<GovernanceTerms<{ | ||
* StartFrequency: 'relativeTime', | ||
* ClockStep: 'relativeTime', | ||
* StartingRate: 'nat', | ||
* lowestRate: 'nat', | ||
* DiscountStep: 'nat', | ||
* }> & {timerService: import('@agoric/time/src/types').TimerService, priceAuthority: PriceAuthority}>} zcf | ||
* @param {{initialPoserInvitation: Invitation, storageNode: StorageNode, marshaller: Marshaller}} privateArgs | ||
* @param {Baggage} baggage | ||
*/ |
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.
In order to review this, I found myself reformatting it to
/** | |
* @param {ZCF<GovernanceTerms<{ | |
* StartFrequency: 'relativeTime', | |
* ClockStep: 'relativeTime', | |
* StartingRate: 'nat', | |
* lowestRate: 'nat', | |
* DiscountStep: 'nat', | |
* }> & {timerService: import('@agoric/time/src/types').TimerService, priceAuthority: PriceAuthority}>} zcf | |
* @param {{initialPoserInvitation: Invitation, storageNode: StorageNode, marshaller: Marshaller}} privateArgs | |
* @param {Baggage} baggage | |
*/ | |
/** | |
* @param {ZCF<GovernanceTerms<{ | |
* StartFrequency: 'relativeTime', | |
* ClockStep: 'relativeTime', | |
* StartingRate: 'nat', | |
* lowestRate: 'nat', | |
* DiscountStep: 'nat', | |
* }> & { | |
* timerService: import('@agoric/time/src/types').TimerService, | |
* priceAuthority: PriceAuthority | |
* }>} zcf | |
* @param {{ | |
* initialPoserInvitation: Invitation, | |
* storageNode: StorageNode, | |
* marshaller: Marshaller | |
* }} privateArgs | |
* @param {Baggage} baggage | |
*/ |
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.
The params to GovernedTerms
seems annoyingly redundant with auctioneerParamTypes
. Can we reduce the redundancy? If not, or in the meantime, are they checked against each other for consistency?
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.
Aside from auctioneerParamTypes
, the privateArgs
are exactly what is needed for handleParamGovernance
. Is this expected to be a typical pattern? Any way to make this more convenient?
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 agree on the redundancy. I don't think they're checked directly against each other, but I think typescript knows that the params returned from handleParamGovernance
(line 157) match this declaration, and compare that to the requirements of makeScheduler, which declares params
to be AuctionParamManaager
which was declared in params.js
.
privateArgs
are exactly what is needed forhandleParamGovernance
. Is this expected to be a typical pattern?
yes.
@turadg understands these interconnections much better than I do. Would you respond?
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.
Any way to make this more convenient?
There's a bunch of refactoring we could do to governance. I'd like to make it require less repetition. Not a priority tho.
For this particular case we could have a type like ParamGovernancePrivateArgs
and union that here. But the args marshaller
and storageNode
probably will leave handleParamGovernance
once we stop using the deprecated StoredPublishKit for governance. I expect that to happen when we make governance durable.
const brandToKeyword = provide(baggage, 'brandToKeyword', () => | ||
makeScalarBigMapStore('deposits', { | ||
durable: true, | ||
}), | ||
); |
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.
AFAICT, the only difference between this and
const brandToKeyword = provide(baggage, 'brandToKeyword', () => | |
makeScalarBigMapStore('deposits', { | |
durable: true, | |
}), | |
); | |
const brandToKeyword = provideDurableMapStore(baggage, 'brandToKeyword'); |
is the use of two different literal strings, so that the errors associated with this mapStore are labeled deposits
instead of brandToKeyword
. Is that the motivation? If so, ok.
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.
No. I should convert to provideDurableMapStore(). AIR, Turadg suggested it here, but I missed one of the MapStores.
* @param {Ratio} ratio | ||
* @returns {number} | ||
*/ | ||
export const coerceToNumber = ratio => { |
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.
While there may not be a widely accepted definition of coercion
in programming languages, I use the term only for conversion functions that are idempotent in the following sense:
coerceToX(y)
is equivalent to coerceToX(coerceToX(y))
IOW, whatever the target form this is coercing to, if the input is already of that form, then it does not need to be coerced and is passed through unmodified.
I saw @turadg 's other suggestion ratioAsFloat
. This seems more right, as this is just converting from one representation to another. In JS, "number" means "double precision floating point", so I'd be equally happy with ratioAsNumber
or ratioToNumber
.
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.
ratioAsNumber
makes it sound like they're equivalent representations, but you can't convert back to the same ratio, so I'm going with ratioToNumber
.
}, | ||
{ | ||
want: AmountShape, | ||
offerDiscount: makeRatioPattern(currencyBrand, currencyBrand), |
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.
as you point out above this can be markup.
I first read that as "markup" as in HTML, and was momentarily confused ;)
I can't think of one word for both either. But it seems like a concept that must have a word. Weird.
); | ||
|
||
trace(`settling`, pricedOffers.length, discOffers.length); | ||
prioritizedOffers.forEach(([key, { seat, price: p, wanted }]) => { |
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 also prefer syntactic for
loops over forEach
specifically. (For the other higher order array methods, I have no general preference, but evaluate case by case.)
Two reasons I prefer syntax:
- usually easier to step through iterations in a debugger, depending on the debugger
- easy to exit early, which often arises after the code is initially written
if (deposits.has(amount.brand)) { | ||
const depositListForBrand = deposits.get(amount.brand); | ||
deposits.set( | ||
amount.brand, | ||
harden([...depositListForBrand, { seat, amount }]), | ||
); | ||
} else { | ||
deposits.init(amount.brand, harden([{ seat, amount }])); | ||
} |
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.
}; | ||
|
||
const driver = Far('Auctioneer', { | ||
descendingStep: () => { |
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.
Became reducePriceAndTrade
?
Please try to wakeup CI so we can see what it has to say. |
If CI is reasonably happy, should this be Ready for review? |
const makeZeroRatio = () => | ||
makeRatioFromAmounts( | ||
AmountMath.makeEmpty(currencyBrand), | ||
AmountMath.make(collateralBrand, 1n), | ||
); |
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.
const makeZeroRatio = () => | |
makeRatioFromAmounts( | |
AmountMath.makeEmpty(currencyBrand), | |
AmountMath.make(collateralBrand, 1n), | |
); | |
const zeroRatio = | |
makeRatioFromAmounts( | |
AmountMath.makeEmpty(currencyBrand), | |
AmountMath.make(collateralBrand, 1n), | |
); |
and then replace every makeZeroRatio()
with zeroRatio
.
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 noticed that somewhere else and removed it, but I apparently missed it here. I don't know what evolution the code must have gone through for that to have happened, but it sure looks dumb now.
|
||
const collShare = makeRatioFromAmounts(collatRaise, totCollDeposited); | ||
const currShare = makeRatioFromAmounts(currencyRaise, totCollDeposited); | ||
/** @type {import('@agoric/zoe/src/contractSupport/atomicTransfer.js').TransferPart[]} */ |
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.
Ugh, should not need a deep import just to use normal helper APIs. This is on me. Any idea how to export the type so it can be imported simply?
In any case, very cool use of a temporarily mutable array to gather all the parts of a rearrange!
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.
Imo deep imports aren't so bad with types since they don't affect runtime import graph.
For the particular case, it'll be solved by putting the atomicTransfer capability on Zoe proper (instead of this contractSupport helper). Since that's already scheduled I don't think it's worth trying to address this case that will be obviated.
export const RelativeTimeRecordShape = harden({ | ||
timerBrand: TimerBrandShape, | ||
relValue: RelativeTimeValueShape, | ||
}); |
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.
export const RelativeTimeRecordShape = harden({ | |
timerBrand: TimerBrandShape, | |
relValue: RelativeTimeValueShape, | |
}); |
Anyone who needs this should import it from @agoric/time
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.
agreed. done.
const trace = makeTracer('SCHED', false); | ||
|
||
/** | ||
* @file The scheduler is presumed to be quiescent between auction rounds. Each |
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.
"presumed" huh?
How confident are we that this presumption cannot be violated?
If it is violated, what happens? How bad is it?
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 mostly wrote that to simplify my mental model for testing. The manual clock complicates things, since in tests, it doesn't seem to reliably hit all intermediate times. (I think this is related to the fact that eventLoopIteration()
is required.)
If the clock steps through every time, and turns have time to finish, then the states change correctly and the auction will be quiescent. I'm pretty confident that I've checked the incoming parameters so that successive auctions cannot overlap, which is the main thing that would violate this rule.
If auction periods overlap, the auctioneer will be confused about what's being sold at which price.
@erights requested some cleanup of the sorted offers in discussion. I don't think I'm making any use of the multipart key, and should simplify to just the first part. If I do that, then the other part of the discussion about not needing to encode the time part becomes moot. |
4eef984
to
2b4e795
Compare
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.
Not done but after the 4th signal to Refresh the review I figure I should post these before they're stale! ;-)
@@ -184,6 +188,18 @@ const makeParamManagerBuilder = (publisherKit, zoe) => { | |||
return builder; | |||
}; | |||
|
|||
/** @type {(name: string, value: import('@agoric/time/src/types').Timestamp, builder: ParamManagerBuilder) => ParamManagerBuilder} */ | |||
const addTimestamp = (name, value, builder) => { | |||
buildCopyParam(name, value, assertTimestamp, ParamTypes.TIMESTAMP); |
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.
out of scope: consider making buildCopyParam take a shape in the assert/coerce parameter. If it's a function, then it's a coercer and if it's not then it's a shape to match.
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. #7118
* The book contains orders for the collateral. It holds two kinds of | ||
* orders: | ||
* - Prices express the bid in terms of a Currency amount | ||
* - Scaled bid express the bid in terms of a discount (or markup) from the |
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.
* - Scaled bid express the bid in terms of a discount (or markup) from the | |
* - Scaled bids express the bid in terms of a discount (or markup) from the |
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.
done
* Offers can be added in three ways. When the auction is not active, prices are | ||
* automatically added to the appropriate collection. If a new offer is at or | ||
* above the current price of an active auction, it will be settled immediately. | ||
* If the offer is below the current price, it will be added, and settled when | ||
* the price reaches that level. |
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.
* Offers can be added in three ways. When the auction is not active, prices are | |
* automatically added to the appropriate collection. If a new offer is at or | |
* above the current price of an active auction, it will be settled immediately. | |
* If the offer is below the current price, it will be added, and settled when | |
* the price reaches that level. | |
* Offers can be added in three ways. 1) When the auction is not active, prices are | |
* automatically added to the appropriate collection. When the auction is active: | |
* 2) If a new offer is at or above the current price, it will be settled immediately; | |
* 3) If the offer is below the current price, it will be added, and settled when | |
* the price reaches that level. |
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.
yes, that's clearer.
); | ||
const BidSpecShape = M.or( | ||
{ | ||
want: AmountShape, |
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 time to use E(brand).getAmountShape()
and I think makeBrandedRatioPattern
should use the same shapes for its amounts.
though looking at makeBrandedRatioPattern
I see we can match on the exact brand by just using the brand
object. I suppose Brand has getAmountShape()
because you might not know the kind from the brand, but if you already do, why spend a remote call to get that shape? shouldn't we have a makeNatAmountshape(brand)
function? That would make the TODOs in #7041 pretty trivial.
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.
done. (at least some interpretation of what you said.) I wanted the patterns to enforce the brands but hadn't noticed brand.getAmountShape()
.
|
||
// @ts-expect-error types are correct. How to convince TS? | ||
const scheduler = await makeScheduler(driver, timer, params, timerBrand); | ||
const depositOfferHandler = zcfSeat => { |
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.
consider inlining.
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 don't think that makes it easier to read.
/** in seconds, how often to reduce the price */ | ||
export const CLOCK_STEP = 'ClockStep'; | ||
/** discount or markup for starting price in basis points. 9999 = 1bp discount */ | ||
export const STARTING_RATE = 'StartingRate'; |
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 comment won't appear when setting this params in UIs. WDYT of appending 'BP' for all the basis points values?
alternately, express as a ratio? if it's too cumbersome to include the brands I think we should consider a RATIONAL param type because the work-around of a Nat is worse
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.
The UI ought to want to provide help text to the user, right? If I were writing that help text, I'd look here. Is there a better place?
appending 'BP' is good.
The brands are a hassle when setting governed params, AFAICT.
When governance is trying to setup a vote, ISTM that creating a ratio would be an issue. Can you think of a way for that UI/CLI to make that palatable?
* @param {RelativeTime} initial.priceLockPeriod | ||
* @param {import('@agoric/time/src/types').TimerBrand} initial.timerBrand | ||
*/ | ||
export const makeAuctioneerParamManager = (publisherKit, zoe, initial) => { |
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.
hoping to find time eventually to DRY out governance params. there's lots of repetition to define a parameter.
@@ -81,6 +84,12 @@ const MILLI = 1_000_000n; | |||
* governorCreatorFacet: GovernedContractFacetAccess<VaultFactoryPublicFacet, VaultFactoryCreatorFacet>, | |||
* adminFacet: AdminFacet, | |||
* }, | |||
* auctionKit: { |
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.
most of those are ${contractName}Kit
.
* auctionKit: { | |
* auctioneerKit: { |
* TRUE if the discount(/markup) applied to the price is higher than the quote. | ||
* | ||
* @param {Ratio} bidScaling | ||
* @param {Ratio} currentPrice | ||
* @param {Ratio} oraclePrice |
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.
* TRUE if the discount(/markup) applied to the price is higher than the quote. | |
* | |
* @param {Ratio} bidScaling | |
* @param {Ratio} currentPrice | |
* @param {Ratio} oraclePrice | |
* @param {Ratio} bidScaling | |
* @param {Ratio} currentPrice | |
* @param {Ratio} oraclePrice | |
* @returns {boolean} TRUE iff the discount(/markup) applied to the price is higher than the quote. |
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.
done
/** @type {'active' | 'waiting'} */ | ||
let auctionState = AuctionState.WAITING; | ||
|
||
const computeRoundTiming = baseTime => { |
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.
wdt of extracting this to unit test with arbitrary params?
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; done.
c29f2c2
to
bfaa587
Compare
refs: #6641
Description
Separate pieces include a scheduler that manages the phases of an auction, the AuctionBook that holds onto offers to buy, and the auctioneer, which accepts good for sale and has the APIs. Params are managed via governance. The classes are not durable.
The changes to VaultFactory to make use of this approach to liquidation rather than the AMM are in a separate PR, which will be available before this PR is finalized.
Security Considerations
Mainly economic. This will be a core part of our economy.
Scaling Considerations
As the vault economy grows, this will be a major point of contention. Traders will be very interested in getting trades executed quickly. The design is intended to allow traders to place the orders they want without requiring lots of interactive adjustments.
Documentation Considerations
The user focus will be mostly on the UI.
Testing Considerations
Decent tests of scheduler, book, and auction. I expect intensive manual testing once it's in a visible place.