Skip to content

Fix order matching max volume #1041 #1047

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.

Sign up for GitHub

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

Merged
sergeyboyko0791 merged 3 commits into from
Aug 21, 2021
Merged

Fix order matching max volume #1041 #1047

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81a8ab8
Make 'test_match_maker_order_and_taker_request' red
sergeyboyko0791 Aug 20, 2021
6b0e648
Don't match a taker order if the result base volume greater than max …
sergeyboyko0791 Aug 20, 2021
2eec069
Check if the resulting base_amount doesn't violate min_base_vol
sergeyboyko0791 Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion mm2src/lp_ordermatch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1619,7 +1619,14 @@ impl MakerOrder {
&& taker_rel_amount >= &self.min_base_vol
&& taker_price >= self.price
{
OrderMatchResult::Matched((taker_base_amount / &self.price, taker_base_amount.clone()))
let base_amount = taker_base_amount / &self.price;
let rel_amount = taker_base_amount.clone();
// If `taker_base_amount == max_base_amount`, then `taker_price` has to be equal to [`MakerOrder::price`],
// otherwise the result base volume will be greater than [`MakerOrder::max_base_amount`].
if base_amount > self.max_base_vol {

@shamardy shamardy Aug 20, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move this check here https://github.com/KomodoPlatform/atomicDEX-API/blob/6b0e648b76512edb02212e01997e5f00531895ff/mm2src/lp_ordermatch.rs#L1618 as it should probably check with available_amount. Also base_amount = taker_base_amount / &self.price will always be greater than or equal to taker_rel_amount.
Also since base_amount is always greater than or equal to taker_rel_amount we can use it for this check too https://github.com/KomodoPlatform/atomicDEX-API/blob/6b0e648b76512edb02212e01997e5f00531895ff/mm2src/lp_ordermatch.rs#L1619 because the taker may have requested a lower volume than min_base_vol but the maker can offer a higher amount because the maker price is lower, so taker_rel_amount >= &self.min_base_vol may prevent matching an order that can be matched with a better price for the taker without violating the min_base_vol for the maker, a test case for this should be added too.
Please correct me if I am wrong.

@sergeyboyko0791 sergeyboyko0791 Aug 20, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is correct. I'm working on this right now, thanks :)

return OrderMatchResult::NotMatched;
}
OrderMatchResult::Matched((base_amount, rel_amount))
} else {
OrderMatchResult::NotMatched
}
Expand Down
34 changes: 34 additions & 0 deletions mm2src/ordermatch_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,40 @@ fn test_match_maker_order_and_taker_request() {
let actual = maker.match_with_request(&request);
let expected = OrderMatchResult::Matched((1.into(), 1.into()));
assert_eq!(expected, actual);

// The following Taker request has not to be matched since the resulted base amount it greater than `max_base_vol`.
// https://github.com/KomodoPlatform/atomicDEX-API/issues/1041#issuecomment-901863864
let maker = MakerOrder {
max_base_vol: MmNumber::from("0.2928826881884105"),
min_base_vol: MmNumber::from(0),
price: MmNumber::from("2643.01935664"),
created_at: now_ms(),
updated_at: None,
base: "ETH-BEP20".to_owned(),
rel: "KMD".to_owned(),
matches: HashMap::new(),
started_swaps: vec![],
uuid: Uuid::new_v4(),
conf_settings: None,
changes_history: None,
save_in_history: false,
};
let request = TakerRequest {
base: "KMD".to_owned(),
rel: "ETH-BEP20".to_owned(),
base_amount: MmNumber::from("774.205645538427044180416545"),
rel_amount: MmNumber::from("0.2928826881884105"),
action: TakerAction::Sell,
uuid: Uuid::new_v4(),
sender_pubkey: H256Json::default(),
dest_pub_key: H256Json::default(),
match_by: MatchBy::Any,
conf_settings: None,
base_protocol_info: None,
rel_protocol_info: None,
};
let actual = maker.match_with_request(&request);
assert_eq!(actual, OrderMatchResult::NotMatched);
}

// https://github.com/KomodoPlatform/atomicDEX-API/pull/739#discussion_r517275495
Expand Down