Skip to content
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

fix(dex-api): best orders proper is_mine provided #1849

Merged
merged 16 commits into from
Jun 15, 2023

Conversation

rozhkovdmitrii
Copy link

@rozhkovdmitrii rozhkovdmitrii commented May 30, 2023

best_orders rpc returns is_mine: false for own orders.
Changes fix this bug and also introduce an optional exclude_my rpc field that exclude orders marked as is_mine from response

@rozhkovdmitrii rozhkovdmitrii added in progress Changes will be made from the author bug fixing week 2 labels May 30, 2023
@rozhkovdmitrii rozhkovdmitrii self-assigned this May 30, 2023
@rozhkovdmitrii rozhkovdmitrii linked an issue May 30, 2023 that may be closed by this pull request
@rozhkovdmitrii rozhkovdmitrii added the bug Something isn't working label May 30, 2023
@rozhkovdmitrii rozhkovdmitrii added under review and removed in progress Changes will be made from the author labels May 30, 2023
@rozhkovdmitrii rozhkovdmitrii marked this pull request as ready for review May 30, 2023 16:07
@shamardy
Copy link
Collaborator

@rozhkovdmitrii I agree with this comment #1015 (comment) that we should exclude orders if they belong to the node from best_orders response instead of setting is_mine to true, what do you think?

@rozhkovdmitrii
Copy link
Author

rozhkovdmitrii commented May 31, 2023

@rozhkovdmitrii I agree with this comment #1015 (comment) that we should exclude orders if they belong to the node from best_orders response instead of setting is_mine to true, what do you think?

Excuse me for having a contrary opinion.
This could certainly be an option, but the truth is that we have a protocol that is already nominated as API 2.0 (Master) containing is_mine as a part of response. Without the right to waver and spoil it leading our own (as a team) arbitrariness. Continuing the thought, another argument is that the protocol assumes that RPC API clients can exclude orders if they think it is ought to be. These use cases are implicitly assumed by the inclusion of is_mine as part of the protocol.

What I wanted to say is if we leave is_mine it doesn't hurt anything, though if we don't it does.
This is why I would keep the protocol consistent, transparent, simple and stupid.

@shamardy
Copy link
Collaborator

Right now we go through all orders in best orders and check if they are is_mine or not, I believe this process is repeated in GUIs to filter my orders out. I agree that keeping is_mine to find how my orders rank in best orders can be beneficial too, how about adding a parameter in BestOrdersRequestV2 called exclude_my_orders which defaults to false to keep the original functionality intact if this new parameter was not included in the request?

@rozhkovdmitrii
Copy link
Author

Right now we go through all orders in best orders and check if they are is_mine or not, I believe this process is repeated in GUIs to filter my orders out. I agree that keeping is_mine to find how my orders rank in best orders can be beneficial too, how about adding a parameter in BestOrdersRequestV2 called exclude_my_orders which defaults to false to keep the original functionality intact if this new parameter was not included in the request?

Sure ) - extending is not reducing

@rozhkovdmitrii
Copy link
Author

rozhkovdmitrii commented Jun 1, 2023

Sure ) - extending is not reducing

@shamardy

Dared to use an abridged version, if you allow me of course

