edits_feb5 #15
edits_feb5 #15
Conversation
anihamde
commented
Feb 7, 2024
anihamde
commented
- fix test script to accomodate recent changes
- convert opportunity store on liquidation server to list of opportunities to enable bidding on old opportunities for a permission key
- better errors on the smart contracts to make debugging way easier
- construct price updates in the monitor in MockPyth format for testing purposes
per_sdk/searcher/simple_searcher.py
Outdated
| @@ -153,6 +154,9 @@ async def main(): | |||
| tx = create_liquidation_transaction( | |||
| liquidation_opp, sk_liquidator, bid_info | |||
| ) | |||
| import pdb | |||
| match verify_with_store(opportunity.clone(), &store).await { | ||
| // just need to check the most recent opportunity, if that fails the rest should also be removed | ||
| // TODO: this is true for subsequent opportunities that only have updated price updates, but may not be true generally; we should think about how best to do this (one option is to just check every single saved opportunity and remove from the store one by one) | ||
| match verify_with_store(opportunity[0].clone(), &store).await { |
There was a problem hiding this comment.
I don't see why we can't verify each and every one of them here? and remove with the same logic.
There was a problem hiding this comment.
mostly concern around efficiency, and for opportunities where the only change is newer price updates that still maintain undercollateralization, invalidity of opp with price_update_t should imply invalidity of opp with price_update_s for s <= t.
but in general when subsequent opportunities have other changes or it's not just using pyth price feeds as TokenVault is, there may be some other dependencies and behavior (e.g. if protocol wants to use pyth price feeds with a more strict latency requirement than enforced by Pyth where it fails if using a stale price). for now will make the change to verifying each one, and we can revert later if efficiency is an issue
| } | ||
| } | ||
|
|
||
| if let Some(x) = write_lock.get_mut(¶ms.permission_key) { |
There was a problem hiding this comment.
you can move the logic above inside this. Please rename x to something meaningful
| "Invalid opportunity_id".to_string(), | ||
| )); | ||
| } | ||
| let liquidation = opportunities |
There was a problem hiding this comment.
for the sake of consistent naming let's use opportunity here as it is a single element of opportunities vector.
| } | ||
|
|
||
| // release the write lock | ||
| drop(write_lock); |
There was a problem hiding this comment.
do we need to explicitly drop it?
There was a problem hiding this comment.
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
| 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 { |
There was a problem hiding this comment.
this made more sense to me after reading this:
| // 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.
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);
}
}
