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

Integrate ETH address conversion; ensure parachain links #459

Closed
KarimJedda opened this issue Nov 4, 2024 · 33 comments · Fixed by #460
Closed

Integrate ETH address conversion; ensure parachain links #459

KarimJedda opened this issue Nov 4, 2024 · 33 comments · Fixed by #460
Assignees
Labels
enhancement New feature or request

Comments

@KarimJedda
Copy link

Quoting from @pgherveou :

it would be nice if we could update https://faucet.polkadot.io/westend with the followings:

  • Have a custom url for each chain, instead of asking the user to pick a chain from the dropdown, it's too easy to forget to do it and send the fund to the relay chain instead.
  • Detect 0x Eth address so that you can fund directly Eth address
@KarimJedda KarimJedda added the enhancement New feature or request label Nov 4, 2024
@lovelaced
Copy link
Collaborator

@pgherveou how is the eth address mapped to a substrate address?

@pgherveou
Copy link

@smiasojed , I think you already did something similar for Remix, do you have some js code to go
from Eth -> AccountId32 that we can reuse here?

@smiasojed
Copy link

Yes, I did, code: https://github.com/paritytech/revive-remix/blob/2eca8b351f96734ffc94adc63b94f3587bd8d67d/libs/remix-ui/helper/src/lib/remix-ui-helper.ts#L93

@pgherveou
Copy link

@lovelaced let us know if you need help on this.
how long do you think it would take to make / publish these changes?

@mutantcornholio mutantcornholio self-assigned this Nov 4, 2024
@mutantcornholio
Copy link
Contributor

Doesn't look complex. I'll try to get it to PR stage today.
Should address conversion work on any testnet, or only on Westend?

@lovelaced
Copy link
Collaborator

@mutantcornholio only required for westend asset hub for now, but if it's easier to add for all chains that's also fine

@mutantcornholio
Copy link
Contributor

mutantcornholio commented Nov 4, 2024

@mutantcornholio only required for westend asset hub for now, but if it's easier to add for all chains that's also fine

Adding it to all chains would be easier. Will it work as expected?

@lovelaced
Copy link
Collaborator

@pgherveou @smiasojed do you have any input on the above? ^

@athei
Copy link
Member

athei commented Nov 4, 2024

It works the same for all chains:
1 ) Take the 20 bit eth address.
2) Suffix with 12 bytes of 0xEE in order to get 32byte address
3) SS58 encode

@smiasojed
Copy link

For Kusama or Polkadot will be different prefix in SS58 encode.

@mutantcornholio
Copy link
Contributor

It works the same for all chains:...

Ah yeah, thanks, the pieces came together now.

For Kusama or Polkadot will be different prefix in SS58 encode.

There's no faucet for Kusama / Polkadot, but good to keep in mind. I'll probably make the prefix configurable.

@mutantcornholio
Copy link
Contributor

  • Have a custom url for each chain, instead of asking the user to pick a chain from the dropdown, it's too easy to forget to do it and send the fund to the relay chain instead.

Turns out, it's actually already implemented: https://faucet.polkadot.io/westend?parachain=1000
What's not implemented, however, is updating the address when changing the parachain from the dropdown 🤦‍♂️. Will fix.

@pgherveou
Copy link

  • Have a custom url for each chain, instead of asking the user to pick a chain from the dropdown, it's too easy to forget to do it and send the fund to the relay chain instead.

Turns out, it's actually already implemented: https://faucet.polkadot.io/westend?parachain=1000 What's not implemented, however, is updating the address when changing the parachain from the dropdown 🤦‍♂️. Will fix.

seems to work for me, it does update it to assethub

@mutantcornholio
Copy link
Contributor

seems to work for me, it does update it to assethub

I meant the other way around: select a different parachain from the dropdown → the url doesn't get updated.
You can send direct link to people, but you've got no way to get direct link, without prior knowledge.

@pgherveou
Copy link

ah right that's correct, I think it will still be nice if not too much extra works to have a "clean url" without querystring

@mutantcornholio
Copy link
Contributor

ah right that's correct, I think it will still be nice if not too much extra works to have a "clean url" without querystring

What's wrong with querystring?

@pgherveou
Copy link

I get it's ok this is just bike shedding, we need to reference the faucets on some repo, .../asset-hub just looks nicer but we will go with parachain=1000 for now

