-
Notifications
You must be signed in to change notification settings - Fork 5
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
edits_feb5 #15
edits_feb5 #15
Changes from 10 commits
15d272f
8ccd3ce
8723a1c
edbf7f6
335682d
1b562ec
559e153
e181a32
5c5b21d
7b2b62b
7a97205
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,15 @@ pub struct OpportunityParamsWithId { | |
params: OpportunityParams, | ||
} | ||
|
||
impl Into<OpportunityParamsWithId> for LiquidationOpportunity { | ||
fn into(self) -> OpportunityParamsWithId { | ||
OpportunityParamsWithId { | ||
opportunity_id: self.id, | ||
params: self.params, | ||
} | ||
} | ||
} | ||
|
||
/// Submit a liquidation opportunity ready to be executed. | ||
/// | ||
/// The opportunity will be verified by the server. If the opportunity is valid, it will be stored in the database | ||
|
@@ -105,12 +114,32 @@ pub async fn post_opportunity( | |
.await | ||
.map_err(|e| RestError::InvalidOpportunity(e.to_string()))?; | ||
|
||
store | ||
.liquidation_store | ||
.opportunities | ||
.write() | ||
.await | ||
.insert(params.permission_key.clone(), opportunity); | ||
|
||
let mut write_lock = store.liquidation_store.opportunities.write().await; | ||
|
||
if write_lock.contains_key(¶ms.permission_key) { | ||
let opportunities_existing = &write_lock[¶ms.permission_key]; | ||
// check if same opportunity exists in the vector | ||
for opportunity_existing in opportunities_existing { | ||
if *opportunity_existing == opportunity { | ||
return Err(RestError::BadParameters( | ||
"Duplicate opportunity submission".to_string(), | ||
)); | ||
} | ||
} | ||
} | ||
|
||
if let Some(x) = write_lock.get_mut(¶ms.permission_key) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can move the logic above inside this. Please rename x to something meaningful |
||
x.push(opportunity.clone()); | ||
} else { | ||
write_lock.insert(params.permission_key.clone(), vec![opportunity]); | ||
} | ||
|
||
tracing::debug!("number of permission keys: {}", write_lock.len()); | ||
tracing::debug!( | ||
"number of opportunities for key: {}", | ||
write_lock[¶ms.permission_key].len() | ||
); | ||
|
||
Ok(OpportunityParamsWithId { | ||
opportunity_id: id, | ||
|
@@ -144,11 +173,14 @@ pub async fn get_opportunities( | |
.await | ||
.values() | ||
.cloned() | ||
.map(|opportunity| OpportunityParamsWithId { | ||
opportunity_id: opportunity.id, | ||
params: opportunity.params, | ||
.map(|opportunities_key| { | ||
opportunities_key | ||
.last() | ||
.expect("A permission key vector should have at least one opportunity") | ||
.clone() | ||
.into() | ||
}) | ||
.filter(|params_with_id| { | ||
.filter(|params_with_id: &OpportunityParamsWithId| { | ||
let params = match ¶ms_with_id.params { | ||
OpportunityParams::V1(params) => params, | ||
}; | ||
|
@@ -199,7 +231,7 @@ pub async fn post_bid( | |
Path(opportunity_id): Path<Uuid>, | ||
Json(opportunity_bid): Json<OpportunityBid>, | ||
) -> Result<Json<BidResult>, RestError> { | ||
let opportunity = store | ||
let opportunities = store | ||
.liquidation_store | ||
.opportunities | ||
.read() | ||
|
@@ -208,21 +240,19 @@ pub async fn post_bid( | |
.ok_or(RestError::OpportunityNotFound)? | ||
.clone(); | ||
|
||
|
||
if opportunity.id != opportunity_id { | ||
return Err(RestError::BadParameters( | ||
"Invalid opportunity_id".to_string(), | ||
)); | ||
} | ||
let liquidation = opportunities | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for the sake of consistent naming let's use |
||
.iter() | ||
.find(|o| o.id == opportunity_id) | ||
.ok_or(RestError::OpportunityNotFound)?; | ||
|
||
// TODO: move this logic to searcher side | ||
if opportunity.bidders.contains(&opportunity_bid.liquidator) { | ||
if liquidation.bidders.contains(&opportunity_bid.liquidator) { | ||
return Err(RestError::BadParameters( | ||
"Liquidator already bid on this opportunity".to_string(), | ||
)); | ||
} | ||
|
||
let params = match &opportunity.params { | ||
let params = match &liquidation.params { | ||
OpportunityParams::V1(params) => params, | ||
}; | ||
|
||
|
@@ -253,8 +283,12 @@ pub async fn post_bid( | |
{ | ||
Ok(_) => { | ||
let mut write_guard = store.liquidation_store.opportunities.write().await; | ||
let liquidation = write_guard.get_mut(&opportunity_bid.permission_key); | ||
if let Some(liquidation) = liquidation { | ||
let opportunities = write_guard.get_mut(&opportunity_bid.permission_key); | ||
if let Some(opportunities) = opportunities { | ||
let liquidation = opportunities | ||
.iter_mut() | ||
.find(|o| o.id == opportunity_id) | ||
.ok_or(RestError::OpportunityNotFound)?; | ||
liquidation.bidders.insert(opportunity_bid.liquidator); | ||
} | ||
Ok(BidResult { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -333,25 +333,50 @@ async fn verify_with_store(opportunity: LiquidationOpportunity, store: &Store) - | |
/// # Arguments | ||
/// | ||
/// * `store`: server store | ||
pub async fn run_verification_loop(store: Arc<Store>) { | ||
pub async fn run_verification_loop(store: Arc<Store>) -> Result<()> { | ||
tracing::info!("Starting opportunity verifier..."); | ||
while !SHOULD_EXIT.load(Ordering::Acquire) { | ||
let all_opportunities = store.liquidation_store.opportunities.read().await.clone(); | ||
for (permission_key, opportunity) in all_opportunities.iter() { | ||
match verify_with_store(opportunity.clone(), &store).await { | ||
Ok(_) => {} | ||
Err(e) => { | ||
store | ||
.liquidation_store | ||
.opportunities | ||
.write() | ||
.await | ||
.remove(permission_key); | ||
tracing::info!("Removed Opportunity with failed verification: {}", e); | ||
for (permission_key, opportunities) in all_opportunities.iter() { | ||
// check each of the opportunities for this permission key for validity | ||
let mut opps_to_remove = vec![]; | ||
for opportunity in opportunities.iter() { | ||
match verify_with_store(opportunity.clone(), &store).await { | ||
Ok(_) => {} | ||
Err(e) => { | ||
opps_to_remove.push(opportunity.id); | ||
tracing::info!( | ||
"Removing Opportunity {} with failed verification: {}", | ||
opportunity.id, | ||
e | ||
); | ||
} | ||
} | ||
} | ||
|
||
// set write lock to remove all these opportunities | ||
let mut write_lock = store.liquidation_store.opportunities.write().await; | ||
|
||
for id_opp in opps_to_remove { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need to get the opportunities twice. Also, we don't want to terminate running this function if some key is not found. You can do something like this: if let Some(opportunities) = write_lock.get_mut(permission_key) {
opportunities.retain(|x| !opps_to_remove.contains(&x.id));
if opportunities.is_empty() {
write_lock.remove(permission_key);
}
} |
||
write_lock | ||
.get_mut(permission_key) | ||
.ok_or(anyhow!("Permission key not found"))? | ||
.retain(|x| x.id != id_opp); | ||
} | ||
|
||
if write_lock | ||
.get(permission_key) | ||
.ok_or(anyhow!("Permission key not found"))? | ||
.is_empty() | ||
{ | ||
write_lock.remove(permission_key); | ||
} | ||
|
||
// release the write lock | ||
drop(write_lock); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to explicitly drop it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is good to explicitly ensure that the write lock is dropped and other processes can access the object here. otherwise, the write lock could persist through other loops of rpc calls |
||
} | ||
tokio::time::sleep(Duration::from_secs(5)).await; // this should be replaced by a subscription to the chain and trigger on new blocks | ||
} | ||
tracing::info!("Shutting down opportunity verifier..."); | ||
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.
this made more sense to me after reading this:
https://doc.rust-lang.org/book/ch15-02-deref.html