search

gnosis / gp-ui

7 stars 9 forks source link

Implementation of the batch transanction view #1061

Closed henrypalacios closed 2 years ago

henrypalacios commented 2 years ago

Summary

Closes #1058

Implement the visualization of a batch of transactions through a node graph.

Cytoscape has been used as interaction library due to its maturity and large community. But we had to apply styles in a way outside the project format.

image

To Test:

github-actions[bot] commented 2 years ago
alongoni commented 2 years ago

We are working with @henrypalacios adding some tooltips to the arrows in order to have the amounts traded by the users (Traders). Also we need to wrap the address of the AMMs.

Also, we are exploring another layout called: "cose" and it's look like this: Default view: image Expected view: image

It' ll require some testing to achieve the result.

alfetopito commented 2 years ago

Funny edge case https://pr1061--gpui.review.gnosisdev.com/tx/0x51757dd21436d695824ceb13d21ec3c3ef5e5e5454a847951b1ea91e1c89b0b7

Screen Shot 2022-03-17 at 15 30 21

EDIT: well, turns out it was my network... took a long time to load and couldn't find the token symbols. After a refresh I can see the token names loaded

Screen Shot 2022-03-17 at 15 33 01
alfetopito commented 2 years ago

Well this is a giant mess https://pr1061--gpui.review.gnosisdev.com/tx/0x172bddae0015331f4b357905ff7995389597a38294a00b4b13e90c1d70884785/ 😅

For reviewers: open https://dune.xyz/queries/205786/384521, filter by barn and sort by num trades

elena-zh commented 2 years ago

Could we add some kind of an anchor into the URL to separate what Tx view we are using? I mean, if we see Batch viewer, add #batch into the URL or so? So a user will be able to share this link and open exactly the viewer, not the list. Also, when a user updates the page, it will also show the viewer. https://watch.screencastify.com/v/V0tvqMqCZzgtUIRcsfnK

henrypalacios commented 2 years ago

Could we add some kind of an anchor into the URL to separate what Tx view we are using?

@elena-zh , as I commented to @ramirotw that requirement will be followed in another PR because we already have a considerable size in this one.

elena-zh commented 2 years ago

Could we add some kind of an anchor into the URL to separate what Tx view we are using? I mean, if we see Batch viewer, add #batch into the URL or so? So a user will be able to share this link and open exactly the viewer, not the list. Also, when a user updates the page, it will also show the viewer. https://watch.screencastify.com/v/V0tvqMqCZzgtUIRcsfnK

@henrypalacios , here is a separate task for this #1072

anxolin commented 2 years ago

Amazing job, really! 🎉

Some quick feedback:

image

One important thing, which can be out of the scope, but i'd love we take it into account at some point is that the way to see the chart is hidden and hard to find.

image

We discussed the other day with Michel, and one idea is to show, for example in the header, a summary of the batch: i.e. Traded volume, number of trades, and liquidity used (maybe we can even show sow the % of liquidity as a pie chart showin the AMMs) Then below the pie chart, would be a “view details” to see the chart

alongoni commented 2 years ago

Thanks for the feedback @anxolin !

Amazing job, really! tada

Some quick feedback:

  • Edges: Can we add the logo for the token if available?

We need to try a solution for that, natively Cytoscape doesn't support images in the edges.

  • Nodes: can we make users to have funny cow icons :)

I've added this cow :) image

  • Nodes: can we make AMMs have their respective logo?

@henry and @ramirotw are analyzing it.

  • [nice to have] Network: We can add a logo of the network too

image

One important thing, which can be out of the scope, but i'd love we take it into account at some point is that the way to see the chart is hidden and hard to find.

image

We discussed the other day with Michel, and one idea is to show, for example in the header, a summary of the batch: i.e. Traded volume, number of trades, and liquidity used (maybe we can even show sow the % of liquidity as a pie chart showin the AMMs) Then below the pie chart, would be a “view details” to see the chart

It's true. Maybe I can work designing those components and access to the batch viewer.

In general, we think there are many things we can improve. Maybe we can handle it in a second iteration. E.g:

elena-zh commented 2 years ago

