Add ENS name resolution to EIP681 support #9563

acolytec3 · 2019-12-02T10:51:26Z

Summary
EIP681 URI parsing does not currently support ENS name verification. This PR enables the parse-uri function to accept properly formatted ENS names as the to address in a URI as well enables the QR scanner to properly resolve URIs with an ENS address.

This fixes #9238 and fixes #9357.

Review notes
NA

Testing notes
NA

Platforms
Android
iOS
Areas that maybe impacted
Wallet/Send-Transaction/QR-Code

Functional
wallet / transactions
Non-functional
N/A

Steps to test
Open Status

Open Wallet

Select any account

Select Send

Select specify-recipient

Select 'Scan QR code'

Scan the below QR code.
ethereum:pay-snt.thetoken.eth/transfer?address=unicorn.stateofus.eth&uint256=1e16

Verify that ENS address is resolved to ethereum address and currency changes to SNT.

status: ready

status-github-bot · 2019-12-02T10:51:32Z

Pull Request Checklist

Docs: Updated the documentation, if affected
Docs: Added or updated inline comments explaining intention of the code
Tests: Ensured that all new UI elements have been assigned accessibility IDs
Tests: Signaled need for E2E tests with label, if applicable
Tests: Briefly described what was tested and what platforms were used
UI: In case of UI changes, ensured that UI matches Figma
UI: In case of UI changes, requested review from a Core UI designer
UI: In case of UI changes, included screenshots of implementation

status-im-auto · 2019-12-02T10:54:37Z

Jenkins Builds

Click to see older builds (21)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	`f422917`	#1	2019-12-02 10:54:37	~2 min	`android`	📄`log`
❌	`f422917`	#1	2019-12-02 10:54:43	~3 min	`android-e2e`	📄`log`
❌	`f422917`	#1	2019-12-02 10:55:47	~4 min	`ios`	📄`log`

✔️	`939b790`	#2	2019-12-02 12:05:18	~11 min	`android-e2e`	📦`apk` 📲
✔️	`939b790`	#2	2019-12-02 12:05:22	~11 min	`android`	📦`apk` 📲
✔️	`939b790`	#2	2019-12-02 12:05:28	~11 min	`ios`	📦`ipa` 📲

✔️	`ba35dcd`	#3	2019-12-02 12:21:57	~8 min	`ios`	📦`ipa` 📲
✔️	`ba35dcd`	#3	2019-12-02 12:28:08	~14 min	`android`	📦`apk` 📲
✔️	`ba35dcd`	#3	2019-12-02 12:28:22	~14 min	`android-e2e`	📦`apk` 📲

❌	`15638bc`	#4	2019-12-02 13:33:15	~3 min	`android`	📄`log`
❌	`15638bc`	#4	2019-12-02 13:33:16	~3 min	`android-e2e`	📄`log`
❌	`15638bc`	#4	2019-12-02 13:33:19	~2 min	`ios`	📄`log`

❌	`51da3af`	#5	2019-12-02 14:15:23	~1 min	`android-e2e`	📄`log`
❌	`51da3af`	#5	2019-12-02 14:15:50	~2 min	`ios`	📄`log`
❌	`51da3af`	#5	2019-12-02 14:16:08	~2 min	`android`	📄`log`

❌	`8ccae5f`	#6	2019-12-02 14:54:29	~24 sec	`android`	📄`log`
❌	`8ccae5f`	#6	2019-12-02 14:54:33	~22 sec	`ios`	📄`log`
✔️	`8ccae5f`	#6	2019-12-02 15:06:29	~12 min	`android-e2e`	📦`apk` 📲

✔️	`99474b2`	#7	2019-12-02 20:17:34	~12 min	`android`	📦`apk` 📲
✔️	`99474b2`	#7	2019-12-02 20:22:47	~17 min	`ios`	📦`ipa` 📲
✔️	`99474b2`	#7	2019-12-02 20:26:48	~21 min	`android-e2e`	📦`apk` 📲

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`a853229`	#8	2019-12-04 15:03:04	~9 min	`ios`	📦`ipa` 📲
✔️	`a853229`	#8	2019-12-04 15:05:42	~12 min	`android-e2e`	📦`apk` 📲
✔️	`a853229`	#8	2019-12-04 15:05:44	~12 min	`android`	📦`apk` 📲

✔️	`f4cc20f`	#9	2019-12-04 20:02:12	~9 min	`ios`	📦`ipa` 📲
✔️	`f4cc20f`	#9	2019-12-04 20:05:04	~12 min	`android`	📦`apk` 📲
✔️	`f4cc20f`	#9	2019-12-04 20:05:11	~12 min	`android-e2e`	📦`apk` 📲

