Add ENS name resolution for EIP681 URIs and support in QR code scanner #9240
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (135)
|
|
@3esmit I believe this solves the basic question of validating ENS addresses within an EIP681 URI, per this specific issue as I have seen events in re-frisk where the ENS address is correctly parsed and set in the wallet/send-transaction/to field in the DB. That said, the only place I'm aware that these URIs are accepted in the app is in the send-transaction flow where you scan a QR code and that flow doesn't handle ENS addresses at present and will throw an unhandled error if you scan a URI with an ENS address embedded in it since it tries to pass it to |
|
Thanks for this. This is one step closer to full EIP681 support :) If the UI is not resolving ENS Names in any |
|
You might also remove the comment "TODO ens support" :) |
|
Ah, I see, you say about "generating ERC681" using ENS. Yeah, that is totally a new feature, it changes the UI because users might want to enable or disable using ENS name in the payment link. We certainly would need discussion on that, so I will open another issue "Allow users generate EIP681 with ENS name". |
Sorry, I think I misstated this. ENS name resolution works fine if you enter an address manually in the send flow. It's only when you scan it from a EIP681 URI in the QR flow that it's failing, due to the above noted explanation. It's happening because that flow doesn't expect ENS addresses today so assumes any time the QR read succeeds that it has a valid Ethereum address and then tries to run it through the checksum validation method before populating in the to field in the send-transaction screen and that's where the error occurs. To your above point, I guess that could probably be addressed as a separate issue/PR since it deals with a specific UI flow not resolving ENS addresses. |
acolytec3
force-pushed
the
ens-in-EIP681
branch
from
17ef6a8 to
57af3ab
Compare
Done! |
|
Also, seems like that it should only support ENS if prefix |
There was a problem hiding this comment.
To not conflicting with EIP831:
prefixis optional and defines the use-case for this URI. If no prefix is given: “pay-“ is assumed to be concise and ensure backward compatibility to ERC-67. When the prefix is omitted, the payload must start with 0x. Also prefixes must not start with 0x. So starting with 0x can be used as a clear signal that there is no prefix.
So this would be the valid semantic:
ethereum: + (ETH_ADDRESS | (pay- + (ETH_ADDRESS | ENS_NAME)) + payload.
|
Got it. I'll take a look at the regex and see if I can make that work.
…On Mon, Oct 21, 2019, 4:33 AM Ricardo Guilherme Schmidt < ***@***.***> wrote:
***@***.**** requested changes on this pull request.
To not conflicting with EIP831 <https://eips.ethereum.org/EIPS/eip-831>:
prefix is optional and defines the use-case for this URI. If no prefix is
given: “pay-“ is assumed to be concise and ensure backward compatibility to
ERC-67. When the prefix is omitted, the payload must start with 0x. Also
prefixes must not start with 0x. So starting with 0x can be used as a clear
signal that there is no prefix.
So this would be the valid semantic:
ethereum: + (ETH_ADDRESS | (pay- + (ETH_ADDRESS | ENS_NAME)) + payload.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9240>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEENFXGWIHEOY2T52DVPHALQPVSNZANCNFSM4JCNVGIA>
.
|
acolytec3
force-pushed
the
ens-in-EIP681
branch
from
57af3ab to
086e2de
Compare
|
@3esmit I've added in additional validation for the |
|
Looks good. Glad you didn't had to use regex for that. I checked the logic and it is according to EIP681: checks if address, if not check if there is prefix @yenda can you validate if the PR and merge it? Thanks! |
|
Cool, just remember we need another issue to deal with the QR code methods
not resolving ens addresses.
…On Thu, Oct 24, 2019, 10:18 PM Ricardo Guilherme Schmidt < ***@***.***> wrote:
Looks good. Glad you didn't had to use regex for that.
I checked the logic and it is according to EIP681: checks if address, if
not check if there is prefix pay, if there is, checks if is ens name
valid or address valid.
@yenda <https://github.com/yenda> can you validate if the PR and merge
it? Thanks!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9240>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEENFXBEU744C32XIRYMMYDQQJJQJANCNFSM4JCNVGIA>
.
|
|
@acolytec3 please, rebase it to current develop and I try to rebuild it |
acolytec3
force-pushed
the
ens-in-EIP681
branch
from
9d70fca to
2290cdf
Compare
|
@churik This only fixes the error message saying that the link is invalid, while it was valid. |
Co-Authored-By: yenda <[email protected]>
|
@flexsurfer I just pushed my latest commit which should resolve all of the existing issues, aside from that oddity about values not being correctly represented (which isn't part of my changes) and now building a version rebased on |
|
Please tell me when it should be retested. |
|
@flexsurfer Having trouble building on the latest |
|
@acolytec3 lets push changes anyway, cc @jakubgs |
acolytec3
force-pushed
the
ens-in-EIP681
branch
from
2a3decd to
8a14435
Compare
|
@flexsurfer Okay, just pushed. Hopefully this didn't break anything. I haven't squashed yet and created a clone of the previous branch so we have it in case something goes awry. |
|
@acolytec3, unfortunately, conflicts have been resolved incorrectly, some changes from develop were reverted |
|
@flexsurfer Ugh, sorry, I'll revert to my previous commit and try again once my current build finishes. I'm starting from a clean nix build to see if that solves the stream error and it's going to take a while. |
|
also this commit might be helpful to understand what has happened in develop 507cc5c#diff-09bf352f4017403bdf258a3d90eac06a |
|
@flexsurfer can you take a look at this commit and see if it's got the merges correct? 445fa32 I've taken another pass at merging on top of current |
Could you gist the whole log? |
| @@ -0,0 +1,72 @@ | |||
| (ns status-im.ui.screens.wallet.choose-recipient.views | |||
There was a problem hiding this comment.
this view doesn't exist anymore in develop
There was a problem hiding this comment.
Makes sense now that I look at it again. I'll try it again later today.
| @@ -6,11 +6,306 @@ | |||
| [status-im.ui.components.react :as react] | |||
| [status-im.ui.components.styles :as components.styles] | |||
| [status-im.ui.components.toolbar.view :as topbar] | |||
| [status-im.ui.components.tooltip.views :as tooltip] | |||
There was a problem hiding this comment.
most of the code here was deleted in develop
@jakubgs I didn't hang onto the console output from the make target. Is there a build log somewhere it might have been outputted to? I'm not real familiar with the logging from the build process and haven't been able to track it down. |
|
probably it might be better to create a new branch from latest develop and move all your changes there and file a new PR, but it's up to you |
|
@flexsurfer thats an horrible thing, it should be a better way of doing this. I have dealt with merging conflicts in past and it was a matter of accepting the correct changes, and manually merging where it messes up. I am trying to understand here why it broke so bad. |
|
@3esmit It's not a big deal. Once I figure out why my build environment refuses to work, it shouldn't take too long to reconstruct everything. Just a case a bad timing where multiple PRs touching the same code going on at the same time. I think I know what all needs to be updated and can hopefully get it done quickly once I get to the underlying environment issue fixed. @flexsurfer Whatever that stream error was yesterday has completely hosed my setup so I think I'm going to have to start from scratch. I've tried |
|
Some problems Ive noticed in this process:
|
|
@3esmit I haven't done any force commits in the last few days but I did rebase on the latest develop yesterday and that's when everything got hosed. Once my PC finishes reinstalling the build environment, I'll get the PR cleaned up and working again as I still have all the working code from the PR and just need to integrate it with the redesigned interface in the qr flow that @flexsurfer introduced in |
|
@acolytec3 what do you mean by:
I find that really hard to believe. What happens when you do |
|
@acolytec3 what do you use as a target platform for android? (geymotion, avd , real ) |
|
@acolytec3 after some talk with @cammellos I see that it's possible that the process hangs. I though what you were describing was exiting without error. But if it hangs than I'd like to see what processes are present. Could you try the |
|
@cammellos @jakubgs After running https://gist.github.com/acolytec3/8f6c9070889ea6e1acbe4999a3317864 |
|
@cammellos @jakubgs Pause on any further worrying about my make issues. I'm going to let the |
|
@flexsurfer @yenda @3esmit I'm pausing work on this for the moment as I can't get the app to build. Gist is below with the errors I see when trying to build from a completely clean repo cloned from status-react building off the current https://gist.github.com/acolytec3/54ac7c1d085bccc48e6e65eaea94ba75 |
|
@flexsurfer @yenda @churik @3esmit My build issues seems to be sorted. Submitted a clean PR rebased on current develop that imports all of the commits from this PR and seems to be working as expected. I'm going to close this one. |
Summary
EIP681 URI parsing does not currently support ENS name verification. This PR enables the
parse-urifunction 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
Areas that maybe impacted
Wallet/Send-Transaction/QR-Code
Functional
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=1e16Verify that ENS address is resolved to ethereum address and currency changes to SNT.
status: ready