-
Notifications
You must be signed in to change notification settings - Fork 93
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
Conversation
✅ Deploy Preview for jolly-shaw-20fe62 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this 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:
- Only link out to the stealth address if the transaction in question used the base asset
- 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:
7168583
to
e49a75c
Compare
There was a problem hiding this 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
Co-authored-by: Matt Solomon <[email protected]>
99e44eb
to
65e8edc
Compare
Partially addresses #135
Native token on top, ERC20 on the bottom