Skip to content

Fix order matching max volume #1041#1047

Merged
sergeyboyko0791 merged 3 commits intodevfrom
fix-order-matching-max-volume
Aug 21, 2021
Merged

Fix order matching max volume #1041#1047
sergeyboyko0791 merged 3 commits intodevfrom
fix-order-matching-max-volume

Conversation

@sergeyboyko0791
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Thank you for the fix :)
I have only one comment.

Comment thread mm2src/lp_ordermatch.rs Outdated
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 {
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.

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 :)

* Extend test_match_maker_order_and_taker_request
Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Approved! 🔥

@sergeyboyko0791 sergeyboyko0791 merged commit 0329a88 into dev Aug 21, 2021
@sergeyboyko0791 sergeyboyko0791 deleted the fix-order-matching-max-volume branch August 21, 2021 05:13
Copy link
Copy Markdown

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I have one question.

Comment thread mm2src/lp_ordermatch.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: ETH-BEP20 maker oders using setprice with + max are not always startable

3 participants