Trades view proposal

[alfetopito]

Update (Jun 1):

Couple improvements:

• Shuffled columns around
• Display data for deleted orders (NaN limit price on some trades)
• CSV download!
• (Ugly) order type pills
• Showing full date, prices and amounts on hover
• Relative date
• Updating trades only once per batch, after it is finalized
• Many minor refactors/improvements

Update (May 27):

Page is working, loading trade events with the minimal functionality needed.

Ready for testing with your own trades on https://pr1012--dexreact.review.gnosisdev.com/trades/
(remember to connect your wallet)

To be decided based on feedback on this PR:

• Where to put this page?
• Are the current columns ok?
• Styling
• Call it Fills instead of Trades

Follow ups to this PR

• Trade reverts:
  • Show reverted trades for recent batches
  • Do not show reverted trades for past batches (>5 batches ago?)
• [Good to have] Order detail page
• [Good to have] Link trade to order detail page
• [Good to have] Download as csv

---

⚠️ Styling is non-existent. Just a very crude (and ugly) mock ⚠️

Proposal for a Trades view

Added a very basic table with ideas for a trades view (page or tab, to be decided).

For now listing a few fields I think might be useful for users.

One idea as to where to put this is as an additional tab on the Trade page, such as:

(Keep in mind the image above is not in the PR)

The current mock is under /trades path.

Notes

