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

feat: removeorder output changed to a more meaningful message (#1526) #1958

Merged
3 commits merged into from
Nov 9, 2020

Conversation

rsercano
Copy link
Collaborator

resolves #1526

@rsercano rsercano requested review from sangaman and raladev October 23, 2020 08:54
@rsercano rsercano self-assigned this Oct 23, 2020
@raladev
Copy link
Contributor

raladev commented Oct 23, 2020

resolves #1526

it does not resolve #1526, main thing that should be done in the issue is grpc response changing (to get more useful info about removeorder action) and only after that it worth to change json output to text.

@rsercano
Copy link
Collaborator Author

But I asked about that in discord to @kilrau and he mentioned how the output should be and what issue is all about, I guess there's a misunderstanding. @raladev

https://discord.com/channels/547402601885466658/709337789304668160/769118027119198278

@rsercano
Copy link
Collaborator Author

It's not a big deal to add removedAmount into GRPC response tho. Just confirm you want it this way and I'll add. @raladev @kilrau

@raladev
Copy link
Contributor

raladev commented Oct 23, 2020

to add removedAmount into GRPC response tho. Just confirm you want it this way and I'll add. @raladev @kilrau

discussed on the meeting today, lets try to implement following structure:

  1. order is in orderbook with 20 qty
  2. swap for 5 qty is started
  3. removeorder my_id 13
{
removedQty:13;# qty that was successfully removed
remainingQty:7;# remaining qty after partial removing (should not contain quantityOnHold because it will be removed after unholding)
quantityOnHold:0; # qty in swap, but it will be removed if swap fails
}
  1. removeorder my_id (swap still in progress)
{
removedQty:2;# qty that was successfully removed
remainingQty:0;# remaining qty after partial removing (should not contain quantityOnHold because it will be removed after unholding)
quantityOnHold:5; # qty in swap, but it will be removed if swap fails
}
  1. swap is finshed
  2. removeorder my_id -> Err no order with such id

@rsercano
Copy link
Collaborator Author

Understood, I'm not sure about remainingQty and quantityOnHold logic, because it maybe requires removeOrder call to be refactored completely, but I'll just double check.

@rsercano rsercano force-pushed the feature/removeorder-output-#1526 branch from d65e593 to ba04193 Compare October 24, 2020 07:55
@rsercano rsercano requested review from raladev and removed request for raladev October 24, 2020 07:55
@rsercano
Copy link
Collaborator Author

Refactored, could you please recheck @raladev

@raladev
Copy link
Contributor

raladev commented Oct 26, 2020

  • it would be good to change satoshi in text messages to user-readeble values (1 ETH, 2 BTC) if we can get access to currency
    Order 7bf55980-1788-11eb-96ca-8f4fabe5ff7a partially removed, remaining quantity: 0, on hold: 210000000
  • there is a problem with remaining qty calculation (it is 0 when should be 1.9)
simnet > sell 3.9 ETH/BTC 0.01 1
remaining 3.9 ETH entered the order book as 3509a9c0-178f-11eb-96ca-8f4fabe5ff7a
no matches found
simnet > removeorder 1 2 -j
{
  "quantityOnHold": 0,
  "remainingQuantity": 0,
  "removedQuantity": 200000000
}
simnet > listorders

Trading pair: BTC/USDT
┌────────────────────────────────────────────────┬────────────────────────────────────────────────┐
│ Buy                                            │ Sell                                           │
├───────────────┬─────────────┬──────────────────┼───────────────┬─────────────┬──────────────────┤
│ Quantity      │ Price       │ Alias            │ Quantity      │ Price       │ Alias            │
├───────────────┼─────────────┼──────────────────┼───────────────┼─────────────┼──────────────────┤
│ 0.00518323    │ 12922.28    │ NestGrid         │ 0.34963553    │ 13449.72    │ NestGrid         │
└───────────────┴─────────────┴──────────────────┴───────────────┴─────────────┴──────────────────┘

Trading pair: ETH/BTC
┌────────────────────────────────────────────────┬────────────────────────────────────────────────┐
│ Buy                                            │ Sell                                           │
├───────────────┬─────────────┬──────────────────┼───────────────┬─────────────┬──────────────────┤
│ Quantity      │ Price       │ Alias            │ Quantity      │ Price       │ Alias            │
├───────────────┼─────────────┼──────────────────┼───────────────┼─────────────┼──────────────────┤
│               │             │                  │ 1.9           │ 0.01        │ ClusterCourse    │
└───────────────┴─────────────┴──────────────────┴───────────────┴─────────────┴──────────────────┘

  • it would be good to change text output for removeallorders command, for onhold case, becaouse it is not full truth (all not holded qty was removed). Order with id 1 was partially removed, xxx btc in hold because of active swap, it will be fully removed afterwards
simnet > removeallorders
Removed order with id 2
Order with id 1 has a hold for a pending swap and will be removed afterwards

Copy link
Contributor

@raladev raladev left a comment

Choose a reason for hiding this comment

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

above

@rsercano rsercano force-pushed the feature/removeorder-output-#1526 branch from 82295db to 1dc4425 Compare October 27, 2020 10:33
@rsercano
Copy link
Collaborator Author

I fixed first two points, for the last point I need to refactor and add fields to grpc response of removeallorders, I rather to do on a separate PR, but if @sangaman if okay with it I can implement it here too.

@rsercano rsercano requested a review from raladev October 27, 2020 10:35
@raladev
Copy link
Contributor

raladev commented Oct 27, 2020

I fixed first two points, for the last point I need to refactor and add fields to grpc response of removeallorders, I rather to do on a separate PR, but if @sangaman if okay with it I can implement it here too.

Im fine with moving third point to separate PR, it looks more correct

@rsercano
Copy link
Collaborator Author

Then let's focus on merging this first, since I'll depend on this one for it @raladev

raladev
raladev previously approved these changes Oct 27, 2020
@rsercano rsercano requested review from sangaman and removed request for sangaman October 28, 2020 05:27
@raladev raladev requested a review from a user October 30, 2020 12:17
@sangaman
Copy link
Collaborator

sangaman commented Nov 1, 2020

I'm a little confused by this feature. The "remaining quantity" and the "quantity on hold" should always be the same, right? And this quantity that's on hold will be removed unless it ends up being swapped, but perhaps that is not very clear. I'd rather we try to clarify the existing fields rather than duplicate data in the response.

Returning the quantity that was removed makes sense, but returning the pair id also seems a little unusual since presumably the caller should know the trading pair the order they're removing is for, although I suppose it doesn't hurt to have for informational purposes only.

@raladev
Copy link
Contributor

raladev commented Nov 2, 2020

I'm a little confused by this feature. The "remaining quantity" and the "quantity on hold" should always be the same, right?

remaining qty is a qty which remains in the order book after deletion, this field is not equal 0 only if u use partial removing and it does not include onhold amount that will dissapear in any case after the swap (it will be swapped or removed).

@sangaman
Copy link
Collaborator

sangaman commented Nov 2, 2020

remaining qty is a qty which remains in the order book after deletion, this field is not equal 0 only if u use partial removing and it does not include onhold amount that will dissapear in any case after the swap (it will be swapped or removed).

Ah yes, my mistake for misunderstanding. I'll review again with this in mind.

ghost
ghost previously approved these changes Nov 6, 2020
@rsercano rsercano dismissed stale reviews from ghost and raladev via 98770f0 November 9, 2020 13:57
@rsercano rsercano requested review from raladev and a user November 9, 2020 13:58
@ghost ghost merged commit c1af693 into master Nov 9, 2020
@ghost ghost deleted the feature/removeorder-output-#1526 branch November 9, 2020 14:54
@rsercano rsercano restored the feature/removeorder-output-#1526 branch November 10, 2020 14:14
This pull request was closed.
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.

feat: Change output of removeorder command/grpc call
3 participants