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

Keep eth address to fetch runtimes data #1564

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

buberdds
Copy link
Contributor

@buberdds buberdds commented Oct 7, 2024

Issue raised by Tadej (while using staker dApp as far as I remember) / for frontend sync meeting.

acc without address_preimage (keep eth addr)
https://pr-1564.oasis-explorer.pages.dev/mainnet/sapphire/address/0x95D683e4365209E5ce64f2e26e00C23207fe7Ec3
https://explorer.dev.oasis.io/mainnet/sapphire/address/0x95D683e4365209E5ce64f2e26e00C23207fe7Ec3

acc with address_preimage (works the same way)
https://pr-1564.oasis-explorer.pages.dev/mainnet/sapphire/address/0x8Bba8906467831962Ed5E524F4Bc47733CBF3b27
https://explorer.dev.oasis.io/mainnet/sapphire/address/0x8Bba8906467831962Ed5E524F4Bc47733CBF3b27

Notes: (thx @Andrew7234)

  • the address preimage is populated whenever nexus encounters the corresponding evm address of a given oasis style address
  • any evm transaction (evm.Create or evm.Call) should create preimage
  • the consensus deposit txs are part of the consensusaccounts module and not the evm module, so the tx body of consensus deposit only includes the oasis-style addresses in the tx body and thus Nexus isn’t aware of the corresponding evm address

Copy link

github-actions bot commented Oct 7, 2024

Deployed to Cloudflare Pages

Latest commit: 2a8b2700c38a3dc4f13e1afb42b14c028c96afbf
Status:✅ Deploy successful!
Preview URL: https://dbe8e57c.oasis-explorer.pages.dev
Alias: https://pr-1564.oasis-explorer.pages.dev

src/oasis-nexus/api.ts Outdated Show resolved Hide resolved
@buberdds buberdds force-pushed the mz/ethAddress branch 2 times, most recently from f89a7a4 to d856438 Compare October 11, 2024 08:12
@buberdds buberdds changed the title Keep eth address when it is provided via URL Keep eth address to fetch runtimes data Oct 11, 2024
@buberdds buberdds marked this pull request as ready for review October 11, 2024 09:00
@buberdds buberdds requested a review from lukaw3d October 13, 2024 10:31
Comment on lines -47 to -49
query: {
enabled: !!oasisAddress,
},
Copy link
Member

Choose a reason for hiding this comment

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

@lubej @csillag are checks like this still needed to handle some empty state

Copy link
Contributor Author

@buberdds buberdds Oct 16, 2024

Choose a reason for hiding this comment

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

oasisAddress used to be async. Since it is sync now and address is always true (loader would fail earlier if not) then I think it is not use to handle empty state

Comment on lines 251 to 258
const oasisAddress =
params?.rel && isValidEthAddress(params?.rel)
? getOasisAddressOrNull(params.rel)
: params?.rel

return {
...tx,
to_eth: !tx.to_eth && tx.to === oasisAddress ? params?.rel : tx.to_eth,
Copy link
Member

Choose a reason for hiding this comment

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

I would refactor this as:

const fallbackMaybeEthAddresses = [params?.rel]
const fallbackEthAddressesMap: { [oasis1Address: string]: `0x${string}` } =
  Object.fromEntries(
    fallbackMaybeEthAddresses.filter(isValidEthAddress).map((eth) => [getOasisAddressOrNull(eth), toChecksumAddress(eth)])
  )

return {
  ...tx,
  to_eth: tx.to_eth ?? fallbackEthAddressesMap[tx.to],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if address map is preferable way than we don't need an array, fromEntries, filter, map as we deal with a single address. Added a new commit with helper function.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, looks like we really don't have any EVM queries that support multiple address fields. nevermind

@lukaw3d
Copy link
Member

lukaw3d commented Oct 16, 2024

@buberdds
Copy link
Contributor Author

buberdds commented Oct 16, 2024

Ok, search will behave the same way. Discord test is failing (not related to changes)

@lukaw3d
Copy link
Member

lukaw3d commented Oct 16, 2024

Ooooh, so searching for "0x95D683e4365209E5ce64f2e26e00C23207fe7Ec3" would have still found the 4 results

@buberdds
Copy link
Contributor Author

Yes, search functionality is not affected by this PR. For now we want to tweak account view with eth address once you navigate from dApp. Will look at search later this week.

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.

2 participants