1. Settled column indicates whether the trade is final or can still be reverted (batch still open for submissions). Possibly display the countdown for how long until settlement.
2. I considered showing the batchId but this info is not displayed anywhere else in the UI, so doesn't make sense to have it here.
3. We could have for each trade a link to the transaction where it originated (although I don't see much value as a user other than transparency)
4. Still considering what to display - if should display anything at all - when a trade is reverted
5. In a regular exchange scenario, I would expect a link from the trade to an order, but here we don't have that. We could show the orderId, but like batchId, this is not displayed anywhere else. Might not be worth starting it now.
6. Alternatively, we could link each trade (and the orders as well) to an Order detail page where one could see all trades/reverts for a single order, and be able to cancel it too.

---

While we discuss this proposal, I'll fiddle around with blockchain events to populate this table.

[ghost]

Travis automatic deployment:
https://pr1012--dexreact.review.gnosisdev.com

[anxolin]

Amazing proposal.

1. Settled column indicates whether the trade is final or can still be reverted (batch still open for submissions). Possibly display the countdown for how long until settlement.

Very good suggestion!

2. I considered showing the batchId but this info is not displayed anywhere else in the UI, so doesn't make sense to have it here.

👌

3. We could have for each trade a link to the transaction where it originated (although I don't see much value as a user other than transparency)

I think we should do it. It offers transparency, and a user can start to understand how his trade "magically" happens because someone submits a solution

4. Still considering what to display - if should display anything at all - when a trade is reverted

Depending on the effort, but ideally yes for a recent batch, because otherwise, the user would just see the trade disapear or change slightly his values. Offers more transparency in how the protocol work.
However, I don't see the point for reverted trades of some batches ago, so I wouldn't show something that is a few batches old.

I would say, reverted trades it's a "good to have"

5. In a regular exchange scenario, I would expect a link from the trade to an order, but here we don't have that. We could show the orderId, but like batchId, this is not displayed anywhere else. Might not be worth starting it now.

Probably for now we can keep it simple and not show it.
A good to have is a link to the order --> it would take us to the orders tab and highlight the row. But I would say that is not that important for a first version if it makes things complicated.

6. Alternatively, we could link each trade (and the orders as well) to an Order detail page where one could see all trades/reverts for a single order, and be able to cancel it too.

This would be amazing.

---

Not from this proposal, but it looks that filters by tokens, dates, filled/unfilled are really good aditions, both for ORDERS and for TRADES.

By the way, @Rafanator likes to call TRADES "Filled", probably is better.

Also, instead of Bough, I would use "Received" to be coherent with the trading widget.

Regarding the dates, I see UTC, I guess we are showing the local time of the user, right?

The layout, I would ask @biocom, but I would vote for get rid of the Liquidity orders here, and move it to the Liquidity section (there's a proposal already) and use the space for the "Filled"

Lastly, although we don't show the order id and the batch id, but I think it's a good idea to at least add it hidden in the HTML5 data in the row (to easily check this thigns): data-batch-id, data-order-id.

[anxolin]

☝️comments above (this is just to tell github, I already reviewed so it don't show up anymore)

[c3rnst]

Hi Leandro - looks smooth and clearer, thanks!!
Just one thing that came up a couple of times over the past week: In the your trades tab, could it also show the price the user wanted? ("Limit price") (as one of the value proposition is, that the final price might indeed be better than the limit price? :)
Just another thought, which has NO priority at all but a though occurred to me: as this is the trade history for the user, possibly for accounting purposes, it might be useful to download it. Is there an easy way to make this a downloadable csv file?

[Rafanator]

Very nice Leandro!

Regarding what Anxo said, I think a more proper term to use here is "Fills" instead of "Trades".

Fills is often used in most trading platforms, alternatively, you could use Trade History but it is too long, I'd go for Fills. Trades by itself is a more generic term that can lead to confusion.

Using fills enables you to have a relevant Type column, where you can indicate Partial and Filled/Full..

Here is DY/DX as an example of the columns in their Fills tab:

In the context of GP, I would suggest that our Trades (hopefully soon Fills) tab has the following columns and order:

Limit Price | Fill Price | Amount | Type | Time | Transaction

Limit Price -> Limit Price of the original order (i.e. 150 WETH/DAI)
Fill Price -> Price at which the order got filled (even if it didn't get filled in its entirety)
Amount -> Amount of the order that got filled, expressed in the sell token (If user wants to buy 1000 DAI worth of WETH at 150 and only 500 Dai of the order get filled, you would display Amount -> 500 Dai)
Type -> Partial or Full
Time -> Date and time of fill
Transaction -> Transaction ID/url to etherscan

Observations:

• I would discard the 'Bought' amount included by @alfetopito , as a user can check it in its balance and it is not common to show bought amount in most trading platforms.

• I add 6 columns instead of 5 originally suggested by Leandro. If we are to disregard one because of space constraints, I would disregard Time. Transaction probably looks nicer if we just add a link to etherscan. Users would also find the time of their fill by checking that info.

I hope this makes sense :)

[alfetopito]

Lot's of great feedback, thank you all!

Regarding the dates, I see UTC, I guess we are showing the local time of the user, right?

@anxolin sure, was just an example of showing the full date time as opposed to what we have else where which is "x minutes ago" etc.

as this is the trade history for the user, possibly for accounting purposes, it might be useful to download it. Is there an easy way to make this a downloadable csv file?

@c3rnst we sure can do that. I'll split this into a separated task though, to not over extend the scope of this one

Type -> Partial or Full

@Rafanator that information is not part of the event (as well as the limit price). We can of course get the data from the original order. May or may not have a performance impact. I'll try it out.

Also, in your suggested columns Rafa, you don't mention Settled. Would you rather not have it or are you indifferent?

Summary:

• Show countdown for Settled while batch is still open for solutions

• Add link to transaction on etherscan

• [Good to have] Show reverted trades for recent batches

• [Good to have] Do not show reverted trades for past batches (>5 batches ago?)

• [Good to have] Order detail page

• [Good to have] Link trade to order detail page

• Show date on local time -- possibly hide date if low on space

• Add batchId, orderId etc as data props

• Show limit price

• [Good to have] Download as csv

• Call it Fills instead of Trades

• Columns now (not clear on Settled yet):

  Limit Price | Fill Price | Amount | Type | Time | Tx | Settled

[anxolin]

Rafa knows more about trading than me, I just want to point out, that regarding this:

I would discard the 'Bought' amount included by @alfetopito , as a user can check it in its balance and it is not common to show bought amount in most trading platforms.

This is what they show in Changly:

Also, that regarding this:

, I would disregard Time. Transaction probably looks nicer if we just add a link to etherscan. Users would also find the time of their fill by checking that info

For me time is important, I don't understand why would we want to remove it. You shouldn't be having to click on all the fills and check the date in Etherescan for see if the last fills are from today or not.

This is for example Kraken, they show fills too. By the way, they call them trades 🤷‍♂️

[alfetopito]

Thanks for checking it out, Ben.
I did not get time to fix this today, but I'm thinking of how to address it.

NaN is definitely not good. I could for now just say N/A, but that is not optimal.
Rather than leave it empty, I think we can fetch this data, from the Order placement event.
Need to investigate further how to query that, shouldn't be too hard.

@bh2smith the latest version should not show you any more NaNs.
Fetching event data (for deleted orders) from OrderPlacement events.

[Velenir]

There may be a better way
we can make calls to check contract state at a specific block
You have blockNumber from TradeEvent and Order corresponding to that event must be active at that point, of course. So you can do

contract.methods.getEncodedUserOrders(userAddress).call({}, blockNumber)
// then you'll have to decode orders as usual and find the one you need

But maybe even better since we have paginated orders and know orderId

exchangeApi.getOrdersPaginated({
 userAddress.
 networkId,
 offset: orderId,
 pageSize:1,
}, blockNumber) // fetches that exact order as it was at that block

I wonder if to have fewer fetches we could, if we have multiple deleted orders close by in id, fetch them all together with appropriate pageSize, but probably not, cause will be hard to guess at what blockNumber they all would be alive.

This is much cheaper (probably) than getting a bunch of unindexed events.

blockNumber isn't typed though, it's not even in the docs
So try with //@ts-ignore first and if it works for you, once again I'll need to hack around types in dex-js, ugh...

[alfetopito]

Well, if that's the case, I could use the getOrder directly with the blockNumber.
Then orders will never be empty and I can skip this additional step entirely.

I'll give this a try 👍

[alfetopito]

Yes, that does work!

No need to check for this event then, I'll remove the method getOrderFromOrderPlacementEvent and this extra hop.

And, as expected, the build does fail:

ERROR in /home/alfeto/work/dfusion/dex-react/src/api/exchange/ExchangeApi.ts
ERROR in /home/alfeto/work/dfusion/dex-react/src/api/exchange/ExchangeApi.ts(347,83):
TS2554: Expected 0-1 arguments, but got 2.

[alfetopito]

Several layers of ignore later :D
1cb9671

I'm wondering if I should still leave the check for empty orders there as an additional check.

But maybe not. There shouldn't be a case that a Trade event is emitted while an order does not exist on the contract.

[anxolin]

@alfetopito this PR is getting too big and difficult to review the new things.

I'm quite happy with the current state.
What do you think is about merging and doing small PRs to address the pending issues?

[anxolin]

Not for this PR, but once it's merge we can style it further.

I did some clumsy test, and I think it looks better with a bit more of air/padding for the rows:

Also I tried to play a bit with the pills.
In my test, I added:

    display: inline-block;
    font-size: 0.9rem;
    padding: 0.2rem 0.8rem;
    min-width: 5em;
    font-weight: 800;
}

Also, probably we'll have to do something with the date. "about 2 months ago" is a bit long.
Maybe with some stylign we make it look better.

We should address mobile independently, maybe @W3stside can help here

[alfetopito]

As Anxo suggested, merged branch as it currently is. Too big to review, will address comments in new PRs.