Skip to content
This repository has been archived by the owner on Feb 2, 2024. It is now read-only.

[Batch Viewer] Show the receiver #120

Merged
merged 4 commits into from
Jun 22, 2022
Merged

[Batch Viewer] Show the receiver #120

merged 4 commits into from
Jun 22, 2022

Conversation

matextrem
Copy link
Contributor

@matextrem matextrem commented Jun 14, 2022

Summary

Closes #106

Added receiver when it's different from the order's owner.

Screen Shot 2022-06-14 at 10 02 20

To Test

  1. Open any tx with receivers and go to batch viewer tab. (i.e tx/0x0d6f18a97690de9d70cdfe00231ee6baf722a12669f3260fb1faf3d2e25dca38
  • You will see the receiver grouped along with the owner of each order.

Example tx
Fix: https://pr120--explorer.review.gnosisdev.com/tx/0x0d6f18a97690de9d70cdfe00231ee6baf722a12669f3260fb1faf3d2e25dca38?tab=graph

Old: https://explorer.cow.fi/tx/0x0d6f18a97690de9d70cdfe00231ee6baf722a12669f3260fb1faf3d2e25dca38?tab=graph

@matextrem matextrem added Enhancement New feature or request Protofire labels Jun 14, 2022
@matextrem matextrem self-assigned this Jun 14, 2022
@coveralls
Copy link

coveralls commented Jun 14, 2022

Pull Request Test Coverage Report for Build 2518789787

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 43.872%

Totals Coverage Status
Change from base Build 2495226889: 0.0%
Covered Lines: 2162
Relevant Lines: 4177

💛 - Coveralls

@github-actions
Copy link

@ramirotw
Copy link
Contributor

Shouldn't this have all the tokens and not just SYN as input to the CoW Protocol node and then a WETH arrow from the Cow Protocol node to the receiver?

image

image

@elena-zh
Copy link

Hey @matextrem , agree with @ramirotw note.
Then, I have noticed that different tooltips appear when hover a mouse on 'SYN'

I personally find that the group name 0x84... 123c-group looks weird. Maybe we could name the block something like 'Cow Protocol orders' and show inside 'From' and 'To' labels
from to

As an enhancement, maybe we could show all orders inside the CowProtocol as a batch diagram when click on the group block?

image

@ramirotw , @alfetopito , @alongoni , WDYT?

Thanks

@matextrem
Copy link
Contributor Author

matextrem commented Jun 14, 2022

@ramirotw I noticed the same but I thought it was due to barn/prod not being integrated since I got it even before starting developing this feature.

And for the arrow coming from the Cow explorer node to the receiver, it does not exists in the transfers coming from tenderly API. Should I create it manually ?

@elena-zh each group node needs a unique id to be differentiated from each other. How should we proceed in this case ?

@elena-zh
Copy link

@matextrem , unfortunately, I'm not a tech specialist to solve these tech issues you are asking me.
While testing this or that feature, I'm proposing some enhancements how this or that thing could look better for an end user.
You can simply answer me that this or that is impossible to implement due some kind of limitations.

@ramirotw , could you please help me to answer Mati's question?

Thanks

@elena-zh
Copy link

Do we need to show 2 the same addresses in the group when Sender and Receiver is the same account?
image

@ramirotw
Copy link
Contributor

Do we need to show 2 the same addresses in the group when Sender and Receiver is the same account?
image

No, here it should be like before, no box, just 1 Trader node

@ramirotw
Copy link
Contributor

As an enhancement, maybe we could show all orders inside the CowProtocol as a batch diagram when click on the group block?

image

@ramirotw , @alfetopito , @alongoni , WDYT?

I'm pretty sure I didn't understand the enhancement you proposed @elena-zh

@ramirotw
Copy link
Contributor

ramirotw commented Jun 14, 2022

And for the arrow coming from the Cow explorer node to the receiver, it does not exists in the transfers coming from tenderly API. Should I create it manually ?

The Transfers' events are in the Tenderly response
image

image

@matextrem ☝️

@elena-zh
Copy link

As an enhancement, maybe we could show all orders inside the CowProtocol as a batch diagram when click on the group block?
image
@ramirotw , @alfetopito , @alongoni , WDYT?

I'm pretty sure I didn't understand the enhancement you proposed @elena-zh

@ramirotw , I mean not to completely hide CoW Protocol transactions in 1-2 arrows
image
but have a possibility to see all transactions by clicking on the group area (expand/open in an additional modal/etc.)
image

Thanks

@matextrem
Copy link
Contributor Author

matextrem commented Jun 16, 2022

And for the arrow coming from the Cow explorer node to the receiver, it does not exists in the transfers coming from tenderly API. Should I create it manually ?

The Transfers' events are in the Tenderly response image

image

@matextrem ☝️

@ramirotw I think those transfers represents the one going from the Cow explorer to the settlement contract (receiver), but not from the cow explorer to the receiver as a trader. If I use it to draw that line, the first edge is removed (See images below)
Screen Shot 2022-06-16 at 00 43 40

Screen Shot 2022-06-16 at 00 44 05

So I'm a bit confused at this point

Copy link
Contributor

@ramirotw ramirotw left a comment

Choose a reason for hiding this comment

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

nice!

image

@elena-zh
Copy link

Nice changes.
The only thing is that I noticed that there is no arrows for 1 node in this transaction:
no arrow

@matextrem
Copy link
Contributor Author

@elena-zh Fixed!

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Hey @matextrem , LGTM now!

I have found another edge case related to overlapping receiver node other elements, so I created a separate issue #131 if you want to fix it later.

Thanks

@henrypalacios henrypalacios merged commit d4c057b into develop Jun 22, 2022
@henrypalacios henrypalacios deleted the 106/show-receiver branch June 22, 2022 20:36
@henrypalacios henrypalacios mentioned this pull request Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[batch viewer] Show the receiver
6 participants