Great changes! I have reported separate big issues #1074 and #1075 related to the current task. As for minor issues I have found, here they are:

  1. I think it would be nice to write a network name from the capital letter image image

  2. As for a trader icon, from my perspective lights from the cow eyes look a bit strange to me. However, might be skipped if is looks nicely to everyone else. image

  3. Can we make token labels larger a bit? This schema is opened in a large screen, but token labels are almost not readable image

  4. Do we really need a clicked state on arrows/icons? image image

  5. As for amounts, I think we should use the same rounding rules as we use for the table. I mean, if we show an amount rounded to 8 decimals in the table view (example 0.00025354 ), we should be able to see this amount in the batch viewer schema. Currently, it is rounded to 3-4 decimals <0.001, and it does not reflect the actual amount image image image

Tx details for this example: https://pr1061--gpui.review.gnosisdev.com/rinkeby/tx/0xf7d7751a1ee17387c1b3bdb2bd16a644a2d112d09cf39ec73729aa30ec406e73

Let me know please if I need to create a separate issue to one of these comments.

Thanks

henrypalacios commented 2 years ago

@elena-zh Regarding your previous message :point_up:

  1. I think it would be nice to write a network name from the capital letter

:heavy_check_mark: done!

  1. As for a trader icon, from my perspective lights from the cow eyes look a bit strange to me. However, might be skipped if is looks nicely to everyone else.

Looks like a funny cow, indifferent here!

  1. Can we make token labels larger a bit? This schema is opened in a large screen, but token labels are almost not readable

I will review it with @alongoni.

  1. Do we really need a clicked state on arrows/icons?

What is your suggestion at this point? make desktop hover only? Or do you mean the shading of the element?

  1. As for amounts, I think we should use the same rounding rules as we use for the table. I mean, if we show an amount rounded to 8 decimals in the table view (example 0.00025354 ), ...

:heavy_check_mark: done!

elena-zh commented 2 years ago

Hey @henrypalacios !

As for a trader icon, from my perspective lights from the cow eyes look a bit strange to me. However, might be skipped if is looks nicely to everyone else.

Looks like a funny cow, indifferent here! I do not insist on this as it was my personal impression! =)

Do we really need a clicked state on arrows/icons?

What is your suggestion at this point? make desktop hover only? Or do you mean the shading of the element? Could we remove these on-click effects at all? Frankly, I can't understand why we need them.

Also, I can't test your changes due to failed build.. =((

Btw, what a nice view for the transaction https://pr1061--gpui.review.gnosisdev.com/tx/0x172bddae0015331f4b357905ff7995389597a38294a00b4b13e90c1d70884785/ ! image

Can we align diagram horizontally if something like this happens?

elena-zh commented 2 years ago

@henrypalacios , I have verified the latest changes! Network names, rounding, and increased token names LGTM! Maybe another nitpick I could mention is that it would be better to fill in the whole screen with the diagram area image

anxolin commented 2 years ago

As as a good to have feature, for future reiterations, do you think we can make the arrows reflect if they are a sell or buy orders?

Normally GREEN reflects a buy, RED a sell. We could use color either in the arrow or the user or somewhere we could tell which ones are what.

alongoni commented 2 years ago

@henrypalacios , I have verified the latest changes! Network names, rounding, and increased token names LGTM! Maybe another nitpick I could mention is that it would be better to fill in the whole screen with the diagram area image

Hey @elena-zh thanks! Is your zoom at 100%? Maybe that could be the cause. https://www.loom.com/share/4c376579ba204d17a32fd3f02d4de014

elena-zh commented 2 years ago

Hey @alongoni , I have fund the root cause of the issue: the diagram area is not auto-scalable. I mean, if I drag and drop a browser's tab with the diagram from a smaller screen to a bigger one, I will get this issue. See the video: https://watch.screencastify.com/v/u2oEJODe2166zN7faBdz

But it is a low-low priority issue, that is why I created a separate task for this #1084

elena-zh commented 2 years ago

As as a good to have feature, for future reiterations, do you think we can make the arrows reflect if they are a sell or buy orders?

Normally GREEN reflects a buy, RED a sell. We could use color either in the arrow or the user or somewhere we could tell which ones are what.

I have created a separate enhancement task for this #1085

henrypalacios commented 2 years ago

Hey @elena-zh, @ramirotw as I mentioned I submitted changes to mitigate issues #1074(pt. 1), #1084 in this first iteration.

So we will continue working on the corresponding issues on a next iteration.