acolytec3 · 2019-12-02T10:57:07Z

@yenda @flexsurfer Can you advise on the errors coming from the build log? When I run the tests locally, I see a bunch of warnings but they don't appear to be related to my code.

flexsurfer · 2019-12-02T11:06:36Z

@acolytec3 huge kudos for your effort, there is an error during the build

[2019-12-02T10:54:03.323Z] Compiling ["index.ios.js" "status-modules/cljs/i18n-raw.js" "status-modules/cljs/network-raw.js"] from ["components/src" "react-native/src/cljsjs" "react-native/src/mobile" "src" "env/prod" "prod"]...

[2019-12-02T10:54:30.834Z] WARNING: Use of undeclared Var status-im.wallet.choose-recipient.core/new-gas at line 134 /Users/jenkins/workspace/status-react_prs_ios_PR-9563/src/status_im/wallet/choose_recipient/core.cljs

[2019-12-02T10:54:30.834Z] Error encountered performing task 'cljsbuild' with profile(s): 'prod'

[2019-12-02T10:54:30.834Z] Subprocess failed

here https://github.com/status-im/status-react/pull/9563/files#diff-09bf352f4017403bdf258a3d90eac06aR134

acolytec3 · 2019-12-02T11:54:09Z

@flexsurfer Thanks! I see the issue. Accidentally reverted one function request-parsed-uri to old code. Added back in the correct code from develop.

acolytec3 · 2019-12-02T12:14:27Z

@flexsurfer Looks like it builds now. I rebased and squashed so hopefully ready for QA at this point.

src/status_im/utils/universal_links/core.cljs

src/status_im/wallet/choose_recipient/core.cljs

churik · 2019-12-04T12:38:47Z

@acolytec3
Still reproducible issues:

ISSUE 1) Amount is not parsed from URI (always set to 0)

should be reported and fixed separately, as far as I understood from discussion

ISSUE 2) [Object object] instead of link when scanning URL with wrong network id

can be fixed later

ISSUE 3) ENS name is not resolved when asset is ETH

ethereum:tanyatest1.eth?value=1e15 is not working (ETH), but ethereum:0x744d70fdbe2ba4cf95131626614a1763df805b9e/transfer?address=tanyatest1.eth&uint256=2e1 (SNT, but the same address) is resolved as expected.

All other issues are fixed.
And thank you again for a lot of work here.
So at this point, I would expect that Issue 3 will be fixed, as it is related to PR itself.

acolytec3 · 2019-12-04T14:45:50Z

@3esmit please confirm if issue 3 outlined by @churik is actually an issue? This should return an "invalid address" error as far as I know since part of the original issue that you and I started with was that basic ETH transaction with an ENS to address must have have the pay- prefix per EIP831. I verified that ethereum:pay-tanyatest1.eth?value=1e15 resolves the ENS address and populates the send transaction as expected while ethereum:tanyatest1.eth?value=1e15 throws the invalid address error as currently coded.

acolytec3 · 2019-12-04T14:54:19Z

@churik Also, just fixed issue 2 so please retest when you have a chance. Was populating that error message incorrectly but it's resolved now. Per your comment, issue 1 is not specific to this PR so should be addressed separately.

churik · 2019-12-04T15:11:31Z

please confirm if issue 3 outlined by @churik is actually an issue? This should return an "invalid address" error as far as I know since part of the original issue that you and I started with was that basic ETH transaction with an ENS to address must have have the pay- prefix per EIP831. I verified that ethereum:pay-tanyatest1.eth?value=1e15 resolves the ENS address and populates the send transaction as expected while ethereum:tanyatest1.eth?value=1e15 throws the invalid address error as currently coded.

Probably it is not an issue, please confirm @3esmit

3esmit · 2019-12-04T18:07:45Z

ethereum:tanyatest1.eth?value=1e15 should not work because is missing pay-. Whenever an ENS_NAME is used, pay- must be present.

However, ethereum:pay-tanyatest1.eth?value=1e15 should work. The same applies to ERC20 transfers.

Consider - TX1 a value(ETH) transfer to the ENS resolved address of tanyatest1.eth - and TX2 as a ERC20 token (SNT) transfer to resolved address of tanyatest1.eth - the examples of valid EIP681 that could be generated for that:

TX1 not using ENS and omitting pay- prefix.

ethereum:0xcf2272205cc0cf96cfbb9dd740bd681d1e86901e?value=1e14

TX2 not using ENS and omitting pay- prefix.