Quick question can you also specify the address from the querystring?

@mutantcornholio
Copy link
Contributor

Quick question can you also specify the address from the querystring?

Yeah, but could you explain the usecase for this?

@pgherveou
Copy link

use case would be to create link that prefill everything, once you have connected your wallet, we can point you to the url where you can ask for test funds by prefilling all the details

@lovelaced
Copy link
Collaborator

@pgherveou do you mean we would be linking directly from remix or something? Where is the user going to be connecting their wallet and being redirected to the faucet? (just for context)

@athei
Copy link
Member

athei commented Nov 4, 2024

We will be linking to the faucet from our tutorial:
paritytech/contract-docs#10

Would be nice if we can select the correct network via direct link. Otherwise the user has to select the correct network.

@lovelaced
Copy link
Collaborator

We will be linking to the faucet from our tutorial: paritytech/contract-docs#10

Would be nice if we can select the correct network via direct link. Otherwise the user has to select the correct network.

Yes, this is already possible. I was wondering about the context for prefilled addresses.

@mutantcornholio
Copy link
Contributor

I'm trying to conjure a meaningful e2e test for address conversion.
If my goal is to obtain a usable eth address... Do I deploy a smart contract?

I want to avoid duplicating the same conversion logic both in code and in test

@athei
Copy link
Member

athei commented Nov 4, 2024

We will be linking to the faucet from our tutorial: paritytech/contract-docs#10
Would be nice if we can select the correct network via direct link. Otherwise the user has to select the correct network.

Yes, this is already possible. I was wondering about the context for prefilled addresses.

Not sure what @pgherveou has in mind. But we could use it to fill the address that is selected in Metamask. Doesn't seem like a high priority, though.

@pgherveou
Copy link

yeah not a hight priority at all just checking if this could be done at all

@lovelaced
Copy link
Collaborator

I'm trying to conjure a meaningful e2e test for address conversion. If my goal is to obtain a usable eth address... Do I deploy a smart contract?

I want to avoid duplicating the same conversion logic both in code and in test

You can just do an ethereum transfer (you can generate a regular eth address in metamask, and then send funds to another address). You could probably use web3.js or ethers.js for this if that makes sense, you can view such transactions here with the resulting substrate addresses.

mutantcornholio added a commit that referenced this issue Nov 4, 2024
Fixes #459

I'm still thinking on how to properly test ethereum drips, will follow
up on that.
mutantcornholio added a commit that referenced this issue Nov 4, 2024
Fixes #459

I'm still thinking on how to properly test ethereum drips, will follow
up on that.
mutantcornholio added a commit that referenced this issue Nov 4, 2024
Fixes #459

I'm still thinking on how to properly test ethereum drips, will follow
up on that.
@lovelaced
Copy link
Collaborator

yeah not a hight priority at all just checking if this could be done at all

Yes it's definitely possible, but we can revisit this if there's a direct need for it

@mutantcornholio
Copy link
Contributor

The changes were deployed, try using 0x.. address here: https://faucet.polkadot.io/westend?parachain=1000

@lovelaced
Copy link
Collaborator

Nice, worked for me @mutantcornholio ! 🎉

@lovelaced
Copy link
Collaborator

@mutantcornholio the RPC URL needs to be changed to https://westend-asset-hub-eth-rpc.polkadot.io

in addition, can the placeholder text in the address field say something like: "5rt6... or 0x833..."?

@lovelaced lovelaced reopened this Nov 5, 2024
@mutantcornholio
Copy link
Contributor

@mutantcornholio the RPC URL needs to be changed to https://westend-asset-hub-eth-rpc.polkadot.io

Why? Faucet dripper account lives on relay chain, and we're teleporting tokens to AH.

in addition, can the placeholder text in the address field say something like: "5rt6... or 0x833..."?

sure

@lovelaced
Copy link
Collaborator

Oh, sorry, my misunderstanding - I thought the eth RPC was used somehow here.

@mutantcornholio
Copy link
Contributor

in addition, can the placeholder text in the address field say something like: "5rt6... or 0x833..."?

#461

@mutantcornholio mutantcornholio changed the title Feature request: Faucet improvements Integrate ETH address conversion; ensure parachain links Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants