Bugfix/deeplink, request modals and wallet connect by estebanmino · Pull Request #1877 · MetaMask/metamask-mobile

estebanmino · 2020-10-07T00:08:32Z

Description

This PR includes

refactor of the Entry and Main components to hooks. The problem with these component was the state handling, it was messy so we were having problems with all the modals on Main.
WalletConnect version update and added a way to handle wallet connect requests when the app is locked.
Deeplinks are being erased if the app is closed, to not get the request when the app is opened again without context.

Checklist

There is a related GitHub issue
Tests are included if applicable
Any added code is fully documented

Issue

Resolves #1798
Resolves #1804

This reverts commit 3f7820d.

ibrahimtaveras00

Android doesn't progress pass splash screen
iOS I'm unable to get any deeplinks to work

built with METAMASK_ENVIRONMENT='production' yarn build:ios:release and detox build -c android.emu.release && detox test -c android.emu.release

rickycodes

left a few comments/question. admittedly, I need to brush up on react hooks... most of where we used them previously i was able to understand, but there's a lot of changes here. I may need to take another look at this tomorrow with a fresh set of eyes.

rickycodes · 2020-10-08T05:50:49Z

app/core/WalletConnect.js

 				this.redirectIfNeeded();
 			}
+			// Clean call ids
+			tempCallIds = [];


instead of reassigning we could make tempCallIds a const and then empty it by setting length property to zero

const arr = ["foo","bar"] undefined arr Array [ "foo", "bar" ] arr.length 2 arr.length = 0 0 arr Array []

rickycodes · 2020-10-08T06:04:54Z

app/core/WalletConnect.js

