Skip to content
This repository was archived by the owner on Apr 4, 2022. It is now read-only.

[Explorer] Custom hook for getting explorer data for BlockExplorerLink component when it is not a gpv2 tx #930

Merged
merged 16 commits into from
Mar 2, 2022

Conversation

matextrem
Copy link
Contributor

Summary

Closes #857

Added a custom hook useTxOrderExplorerLink that returns the data needed to be used in BlockExplorerLink component in case it is not a Gpv2 tx otherwise returns an empty object.

To Test

Since tx table is not ready yet, there's no UI to reproduce this behaviour. You can test it by importing useTxOrderExplorerLink from hooks/useGetOrders and use it with a tx hash. i.e:
const explorerLinkData = useTxOrderExplorerLink('0xd51f28edffcaaa76be4a22f6375ad289272c037f3cc072345676e88d92ced8b5')

And then use this data in BlockExplorerLink component:

<BlockExplorerLink {...explorerLinkData} />

@matextrem matextrem added the Protofire task to the protofire team label Dec 27, 2021
@matextrem matextrem self-assigned this Dec 27, 2021
@github-actions
Copy link

@matextrem matextrem requested a review from alfetopito January 12, 2022 13:27
@elena-zh
Copy link

Hey @matextrem , can we finish this task?

@matextrem
Copy link
Contributor Author

@elena-zh Sure. Do we have a design for this? Where will the link be located?

@henrypalacios
Copy link
Contributor

I'm in doubt as to how to evaluate this PR, since I'm not sure where this hook should be implemented in the first instance.

Would be in the link from the detail view?
Selection_459

Maybe @alfetopito can clarify it.

@elena-zh
Copy link

@matextrem , the only thing I could clarify is that we have already discussed it in the 'parent' issue: #857 (comment)

@matextrem
Copy link
Contributor Author

@elena-zh regarding that comment, the logic is already done. The only thing left is where to show the Explorer link.

@alfetopito
Copy link
Contributor

This should be used when searching for a tx by hash.

The steps are basically:

  1. Search by a string
  2. If it matches txHash regex, checks whether it's a protocol tx
  3. If it is, show the tx view
  4. Else, check from which network it is (this hook)
  5. If it matches any network, return a link to etherscan for given tx
  6. Else, search not found.

For example, if I search right now for a non-protocol tx I get a not found https://explorer.cow.fi/search/0x46abe93685cc328f908f44695cd2cc2e65bf75c45b69fc787867468e8735653c
This hook enhances it a little bit by offering the link to the correct block explorer of an existing tx

@alfetopito
Copy link
Contributor

Regarding where to display it, I think the appropriate place is in the not found page with a sentence like:

This is not a CowProtocol transaction.
See it in the <a>block explorer</a>

Feel free to propose a better sentence

@elena-zh
Copy link

Hey @matextrem , great changes!

However, I'd move the message outside the search block area
ouside

Besides, I'd add an etherscan icon to the link like we do for the all the rest external links in the Explorer
Screenshot_3
and showed 'etherscan' instead of the cropped hash for TxHash from Etherscan/Rinkeby, and 'blocscout' for TxHash in GC.
expl

Also, I noticed that the app navigates always to Etherscan for external transactions: I searched for GC TXHash, and was navigated to the 404 page in Etherscan:
blocscout

I searched for this TxHash https://blockscout.com/xdai/mainnet/tx/0xb97a644f31364f47efa2a8e3cb98b2c6ed29846aa6a71f514087523b1f2df845
The same is for Rinkeby transactions.

Could you please fix it?

@matextrem matextrem requested a review from elena-zh February 21, 2022 19:53
@elena-zh
Copy link

Hey @matextrem , looks much-much better!
However, the app does not offer me a link to the Blockscout when I search for an external transaction:
image

Also, it would be nice to increase margin between lines in the mobile view:
increase margin

@matextrem
Copy link
Contributor Author

@elena-zh I've just tested it and it is working for me. Maybe you have it cached?

image

Besides, I'm working on your feedback regarding styling.

@elena-zh
Copy link

@matextrem , hmm, thanks. Will check.
I will retest it when you merge you latest changes then.

@matextrem
Copy link
Contributor Author

@elena-zh done!

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.

@matextrem , changes LGTM now.
Thank you!

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Minor comments, looking good otherwise

@matextrem matextrem removed the request for review from henrypalacios February 23, 2022 22:06
@elena-zh
Copy link

LGTM now!

Copy link
Contributor

@henrypalacios henrypalacios left a comment

Choose a reason for hiding this comment

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

👍

@matextrem Just Nitpick: How about not explicitly using undefined in the state initialization? in order to be consistent with the rest of the state definitions

@matextrem matextrem added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Mar 2, 2022
@mergify mergify bot merged commit 15eabd6 into develop Mar 2, 2022
@henrypalacios henrypalacios deleted the 857/gpv2-tx-explorer branch March 2, 2022 18:26
@henrypalacios henrypalacios mentioned this pull request Apr 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds Protofire task to the protofire team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transaction viewer page: Identify when tx is/is not a GPv2 transaction (offer link to blockexplorer)
4 participants