-
Notifications
You must be signed in to change notification settings - Fork 101
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
[r2r] API call optimization #1279
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.
Thanks for the PR! It's the first review iteration.
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.
Second review iteration.
mm2src/lp_ordermatch.rs
Outdated
@@ -4263,6 +4275,37 @@ async fn get_max_volume(ctx: &MmArc, my_coin: &MmCoinEnum, other_coin: &MmCoinEn | |||
)) | |||
} | |||
|
|||
pub async fn check_balance_update_loop(ctx: MmWeak, ticker: String) { | |||
let mut current_balance = None; |
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.
Current balance is known when order is created. You can add it to fn arguments and simplify it a bit as Option won’t be used.
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.
Next review iteration.
@caglaryucekaya |
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.
One change.
@caglaryucekaya Also, there are 4 tests failing that have been stable:
And these should be related to this PR changes. Please check them and fix. |
@caglaryucekaya rustfmt check is failing. |
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.
Next review iteration.
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.
Last note :)
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.
One more note.
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.
Great work! 🙂 @sergeyboyko0791 @shamardy Please take a look too.
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 think we need to add balance loop if it doesn't exist for taker orders that get converted to maker in handle_timed_out_taker_orders
, it would be good to also add a test case for this to not forget about it in the future if we were to change anything.
} | ||
ordermatch_ctx | ||
.makerorders_ctx | ||
.lock() |
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.
@artemii235 here's the lock inside the loop
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.
It's not a problem here as Taker->Maker order conversion is not that frequent :)
@sergeyboyko0791 Waiting for your review 🙂 |
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.
Amazing Work 🚀 !
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.
Great progress!
Please consider my suggestions
mm2src/mm2_main/src/lp_ordermatch.rs
Outdated
if !orderbook.order_set.contains_key(uuid) { | ||
missing_uuids.push(*uuid); | ||
} | ||
} | ||
} | ||
|
||
for uuid in missing_uuids { | ||
let order_mutex = match ordermatch_ctx.my_maker_orders.lock().get(&uuid) { | ||
let order_mutex = match ordermatch_ctx.makerorders_ctx.lock().orders.get(&uuid) { |
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 think it's better to add MakerOrdersContext::get_order
method
…i-call-optimization
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.
Only one non-critical comment. If you have time, you can fix it, otherwise we can merge this PR
mm2src/mm2_main/src/lp_ordermatch.rs
Outdated
self.balance_loops.insert(ticker.to_string(), handle); | ||
} | ||
|
||
fn spawn_balance_loop(&mut self, ctx: MmWeak, order_base: &str, balance: Option<BigDecimal>) { |
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.
It's better to take order_base: String
by value.
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.
Sure I'll fix 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.
🔥
Requests current balance only for coins with an active maker order, as part of #1269.