-
Notifications
You must be signed in to change notification settings - Fork 985
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
Add universal qr scanner #11014
Add universal qr scanner #11014
Conversation
Jenkins BuildsClick to see older builds (31)
|
d608f7d
to
f8ec3f6
Compare
f8ec3f6
to
26baf8b
Compare
98% of end-end tests have passed
Failed tests (2)Click to expand
Passed tests (95)Click to expand |
@Ferossgp can you pleae rebase it? thanks! |
26baf8b
to
497de1b
Compare
ISSUE 1: on scanning chat keys or universal links app is redirected to profile view with profile of another userAnd to see again your profile you need to go back. Expected result: Actual result: ISSUE 2: unable to scan valid QR after scanning invalid ENS nameSteps:
Expected result: ISSUE 3: on scanning deep link with public chat - null is not an object errorSteps:
Expected result: ISSUE 4: doesn't support universal links to dapps with full URLsSteps:
Expected result: Actual result: ISSUE 5: error at attempt to scan EIP linkSteps:
Expected result: Actual result: |
Cannot replicate, maybe there was some context before that action, or can you please share INFO logs? All other issues fixed |
04a7513
to
d576e72
Compare
d576e72
to
872ccb5
Compare
@Ferossgp thanks for fixes! About other issues: ISSUE 5: EIP links are still not fully supported (for tokens, ENS names, payment links)More context of supported params is here: #10280 Links for testing:
Test case is here: https://ethstatus.testrail.net/index.php?/cases/view/6282 Also in case of scanning valid wallet address as it mentioned in #10764 should open If users scans wallet address open Wallet > Send > To field includes address. ISSUE 6: after scanning another user profile scanning your profile key redirects to another user profileSteps:
Expected result: Actual result: |
@churik fixed |
(navigation/navigate-to-cofx :tabs {:screen :wallet}))) | ||
|
||
(fx/defn match-scan | ||
{:events [::match-scanned-value]} |
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.
could you please elaborate why handlers are here and not in the router ns ?thanks
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.
should it be handled different if we scan it or if we open universal link?
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.
Handlers are almost similar, the only difference is on errors strings. Also in some places we do not show error in Universal Links, while in QR we say that we can't scan this code
29a35bd
to
e4c00ee
Compare
@churik fixed wallet scanner |
90% of end-end tests have passed
Failed tests (10)Click to expand
Passed tests (87)Click to expand
|
Tested universal code scanner + existing QR code scanners from chat view, wallet view, bootnode, mailserver QR code scanner on IOS 13 and Android 10 Awesome work! |
@Ferossgp new issue with redirect to user profile from universal link: Thanks for the clue! |
Fixed and verified. Ready to be merged! |
Rename events Add router to handle all links Use router in add new chat Unify universal link and universal qr with router Add icon for universal scanner Update tests Now routing is tested in routing PR lint Cleanup QA fixes Scan own profile Handle more EIP Fix wallet scanner Fix stack for view profile in UL Signed-off-by: Gheorghe Pinzaru <feross95@gmail.com>
f720518
to
7935798
Compare
Closes #10764
Added bidi to handle routes instead of regexp. Also unified the QR and Universal links, to use same routing as they use the same links.