ethereum:0x744d70fdbe2ba4cf95131626614a1763df805b9e/transfer?address=0xcf2272205cc0cf96cfbb9dd740bd681d1e86901e&uint256=2e1

TX2 using ENS only for address parameter of transfer and omitting pay- prefix.

ethereum:0x744d70fdbe2ba4cf95131626614a1763df805b9e/transfer?address=tanyatest1.eth&uint256=2e1

TX1 not using ENS and using pay- prefix.

ethereum:pay-0xcf2272205cc0cf96cfbb9dd740bd681d1e86901e?value=1e14

TX2 not using ENS and using pay- prefix.

ethereum:pay-0x744d70fdbe2ba4cf95131626614a1763df805b9e/transfer?address=0xcf2272205cc0cf96cfbb9dd740bd681d1e86901e&uint256=2e1

TX2 using ENS only for address parameter of transfer and using pay- prefix.

ethereum:pay-0x744d70fdbe2ba4cf95131626614a1763df805b9e/transfer?address=tanyatest1.eth&uint256=2e1

TX1 using ENS and using pay- prefix:

ethereum:pay-tanyatest1.eth?value=1e14

TX2 using ENS everywhere and using pay- prefix:

ethereum:pay-snt.thetoken.eth/transfer?address=tanyatest1.eth&uint256=2e1

TX2 using ENS only in token address and using regular address transfer parameter, and using pay- prefix.

ethereum:pay-snt.thetoken.eth/transfer?address=0xcf2272205cc0cf96cfbb9dd740bd681d1e86901e&uint256=2e1

This above does not include custom setting for gas, and is a limited test set only for this issue, not for all EIP681. I will test them myself as well, and we might want to setup an automated test to include all those, and if possible, testing the ENS resolution (we might need to setup a development chain with a fake ENS to do that, and we might want to leave it to another issue.)

3esmit · 2019-12-04T19:01:16Z

ENS names work fine for the selected token and for the destination address.

Some more examples I used to test if I can rescan right after scanning, and it works:
ethereum:pay-snt.thetoken.eth/transfer?address=tanyatest1.eth&uint256=1e18

Notice, change only address parameter of transfer
ethereum:pay-snt.thetoken.eth/transfer?address=unicorn.stateofus.eth&uint256=2e17

Notice, changes token:
ethereum:pay-ant.thetoken.eth/transfer?address=acolytec3.stateofus.eth&uint256=3e16

Notice, changes back to ETH asset:
ethereum:pay-ethereum.eth?value=3e16

Future issues we might open related to this:

I see in a next issue we might also want to polish the UX, such as: address field, to display "loading ENS", and also display the ENS name alongside the resolved name.
I found too far away to scan the qrcode, and the place is wrong, it should not scan under destination, but elsewhere, as the scanning affects all transaction parameters (not just the address). Scanning the address is an important feature, and we can leave it there how it is (enter address if is just a regular address, fill transaction if is EIP681), but also include another more handy and better contextful on scan qr code.
I see that the scan qr code from chat screen does not work anymore, and we might also want to open another issue about this, as it was a nice place to scan anything. WeChat have a button that allows qrcode anything and it figures out what to do with it.
Also the issue Chu already mentioned about the value.

3esmit · 2019-12-04T19:29:22Z

Also noticed that the error message shows [Object object] when scanning some invalid ENS.

acolytec3 · 2019-12-04T19:53:56Z

@3esmit Fixed that error message to display the address when there's an invalid ENS address as well. Same root cause as what @churik identified but I just missed that spot in the code. Fixed now.

churik · 2019-12-05T09:46:24Z

@3esmit @acolytec3
Ready to merge.
Thanks all for your collaboration!
It was a tough one)

Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>

flexsurfer · 2019-12-05T09:49:11Z

huge kudos @acolytec3 !

acolytec3 · 2019-12-05T10:15:52Z

Thanks all for the support. This was a tough one but glad to see it merged!

acolytec3 requested a review from a team as a code owner December 2, 2019 10:51

auto-assign bot removed the request for review from a team December 2, 2019 10:51

acolytec3 mentioned this pull request Dec 2, 2019

Add ENS name resolution for EIP681 URIs and support in QR code scanner #9240

Closed

flexsurfer requested review from 3esmit and yenda December 2, 2019 11:03

flexsurfer added community request-manual-qa labels Dec 2, 2019

acolytec3 force-pushed the ens-inEIP681-3 branch from 939b790 to ba35dcd Compare December 2, 2019 12:13

flexsurfer requested changes Dec 2, 2019

View reviewed changes

src/status_im/utils/universal_links/core.cljs Outdated Show resolved Hide resolved