+const waitForKeychainUnlocked = async () => {
+	let i = 0;
+	const { KeyringController } = Engine.context;
+	while (!KeyringController.isUnlocked()) {


does the KeyringController not have an unlock event we can listen for? checking every second for a whole minute seems not ideal 🤔

i see on the extension KeyringController extends EventEmitter which seems like a better pattern.

we don't have that on controllers KeyringController

rickycodes · 2020-10-08T06:19:19Z

app/components/Nav/Main/index.js

+				transactionMeta.transaction.gasPrice = hexToBN(gasPrice);
+
+				if (
+					(value === '0x0' || !value) &&


rather than doing all the magic directly in an if condition what if instead we outline what the constraints are separately? I think that will make things a bit easier to understand.

something like:

const valueIsEmpty = value === '0x0' || !value; const dataIsNotEmpty = data && data !== '0x'; ... if (valueIsEmpty && dataIsNotEmpty && ...)

this is payment channel stuff which we're deprecating, I think we shouldn't spend time on this (in fact I was deleting it acd5d43)

rickycodes · 2020-10-08T06:23:05Z

app/components/Nav/Main/index.js

+			if (
+				props.paymentChannelsEnabled &&
+				AppConstants.CONNEXT.SUPPORTED_NETWORKS.includes(props.providerType) &&
+				transactionMeta.transaction.data &&


does transactionMeta always have a transaction key? what happens if it doesn't? you could use optional chaining here to be safe

payment channel stuff

rickycodes · 2020-10-08T06:28:05Z

app/components/Nav/Main/index.js

-			}, 1000);
-		});
+	const initForceReload = () => {
+		// Force unmount the webview to avoid caching problems


Why do we have to do this?

is to mount MainNavigator on language change

ibrahimtaveras00 · 2020-10-09T00:21:56Z

After retesting, here are the following issues:

Issue 1:

Android still isn't moving past the splash screen

Issue 2:

I'm unable to open deeplinks/Wallet Connect when the app is either in the background and locked, or completely in a killed state

also too, as a side note, I'm in a broken state when I attempt to do this, seen here = http://recordit.co/Flpx8M1fyG
the app stays with a white loading screen on wallet, and none of my account information is displayed

This currently works on iOS build 525 seen here = http://recordit.co/67bKuGHqLF

Issue 3:

When you are going via Wallet connect (in this scenario I had uniswap.exchange open on Safari and came to MM via wallet connect), it shows the wrong title URL on the approve modal. Furthermore, when I kill the app and relaunch, I'm no longer able to see the ERC-20 token I want to swap to; seen here = https://recordit.co/zPi5Z8hxOo

You can also follow this thread for more information

ibrahimtaveras00

All issues have been resolved on Android

With iOS, the only outstanding issue is that in a killed, or locked state deeplinks will not work on a device that has biometrics enabled (FaceID or TouchID)

similar to #1798

ibrahimtaveras00

Only 1 scenario fails on both OS's:

When you have the app in a locked state and you're not on the wallet view

ios = http://recordit.co/8sEuCj1qyp
Android = https://recordit.co/LnIdgLJavd

This nonetheless works if you have a device without biometrics and use your password to log in

ibrahimtaveras00

Wallet connect is now fixed on both OS's 👍

However, there are now 2 places of failure on both OS's

Payment request isn’t working when app is in a locked state (set to immediately)

Seen here = http://recordit.co/uyQfcwuGXF

As you can see it opens to the correct view then immediately closes the view

Scanning a public address is not working as it should
Ever since #1859 when a user scans a public address QR code, they should be sent to the send to view

Seen here = http://recordit.co/JmRNIYrhl8

ibrahimtaveras00

fixes look good on both OS's, QA Passed 👍

* hooksentry * handleappsyayedeeplinkmanager * branch * rm payment channel stuff * Revert "rm payment channel stuff" This reverts commit 3f7820d. * remove payment channel stuff * works * update * init once and bump wc| * wc temp call ids * waitForKeychainUnlocked * paymentchannelstuff * snaps * tempcallids * build * :527 * android * bumpandfixes * CURRENT_PROJECT_VERSION * circleci * pod * resource_class * bump * builds * bump * dl manager to main * unlock * unlock * snaps * pass to root * fix titlw * finally * 534 * lockout * scan * android * test * finally * bump * snaps * lock * browser * goToNewTab * passdeeplink to main| * 538 * fix * 539 * locked * 541 * bump * 542 * fixaddress * drwaer * 543 * fix * 544 * fix * a * test

estebanmino added 12 commits September 29, 2020 19:21

hooksentry

7ffa514

handleappsyayedeeplinkmanager

7f78013

Merge branch 'develop' into bugfix/deeplink-modals

b65e843

branch

261e1f6

rm payment channel stuff

3f7820d

Revert "rm payment channel stuff"

53075b1

This reverts commit 3f7820d.

remove payment channel stuff

bbb7e99

works

06cd635

update

1f5a87e

init once and bump wc|

9960bc1

wc temp call ids

77ac923

waitForKeychainUnlocked

eb43cf6

estebanmino requested a review from a team as a code owner October 7, 2020 00:08

estebanmino added 3 commits October 6, 2020 22:08

paymentchannelstuff

acd5d43

snaps

64226b9

Merge branch 'develop' into bugfix/deeplink-modals

92defd0

ibrahimtaveras00 added needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 7, 2020

ibrahimtaveras00 suggested changes Oct 7, 2020

View reviewed changes

ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels Oct 7, 2020

rickycodes suggested changes Oct 8, 2020

View reviewed changes

estebanmino added 3 commits October 8, 2020 09:05

tempcallids

0fc20f1

build

4ddf6ff

:527

6715037

ibrahimtaveras00 added the next release label Oct 8, 2020

estebanmino added 2 commits October 14, 2020 12:28

android

8891519

bumpandfixes

f456d07

538

6ba67ac

ibrahimtaveras00 suggested changes Oct 30, 2020

View reviewed changes

estebanmino added 2 commits November 2, 2020 14:08

fix

4f7db1e

539

23ea80a

estebanmino added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Nov 2, 2020

ibrahimtaveras00 suggested changes Nov 2, 2020

View reviewed changes

ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Nov 2, 2020

estebanmino added 4 commits November 2, 2020 19:57

locked

cbe54b2

541

dc70d61

bump

40ac996

542

fd8fc8e

ibrahimtaveras00 suggested changes Nov 3, 2020

View reviewed changes

estebanmino added 9 commits November 3, 2020 23:52

fixaddress

f4af57e

drwaer

3a8886d

543

d97e2c1

merge

35e18a8

fix

557a294

544

8eb90a9

fix

524fdf1

merge

e14cc78

a

2692131

ibrahimtaveras00 approved these changes Nov 5, 2020

View reviewed changes

ibrahimtaveras00 added QA Passed QA testing has been completed and passed and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Nov 5, 2020

test

6cac900

estebanmino merged commit f3a066e into develop Nov 5, 2020

estebanmino deleted the bugfix/deeplink-modals branch November 5, 2020 00:27

Uh oh!

Conversation

estebanmino commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

Uh oh!

rickycodes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibrahimtaveras00 commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibrahimtaveras00 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

Uh oh!

ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

Uh oh!

ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

estebanmino commented Oct 7, 2020 •

edited

Loading

ibrahimtaveras00 commented Oct 9, 2020 •

edited

Loading

ibrahimtaveras00 left a comment •

edited

Loading