[]$ adex-cli best-orders --number 100 MORTY sell
Getting best orders: Sell MORTY
┌───┬───────┬──────────────────────────────────────┬────────────────────┬────────────────────┬────────────────────────────────────┬───────────────────┐
│   │ Price │ Uuid                                 │ Base vol(min:max)  │ Rel vol(min:max)   │ Address                            │ Confirmation      │
│ RICK                                                                                                                                                │
│ * │ 0.50  │ 1fc0df20-9c21-461a-ad78-a4b37d4ab336 │ 0.00020:1.00       │ 0.000100:0.50      │ RPFGrvJWjSYN4qYvcXsECW1HoHbvQjowZM │ 111,true:555,true │
│   │ 1.00  │ 7cb4735b-0c1e-4c55-a58e-35699e26e194 │ 0.000100:0.65      │ 0.000100:0.65      │ R9gWj7fzSxZtJZCSDMQz5G5J7x4rg6UmiQ │ 1,false:1,false   │
│   │ 1.00  │ 880fadcb-ec7e-40dc-9299-6957dd8ce467 │ 0.000100:1.99      │ 0.00010:2.00       │ RMaprYNUp8ErJ9ZAKcxMfpC4ioVycYCCCc │ 1,false:1,false   │
│   │ 0.04  │ 9199a9f3-d0ca-4746-b99a-438bf85f9204 │ 0.0022:42.77       │ 0.000100:1.94      │ RJVrcGS1H8LKqdFxbreTcRzpGiWsHQZDpc │ 1,false:1,false   │
│ * │ 0.90  │ 9664d472-0313-494c-9a03-f8cd5d4f7e5b │ 0.00011:1.00       │ 0.000100:0.90      │ RPFGrvJWjSYN4qYvcXsECW1HoHbvQjowZM │ 1,false:1,false   │
│   │ 1.00  │ eeb888af-5392-4b1a-a4a1-28250d6c3c7e │ 0.000100:363783.58 │ 0.000100:363783.58 │ RB8yufv3YTfdzYnwz5paNnnDynGJG6WsqD │ 1,false:1,false   │
│ ZOMBIE                                                                                                                                              │
│   │ 1.00  │ b1d92c46-b782-4dea-9825-f009708627d7 │ 0.000100:2.00      │ 0.000100:2.00      │ Shielded                           │ 1,false:1,false   │
└───┴───────┴──────────────────────────────────────┴────────────────────┴────────────────────┴────────────────────────────────────┴───────────────────┘
[]$ adex-cli best-orders --number 100 MORTY sell --exclude-my
Getting best orders: Sell MORTY
┌──┬───────┬──────────────────────────────────────┬────────────────────┬────────────────────┬────────────────────────────────────┬─────────────────┐
│  │ Price │ Uuid                                 │ Base vol(min:max)  │ Rel vol(min:max)   │ Address                            │ Confirmation    │
│ RICK                                                                                                                                             │
│  │ 1.00  │ 7cb4735b-0c1e-4c55-a58e-35699e26e194 │ 0.000100:0.65      │ 0.000100:0.65      │ R9gWj7fzSxZtJZCSDMQz5G5J7x4rg6UmiQ │ 1,false:1,false │
│  │ 1.00  │ 880fadcb-ec7e-40dc-9299-6957dd8ce467 │ 0.000100:1.99      │ 0.00010:2.00       │ RMaprYNUp8ErJ9ZAKcxMfpC4ioVycYCCCc │ 1,false:1,false │
│  │ 0.04  │ 9199a9f3-d0ca-4746-b99a-438bf85f9204 │ 0.0022:42.77       │ 0.000100:1.94      │ RJVrcGS1H8LKqdFxbreTcRzpGiWsHQZDpc │ 1,false:1,false │
│  │ 1.00  │ eeb888af-5392-4b1a-a4a1-28250d6c3c7e │ 0.000100:363783.58 │ 0.000100:363783.58 │ RB8yufv3YTfdzYnwz5paNnnDynGJG6WsqD │ 1,false:1,false │
│ ZOMBIE                                                                                                                                           │
│  │ 1.00  │ b1d92c46-b782-4dea-9825-f009708627d7 │ 0.000100:2.00      │ 0.000100:2.00      │ Shielded                           │ 1,false:1,false │
└──┴───────┴──────────────────────────────────────┴────────────────────┴────────────────────┴────────────────────────────────────┴─────────────────┘

caglaryucekaya
caglaryucekaya previously approved these changes Jun 1, 2023
Copy link

@caglaryucekaya caglaryucekaya left a comment

Choose a reason for hiding this comment

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

LGTM! Can you also add a test case in best_orders_tests to check if the exclude_mine and is_mine are working as intended?

mm2src/mm2_main/src/lp_ordermatch/best_orders.rs Outdated Show resolved Hide resolved
@rozhkovdmitrii rozhkovdmitrii added in progress Changes will be made from the author and removed under review labels Jun 5, 2023
@rozhkovdmitrii rozhkovdmitrii force-pushed the 1015-best-orders-is_mine branch 3 times, most recently from 8bbba7f to 174ff34 Compare June 5, 2023 15:09
@rozhkovdmitrii rozhkovdmitrii added under review and removed in progress Changes will be made from the author labels Jun 5, 2023
caglaryucekaya
caglaryucekaya previously approved these changes Jun 5, 2023
Copy link
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.

Thanks a lot for the fix! I have a few comments.

mm2src/mm2_main/src/lp_ordermatch/best_orders.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/lp_ordermatch/best_orders.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/lp_ordermatch/best_orders.rs Outdated Show resolved Hide resolved
Copy link
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.

Last 2 minor comments :)

mm2src/mm2_main/src/lp_ordermatch.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/lp_ordermatch/best_orders.rs Outdated Show resolved Hide resolved
Copy link
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.

One more last comment!

mm2src/crypto/src/crypto_ctx.rs Outdated Show resolved Hide resolved
Copy link
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.

Thanks a lot for your patience with the fixes! LGTM!

@shamardy shamardy merged commit 2012d14 into dev Jun 15, 2023
@shamardy shamardy deleted the 1015-best-orders-is_mine branch June 15, 2023 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] best_orders returns incorrect is_mine
4 participants