flexsurfer reviewed Dec 2, 2019

View reviewed changes

src/status_im/wallet/choose_recipient/core.cljs Outdated Show resolved Hide resolved

flexsurfer reviewed Dec 2, 2019

View reviewed changes

src/status_im/wallet/choose_recipient/core.cljs Outdated Show resolved Hide resolved

flexsurfer approved these changes Dec 2, 2019

View reviewed changes

flexsurfer reviewed Dec 2, 2019

View reviewed changes

src/status_im/wallet/choose_recipient/core.cljs Outdated Show resolved Hide resolved

flexsurfer reviewed Dec 2, 2019

View reviewed changes

src/status_im/wallet/choose_recipient/core.cljs Outdated Show resolved Hide resolved

acolytec3 force-pushed the ens-inEIP681-3 branch from 8ccae5f to 99474b2 Compare December 2, 2019 20:05

churik self-assigned this Dec 4, 2019

churik added the Tested - Issues label Dec 4, 2019

acolytec3 force-pushed the ens-inEIP681-3 branch from 99474b2 to a853229 Compare December 4, 2019 14:53

3esmit approved these changes Dec 4, 2019

View reviewed changes

acolytec3 force-pushed the ens-inEIP681-3 branch from a853229 to f4cc20f Compare December 4, 2019 19:52

churik added Tested - OK and removed Tested - Issues labels Dec 5, 2019

Add ENS name resolution to EIP681 support

2fda266

Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>

flexsurfer force-pushed the ens-inEIP681-3 branch from f4cc20f to 2fda266 Compare December 5, 2019 09:48

flexsurfer merged commit 2fda266 into status-im:develop Dec 5, 2019

flexsurfer mentioned this pull request Jan 16, 2020

Open app by ens name #9811

Merged

churik mentioned this pull request Feb 27, 2020

Value is not supported in EIP681 links #10093

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ENS name resolution to EIP681 support #9563

Add ENS name resolution to EIP681 support #9563

acolytec3 commented Dec 2, 2019 •

edited

Loading

status-github-bot bot commented Dec 2, 2019

status-im-auto commented Dec 2, 2019 •

edited

Loading

acolytec3 commented Dec 2, 2019

flexsurfer commented Dec 2, 2019 •

edited

Loading

acolytec3 commented Dec 2, 2019

acolytec3 commented Dec 2, 2019

churik commented Dec 4, 2019 •

edited

Loading

acolytec3 commented Dec 4, 2019 •

edited

Loading

acolytec3 commented Dec 4, 2019

churik commented Dec 4, 2019

3esmit commented Dec 4, 2019 •

edited

Loading

3esmit commented Dec 4, 2019 •

edited

Loading

3esmit commented Dec 4, 2019

acolytec3 commented Dec 4, 2019

churik commented Dec 5, 2019

flexsurfer commented Dec 5, 2019

acolytec3 commented Dec 5, 2019

Add ENS name resolution to EIP681 support #9563

Add ENS name resolution to EIP681 support #9563

Conversation

acolytec3 commented Dec 2, 2019 • edited Loading

status-github-bot bot commented Dec 2, 2019

Pull Request Checklist

status-im-auto commented Dec 2, 2019 • edited Loading

Jenkins Builds

acolytec3 commented Dec 2, 2019

flexsurfer commented Dec 2, 2019 • edited Loading

acolytec3 commented Dec 2, 2019

acolytec3 commented Dec 2, 2019

churik commented Dec 4, 2019 • edited Loading

ISSUE 1) Amount is not parsed from URI (always set to 0)

ISSUE 2) [Object object] instead of link when scanning URL with wrong network id

ISSUE 3) ENS name is not resolved when asset is ETH

acolytec3 commented Dec 4, 2019 • edited Loading

acolytec3 commented Dec 4, 2019

churik commented Dec 4, 2019

3esmit commented Dec 4, 2019 • edited Loading

3esmit commented Dec 4, 2019 • edited Loading

3esmit commented Dec 4, 2019

acolytec3 commented Dec 4, 2019

churik commented Dec 5, 2019

flexsurfer commented Dec 5, 2019

acolytec3 commented Dec 5, 2019

acolytec3 commented Dec 2, 2019 •

edited

Loading

status-im-auto commented Dec 2, 2019 •

edited

Loading

flexsurfer commented Dec 2, 2019 •

edited

Loading

churik commented Dec 4, 2019 •

edited

Loading

acolytec3 commented Dec 4, 2019 •

edited

Loading

3esmit commented Dec 4, 2019 •

edited

Loading

3esmit commented Dec 4, 2019 •

edited

Loading