orbital auction #24
orbital auction #24
Conversation
…und start date into phases config
| /// - filling: window where the auction is finalized and orders are matched | ||
| /// - cleanup: window where the auction is reset and the next round is prepared | ||
| // TODO: validate that all durations are passed in seconds. | ||
| // or just drop Duration altogether and deal in seconds? |
| pub fn advance(&self, storage: &mut dyn Storage) -> StdResult<Self> { | ||
| let auction_config = AUCTION_CONFIG.load(storage)?; | ||
|
|
||
| let next_id = self.id + Uint64::one(); |
| pub start_expiration: Expiration, | ||
| pub auction_expiration: Expiration, | ||
| pub filling_expiration: Expiration, | ||
| pub cleanup_expiration: Expiration, |
There was a problem hiding this comment.
I find the naming confusing .. I understand the type is named Expiration, but adding it as a prefix in member names still involve some added brain processing; how about auction_start, auction_end, filling_end and cleanup_end (if I understood it correctly) ?
There was a problem hiding this comment.
agree, was confused by the naming as well here
There was a problem hiding this comment.
fair enough! the time-related types in cw are a bit scattered so I felt like having _expiration would add clarity as to what the field is, but I'll change it
| } else { | ||
| AuctionPhase::Bidding | ||
| } | ||
| } |
There was a problem hiding this comment.
I find this to be very risky and a potential vector for many issues .. maybe I don't have the full context, but I would not rely on block time at all but on block heights only. I have the impression (but correct me if I'm wrong) that there is an implicit / hidden assumption on constant block times and/or (almost) zero down-time. I feel that a design relying on blocks (+ detection for chain downtimes) would be more resilient to the underlying chain infrastructure's failures and also, potentially, more portable.
| } else { | ||
| Err(StdError::generic_err("order exceeds batch capacity")) | ||
| } | ||
| } |
| // otherwise, we still have time to include the order in the active batch that is about to start | ||
| // to do that, we first check if the batch has the capacity to fit this order | ||
| if current_auction.batch.can_fit_order(&user_intent) { | ||
| current_auction.batch.include_order(user_intent)?; |
There was a problem hiding this comment.
Same as above .. further encapsulate logic: current_auction.can_fit_order& current_auction.include_order would be more readable.
There was a problem hiding this comment.
Given the complexity of the domain, I would avoid structs with all members being public. I'd strive to encapsulate and control access to private members just so as to ensure that domain invariants are enforced no matter what the calling code does.
| } | ||
|
|
||
| /// moves the user intents from the orderbook queue to the active auction batch | ||
| fn process_orderbook(storage: &mut dyn Storage) -> StdResult<()> { |
There was a problem hiding this comment.
You could also pass the active_auction as param as it's already been loaded from storage before.
| { | ||
| if let Some(intent) = ORDERBOOK.pop_front(storage)? { | ||
| let capacity = active_auction.batch.remaining_capacity(); | ||
| if intent.amount <= capacity { |
There was a problem hiding this comment.
Use the can_fit_order helper func here instead ?
| amount, | ||
| offer_domain: self.offer_domain.clone(), | ||
| ask_domain: self.ask_domain.clone(), | ||
| }; |
There was a problem hiding this comment.
You could take a &mut selfreference, and modify the current intent's amount, and return (self, remainder).
| }, | ||
| }; | ||
|
|
||
| pub const CONTRACT_NAME: &str = "orbital-auction"; |
There was a problem hiding this comment.
nit: prefer
const CONTRACT_NAME: &str = env!("CARGO_PKG_NAME");to not hardcode in two places
| fn slash_solver(storage: &mut dyn Storage, solver: Addr) -> StdResult<()> { | ||
| let auction_config = AUCTION_CONFIG.load(storage)?; | ||
| let mut posted_bond = POSTED_BONDS.load(storage, solver.to_string())?; | ||
| posted_bond.amount = posted_bond | ||
| .amount | ||
| .checked_sub(auction_config.solver_bond.amount)?; | ||
|
|
||
| POSTED_BONDS.save(storage, solver.to_string(), &posted_bond)?; | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
What is done with the slashed amount? If we are not doing anything with it yet or it's not decided maybe add a TODO here
| // bidder actions | ||
| /// post a bond to participate in the auction | ||
| PostBond {}, | ||
|
|
||
| /// withdraw the posted bond | ||
| WithdrawBond {}, |
There was a problem hiding this comment.
Not that important: Should we have an increase/decrease bond? If we are happy with Withdrawing and posting again It's fine
| pub fn advance(&self, storage: &mut dyn Storage) -> StdResult<Self> { | ||
| let auction_config = AUCTION_CONFIG.load(storage)?; | ||
|
|
||
| let next_id = self.id + Uint64::one(); | ||
|
|
||
| let next_phases = self.phases.shift_phases(&auction_config.auction_phases)?; | ||
| let next_batch = Batch { | ||
| batch_capacity: auction_config.batch_size, | ||
| batch_size: Uint128::zero(), | ||
| user_intents: vec![], | ||
| current_bid: None, | ||
| }; | ||
|
|
||
| Ok(AuctionRound { | ||
| id: next_id, | ||
| phases: next_phases, | ||
| batch: next_batch, | ||
| }) |
There was a problem hiding this comment.
Should we save the AUCTION_CONFIG here instead of after this function?
| pub start_expiration: Expiration, | ||
| pub auction_expiration: Expiration, | ||
| pub filling_expiration: Expiration, | ||
| pub cleanup_expiration: Expiration, |
There was a problem hiding this comment.
agree, was confused by the naming as well here
| pub struct AuctionPhaseConfig { | ||
| // duration of the bidding window in seconds | ||
| pub auction_duration: Duration, | ||
| // duration of the filling window in seconds | ||
| pub filling_window_duration: Duration, | ||
| // duration of the cleanup window in seconds | ||
| pub cleanup_window_duration: Duration, | ||
| } | ||
|
|
||
| impl AuctionPhaseConfig { | ||
| /// validates that the auction phase configuration is all based on time (seconds). | ||
| pub fn validate(&self) -> StdResult<()> { | ||
| if let Duration::Height(_) = self.auction_duration { | ||
| return Err(StdError::generic_err( | ||
| "auction duration must be a time duration", | ||
| )); | ||
| } | ||
| if let Duration::Height(_) = self.filling_window_duration { | ||
| return Err(StdError::generic_err( | ||
| "filling duration must be a time duration", | ||
| )); | ||
| } | ||
| if let Duration::Height(_) = self.cleanup_window_duration { | ||
| return Err(StdError::generic_err( | ||
| "cleanup duration must be a time duration", | ||
| )); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Since we are using Timestamps only for everything, why not use u64 directly instead of using Duration?
closes #19
introduces the core logic of orbital auction:
this pr does not include integration with
orbital-core, so it is still rough around the edges and contains someunimplemented!()calls. that being said, the missing pieces are quite isolated from the core auction mechanics and this pr is already big enough so I think it makes sense to get some feedback on this sooner rather than later.