lukechilds / hyperdex-bugtracker

0 stars 1 forks source link

Swap list filter on the Exchange tab uses incorrect pair #15

Closed jansako closed 6 years ago

jansako commented 6 years ago

The larger issue is to account for the existence of separate pairs. REVS/KMD is not the same pair as KMD/REVS as far as MM is concerned.

The way the issue manifests now is that when making trades on REVS (left) to KMD (right), the trades are displayed as KMD/REVS on the list of trades. When the user tries to filter by clicking the REVS/KMD button, ALL trades disappear from the list, because they are KMD/REVS, and not REVS/KMD.

Grandma friendly check: I am using the Komodo on the right side, because that is where it comes up by default & switch the other traded coin on the left. (feels more natural too)

See screenshots. This is the trade list: image

When clicking on the REVS/KMD filter: image

Let me know if you need me to clarify.

If Hyperdex is able to correctly handle MM data pertaining to both pairs and display/interact with it in a single screen, that would be a great step from the usability standpoint!

lukechilds commented 6 years ago

I have actually noticed this and have it on a list to fix but havan't got round to it. It's actually (in my opinion) a marketmaker bug. marketmaker has no concept of buys and sells. Just orders.

If we place a buy on KMD/CHIPS, marketmaker posts it as KMD/CHIPS. If we post a sell on KMD/CHIPS, marketmaker inverts the values round and places it as CHIPS/KMD. We can't change that, that's marketmaker behaviour.

We can definitely handle it better though. I think changing the filter so instead of only listing BASE/QUOTE we show BASE/QUOTE and QUOTE/BASE. That will allow you to see all buys and sells for that pair.

lukechilds commented 6 years ago

The way the issue manifests now is that when making trades on REVS (left) to KMD (right), the trades are displayed as KMD/REVS on the list of trades.

Wait, do you mean to say you are on REVS/KMD and you're placing a buy order and it's coming through as KMD/REVS?

Or are you placing sell orders?

jansako commented 6 years ago

I am placing a 'Sell Revs' order. Sorry for not clarifying, it really should not matter though. I am expecting the pairs on the UI to match the pairs on the Swaps list and the filter. Any trades done on REVS (left side of UI) to KMD (right side of UI) should show up as REVS/KMD on the Swap list (right now they show inverted as KMD/REVS), and the filter "REVS/KMD" shown on the filters list should filter them correctly (which it does not right now).

I agree that it comes from the 'order type agnistic behavior', just not sure it is a bug :-) I recall discussions with JL about how this is a feature (bug/feature is a matter of opinion, I know), working as designed to allow total freedom to swap whatever for whatever else. You and I are probably more concerned with general usability & UX, so this one may need to be handled (limited/dumbed down) on the GUI level... if you can translate the pairs & amounts to show correctly. Unless the MM v2 is changing it , of course :-)

lukechilds commented 6 years ago

Any trades done on REVS (left side of UI) to KMD (right side of UI) should show up as REVS/KMD on the Swap list (right now they show inverted as KMD/REVS), and the filter "REVS/KMD" shown on the filters list should filter them correctly (which it does not right now).

Totally agree

I agree that it comes from the 'order type agnistic behavior', just not sure it is a bug :-)

I consider it a bug because we send:


{
    "method":"sell",
    "base":"CHIPS",
    "rel":"KMD",
    "basevalue":1.37297157,
    "relvalue":0.1,
    "price":0.07283472
}

And we receive:

{  
  "uuid":"52e5d0b71ff399574b9a7ff427c27d2d8308f85504fae4b460ff72b83e525309",
  "base":"KMD",
  "rel":"CHIPS",
  "basevalue":0.10020747,
  "relvalue":1.37307157
}

As you can see the base/rel values get swapped around between the request and response when we do a sell but not a buy. This is clearly bad API design, it's unintuitive and kind of awkward to handle.

I did intentionally add the "IMO" in my previous comment:

It's actually (in my opinion) a marketmaker bug.

because I know it's kind of controversial. Not technically a bug, just unintuitive behaviour. Maybe I should change the GitHub label from "marketmaker bug" to "marketmaker related".

But anyway, I completely understand your point, we're aware of it, and we can work around the API quirks to make sure we're displaying it in a more user friendly manner.

One potential solution:

We could add a "buy/sell" property to the swap list.

So on the CHIPS/KMD trading pair:

And then on the KMD/CHIPS trading pair:

Thoughts?

Also, @sindresorhus, would be good to have your thoughts on this too.

jansako commented 6 years ago

Here are some thoughts:

jansako commented 6 years ago

Just noticed, the + and - columns seem incorrect in relation to the coin pair displayed :-)

If we are showing KMD/REVS pair, the following column should be the change in KMD amount, then change in REVS amount. Basically either swap the KMD/REVS description to REVS/KMD, or swap the two 'deltas' columns. My preference would be the latter & always have the KMD change in the first column.

As a general rule, we should be consistent to display the 'more likely' reference currency in the same position for the sake of consistency. I understand it does depend on how much calculations do you want Hyperdex to be doing on the front-end as opposed to displaying whatever MM spits back out at us.

jansako commented 6 years ago

wrong button... sorry!

lukechilds commented 6 years ago

swap the two 'deltas' columns

THIS!

Thanks, this is the perfect solution. Then you can easily visually differentiate between buys and sells by the colour of the first value.

I think we should keep the pair as marketmaker reports it though, which means sells will display as the inverse pair. I know it seems counter intuitive but this is how marketmaker works under the hood.

We obviously want to hide as much of the complexity as possible from the user, but completely faking the order book value just to make it seem more intuitive is too far IMO. As far as marketmaker is concerned KMD/REVS orders are not REVS/KMD orders.

Currently this is very confusing, but I think with the UI tweak you mentioned that will avoid the confusion.

Let's go with this for now, I'll try and get it implemented in the next test release. Then you can let us know if you still think there's an issue.

lukechilds commented 6 years ago

@jansako I've just updated it to work like this:

screen shot 2018-05-09 at 9 10 28 am

Regarding your point:

always have the KMD change in the first column.

I agree, always having the base currency of the active trading pair in the first column would be great from a UX perspective, because you just glance at the first column and whether it's green or red indicates a buy or sell. However this doesn't actually make a lot of sense and isn't consistent.

For example that means whe're changing the swap row Component based on external properties (the current screen trading pair). Which means if you flip your trading pair all the values will flip. But only on the filter view. On the "All" view it doesn't make sense to flip them because it'll include swaps from other pairs of which we aren't on a pair for we just have to show them the default base/quote order.

I think the above screenshot actually makes the most sense with the data we have. What are your thoughts?