feat(auction-server): Bid states #23
Conversation
auction-server/src/api/bid.rs
Outdated
| store.ws.broadcast_sender.clone(), | ||
| ) | ||
| .await; | ||
| Ok(bid_id) |
There was a problem hiding this comment.
I think the function above contains business logic and it's better to not have any business logic on the API layer. It is also creating dependency from liquidation.rs to bid.rs.
auction-server/src/api/ws.rs
Outdated
| opportunity_bid, | ||
| opportunity_id, | ||
| } => { | ||
| match handle_liquidation_bid( |
There was a problem hiding this comment.
this is also a call from an api to api. Perhaps the handle_liquidation_bid should be outside API too.
auction-server/src/auction.rs
Outdated
| let winner_ids:Vec<Uuid> = winner_bids.iter().map(|b| b.id).collect(); | ||
| for bid in cloned_bids { | ||
| let status = match winner_ids.contains(&bid.id){ | ||
| true =>BidStatus::Submitted(receipt.transaction_hash), |
auction-server/src/state.rs
Outdated
| &self, | ||
| id: BidId, | ||
| status: BidStatus, | ||
| event_sender: broadcast::Sender<UpdateEvent>, |
There was a problem hiding this comment.
The way event_sender is passed and used doesn't make much sense to me. Either it should be part of the struct or I think the caller should just set it (as it doesn't fit the responsibility of the store).
auction-server/src/auction.rs
Outdated
| true =>BidStatus::Submitted(receipt.transaction_hash), | ||
| false =>BidStatus::Lost | ||
| }; | ||
| store.bid_status_store.set_status(bid.id, status, store.ws.broadcast_sender.clone()).await; |
There was a problem hiding this comment.
back to the comment on the method definition. The name here also doesn't convey that we are sending an event (the code reader might lose the code flow)
auction-server/src/state.rs
Outdated
| @@ -118,8 +127,47 @@ pub struct LiquidationStore { | |||
| pub opportunities: RwLock<HashMap<PermissionKey, Vec<LiquidationOpportunity>>>, | |||
There was a problem hiding this comment.
Dashmap is more efficient here.
auction-server/src/api/bid.rs
Outdated
| #[derive(Serialize, Deserialize, ToResponse, ToSchema, Clone)] | ||
| pub struct BidResult { | ||
| pub status: String, | ||
| /// The unique id created to identify the bid. This id can be used to query the status of the bid. | ||
| #[schema(example = "f47ac10b-58cc-4372-a567-0e02b2c3d479", value_type=String)] | ||
| pub id: Uuid, |
There was a problem hiding this comment.
slight nit: it may be nice to include a header in front of different types of UUIDs, e.g. opportunity UUIDs always prefaced by O, bid UUIDs always prefaced by B. would help for verbosity/identification on client side
There was a problem hiding this comment.
sg, opportunity ids start with obo3ee3e and bids start with beedbeed
|
|
||
| #[derive(Serialize, Deserialize, ToSchema, Clone)] | ||
| pub struct OpportunityBid { | ||
| /// The opportunity permission key |
There was a problem hiding this comment.
I think this struct should include opportunity ID. I know this is submitted with ID in the path, but it helps with identification and specificity, especially on the client side. Redundancy isn't so bad here
There was a problem hiding this comment.
I agree, but this duplications makes the logic a bit weird on the server side. For example we should throw some error when these ids do not match or ...
On the js client side, I have created another OpportunityBid class which includes the id and is formed differently.
In general these structures are not necessarily the ones that the users will interact with in the sdks.
| let params = match versioned_params.clone() { | ||
| OpportunityParams::V1(params) => params, | ||
| }; | ||
| let OpportunityParams::V1(params) = versioned_params.clone(); |
There was a problem hiding this comment.
wouldn't it be better to match against this, in anticipation of future versions?
There was a problem hiding this comment.
clippy did this automatically. We will switch to match when we have more than one variant I guess.
| let mut write_lock = store.liquidation_store.opportunities.write().await; | ||
|
|
||
| if let Some(opportunities_existing) = write_lock.get_mut(¶ms.permission_key) { | ||
| let opportunities_map = &store.liquidation_store.opportunities; |
There was a problem hiding this comment.
why get rid of the write_lock? no concurrency issues here?
| .ok_or(RestError::OpportunityNotFound)?; | ||
|
|
||
| // TODO: move this logic to searcher side | ||
| if opportunity.bidders.contains(&opportunity_bid.liquidator) { |
There was a problem hiding this comment.
i think just get rid of this, allow it to just update the bid amount
There was a problem hiding this comment.
will do in another PR. We should discuss whether it's an update or just an additional bid.
| #[derive(Serialize, Deserialize, ToSchema, Clone, PartialEq)] | ||
| #[serde(tag = "status", content = "result", rename_all = "snake_case")] | ||
| pub enum BidStatus { | ||
| /// The auction for this bid is pending |
There was a problem hiding this comment.
similar comment to above: any reason not to include the bid id here? may help for verbosity/clarity on client side
There was a problem hiding this comment.
there is usually a larger structure which includes this status field and the bid id. Like a hashmap where these are the values and the id is the key or the BidResult structure.
| @@ -99,6 +100,10 @@ pub async fn start_server(run_options: RunOptions) -> anyhow::Result<()> { | |||
| let store = Arc::new(Store { | |||
There was a problem hiding this comment.
is update_rx used anywhere?
There was a problem hiding this comment.
what does update_tx stand for? as i understand it, it's supposed to be where you send messages for the channel that ought to be broadcasted, but it also sounds like "transaction" in the blockchain context? not sure if there is a clearer name you can use
There was a problem hiding this comment.
yes update_rx is used as broadcast_receiver later on.
tx and rx are the default names which stand for transmit and receive and are used mostly in a network context.
Renamed
Implement APIs for fetching a bid state via http and ws