-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix wallet_scanQRCode rpc method #1758
Conversation
result = data.target_address; | ||
} else if (data.scheme) { | ||
result = JSON.stringify(data); | ||
await new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think you can simplify:
wallet_scanQRCode: () => new Promise((resolve, reject) => { ... })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
if (regex && !regex.exec(data)) { | ||
reject({ message: 'NO_REGEX_MATCH', data }); | ||
} else if (!regex && !/^(0x){1}[0-9a-fA-F]{40}$/i.exec(data.target_address)) { | ||
reject({ message: 'INVALID_ETHEREUM_ADDRESS', data: data.target_address }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these error names standard? I couldn't find these on the link you posted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR doesn't technically implement the EIP as it's outlined anyway, but there doesn't appear to be a standard for the error names
@EtDu I went ahead and followed the instructions to reproduce here and I'm still seeing that it's not detected via the link https://cipher-qr-scan.netlify.com/ I also tried the instructions here as well and doesn't work either, with the following warnings: I also tried https://ropsten.kyber.network transfer option on iOS and it still doesn't support it Any other dapps you know of to test this with? |
@ibrahimtaveras00 To test this, open a webview debug console and type |
@EtDu I get this when I tap X to close the scanner on browser = http://recordit.co/TLdR6OWJxX Same on iOS = http://recordit.co/8IbXZ81Phe |
@ibrahimtaveras00 That's an internal error that does not appear in production and has existed before this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* browser tab: fix wallet_scanQRCode rpc method: wrap in a promise * BrowserTab: implement regex for scanQRCode * BrowserTab: return promise in wallet_scanQRCode
Description
Calling
provider.request({ method: 'wallet_scanQRCode' })
was returning the error:'RPC Error: Error: JsonRpcEngine - response has no error or result for request:'
because it wouldn't wait for the QR code to be scanned before passing the response as empty. Now it properly waits for the user to scan before passing the given result.
This also adds the option for a regex parameter outlined in ethereum/EIPs#945 to enable arbitrary (non ETH address) QR code scanning.
Will reply and close #1485 after this is merged.
Issue
https://trello.com/c/J58J4xOC/77-bug-scan-qr-code-missing