Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Etherscan links to sender and receiver addresses #381

Merged
merged 10 commits into from
Sep 2, 2022
Merged

Conversation

garyghayrat
Copy link
Member

@garyghayrat garyghayrat commented Aug 22, 2022

Partially addresses #135
Screenshot_2022-08-31_15-24-06 Native token on top, ERC20 on the bottom

@garyghayrat garyghayrat added the app A change related to the Umbra frontend label Aug 22, 2022
@netlify
Copy link

netlify bot commented Aug 22, 2022

Deploy Preview for jolly-shaw-20fe62 ready!

Name Link
🔨 Latest commit 65e8edc
🔍 Latest deploy log https://app.netlify.com/sites/jolly-shaw-20fe62/deploys/6312043aeda2b70008e99455
😎 Deploy Preview https://deploy-preview-381--jolly-shaw-20fe62.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@garyghayrat garyghayrat removed the app A change related to the Umbra frontend label Aug 22, 2022
@garyghayrat garyghayrat requested a review from mds1 August 23, 2022 00:42
frontend/src/utils/utils.ts Outdated Show resolved Hide resolved
frontend/src/utils/address.ts Outdated Show resolved Hide resolved
@garyghayrat garyghayrat requested review from apbendi and mds1 August 25, 2022 15:30
@garyghayrat garyghayrat self-assigned this Aug 25, 2022
Copy link
Member

@apbendi apbendi left a comment

Choose a reason for hiding this comment

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

This works well, and the code looks good too, but I think we want to change this a bit to avoid confusion by users.

The intent of #135 is to have the word "Withdrawn" be clickable after the withdraw occurs, and to have the link go to the Etherscan record of the withdraw txn itself. In the case of base asset (ETH, OETH, AETH, or MATIC), this will be a normal transfer by the stealth address. In the case of a token, this will be a meta-tx transaction sent through the Umbra contract by a relayer.

In both cases, it's a bit difficult to actually find this transaction quickly. We would need to use an indexing service like The Graph and do a little query-fu. As a result, a simple first pass approach is to link to the stealth address on Etherscan rather than the transaction itself. This is helpful in the base asset case, because the stealth address will have a single transaction (the withdrawal) and it would be easy for the user to see that. It is not helpful at all for Token withdrawals because in that case, the stealth address in question will never have been seen on chain, except as calldata. As such, I think it's actually more confusing to link to the stealth address in the token withdraw case than simply not including anything.

Based on all this, I think we should make two changes to this PR:

  1. Only link out to the stealth address if the transaction in question used the base asset
  2. Change the link to be on the word "Withdrawn" instead of on the stealth address in the table. Keep the style as-is, "Withdrawn" should just be clickable. A nice to have the little square/arrow indicator appear on hover if it's clickable, as seen here:

Screen Shot 2022-08-29 at 1 29 35 PM

@garyghayrat
Copy link
Member Author

garyghayrat commented Aug 31, 2022

Screenshot_2022-08-31_15-24-06
With the latest PR, native token withdrawals looks like the row on top and ERC20 tokens like the one on the bottom.

Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

LGTM, CI failure is unrelated, looks like its due to infra providers being down during the arby upgrade yesterday. Will defer to @apbendi to review / make sure UX is good before merging

image

@apbendi apbendi merged commit efa104c into master Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

After withdraw, clicking "Withdrawn" in receive table should take user to withdrawal tx on Etherscan
3 participants