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

Display EVM addresses in events #1364

Merged
merged 6 commits into from
Apr 5, 2024
Merged

Display EVM addresses in events #1364

merged 6 commits into from
Apr 5, 2024

Conversation

lubej
Copy link
Collaborator

@lubej lubej commented Apr 4, 2024

Related to oasisprotocol/nexus#653

Description

This pull request addresses the issue, where custom events do not take into account address type toggle.

Account to test with:
testnet/sapphire/tx/0x8a99091101d5ee03c3e120ee222eaafcff6715a324c3936f88e733231e1c3ea5

Copy link

github-actions bot commented Apr 4, 2024

Deployed to Cloudflare Pages

Latest commit: 05b290febd6ad5af73e81e7ff43c3d9a9a46ec58
Status:✅ Deploy successful!
Preview URL: https://b3ccba1a.oasis-explorer.pages.dev

@lubej lubej marked this pull request as ready for review April 4, 2024 04:53
.changelog/1364.bugfix.md Outdated Show resolved Hide resolved
resolveAddresses()
}, [address, addressSwitchOption])

return <Component {...restProps} address={convertedAddress} />
Copy link
Member

Choose a reason for hiding this comment

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

Ideally it should add a tooltip too. See

<dd>
<TransactionInfoTooltip
label={
from
? isOasisAddressFormat
? t('transaction.tooltips.senderTooltipOasis')
: t('transaction.tooltips.senderTooltipEth')
: t('transaction.tooltips.senderTooltipUnavailable')
}
>
<AccountLink
scope={transaction}
address={
from ||
((isOasisAddressFormat ? transaction?.sender_0_eth : transaction?.sender_0) as string)
}
/>
</TransactionInfoTooltip>
{from && <CopyToClipboard value={from} />}
</dd>

Copy link
Member

Choose a reason for hiding this comment

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

Ah, but the tooltip text is annoyingly specific (senderTooltipOasis, senderTooltipEth)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great point, but I think this should be included in the events as well. Even though I doubt the "validity" of it, in a sense, how much additional information it adds. But I guess we decided on this for a reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did add the tooltip inside withAddressSwitch, but I am unsure of naming, to make it generic. Should we accept translations as an input? As withAddressSwitch can be used as generic component, that could technically wrap TransactionLink.

Copy link
Member

Choose a reason for hiding this comment

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

I think having them as input shouldn't be too much overhead

Copy link
Member

Choose a reason for hiding this comment

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

(in the current state of labels, it doesn't add much information, except when hovering transaction hash. Oasis transaction hashes don't have an obvious "oasis1" prefix)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, wanted to discuss on next frontend sync, what labels we want and even if we want them(can always add them later) - and just remove the Tooltip for now. Will ping @donouwens next week.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed Tooltip for now, as I probably won't be able to make it to the next FE sync + Don as well. So we can discuss on the next one, that we are all able to join. Just want to get this ready for release before the conference.

@lubej lubej force-pushed the ml/fix-transaction-toggle branch from cc35a94 to 3f2eb9a Compare April 5, 2024 11:51
@lubej lubej changed the title Fix address type toggle on transaction page Display EVM addresses in events Apr 5, 2024
@lubej lubej force-pushed the ml/fix-transaction-toggle branch 4 times, most recently from 3fe333f to 83879a2 Compare April 5, 2024 12:42
@lubej lubej force-pushed the ml/fix-transaction-toggle branch from 83879a2 to 41492be Compare April 5, 2024 12:45
@lubej lubej requested review from buberdds and lukaw3d April 5, 2024 12:49
@lubej lubej force-pushed the ml/fix-transaction-toggle branch from cabd21b to d2bd853 Compare April 5, 2024 17:27
@lubej lubej requested a review from lukaw3d April 5, 2024 17:28
@lubej lubej force-pushed the ml/fix-transaction-toggle branch from d2bd853 to 8ff6e0c Compare April 5, 2024 19:40
- refactor address variables in withAddressSwitch
@lubej lubej force-pushed the ml/fix-transaction-toggle branch from 8097964 to 05b290f Compare April 5, 2024 19:46
@lubej lubej merged commit 7bcbbcd into master Apr 5, 2024
8 checks passed
@lubej lubej deleted the ml/fix-transaction-toggle branch April 5, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants