[Wallet] Wallet can switch between hosted and local node #1419

annakaz · 2019-10-21T22:49:19Z

Description

Allow wallet users to switch between a local and a hosted node.

Handle storing the private key in web3.eth.personal (with local node) vs in the WalletKit Celo provider (with hosted node)

Tested

Tested on device on alfajores.
Build that starts in zeroSync mode:
sent tx, turned zeroSync mode off and confirmed that geth starts, sent tx, turned zeroSync mode off and confirmed that geth stops, sent tx

Build that starts in ultralight mode:
sent tx, turned on zeroSync mode, confirmed that geth turns off, sent tx

More testing to be done by the QA team

Other changes

Move verification and invite to generators in order to access zeroSyncMode state

Related issues

Fixes Users SBAT select a hosted node setup when in Low Connectivity #1171

Backwards compatibility

Not with pervious code with ZeroSync mode encoded as DEFAULT_SYNC_MODE=-1.
There are now Separate parameters (DEFAULT_SYNC_MODE and ZERO_SYNC_INITIALLY) in .env file

jmrossy

Great progress! See questions below

jmrossy · 2019-10-22T11:06:03Z

README-dev.md

@@ -4,23 +4,23 @@



yeah it did the same for me, i had to manually revert it. But it's nbd feel free to leave in this PR

jmrossy · 2019-10-22T12:41:32Z

packages/mobile/.env.alfajores

 DEFAULT_SYNC_MODE=5
+ZERO_SYNC_INITIALLY=true


Can you clarify what this config is for exactly? My first guess would be it's whether or not the app should use zero sync when it starts, but that would conflict with the DEFAULT_SYNC_MODE config we already have.

DEFAULT_SYNC_MODE now specifies the sync mode for when geth is turned on
Because geth will turn on and off, DEFAULT_SYNC_MODE can't encode whether the app should use zeroSync initially. Added a comment in the .env file

jmrossy · 2019-10-22T12:45:19Z

packages/mobile/src/web3/contracts.ts

-    throw new Error('addLocalAccount can only be called in Zero sync mode')
+// Mutates web3 with new provider
+export function switchWeb3ProviderForSyncMode(zeroSync: boolean) {
+  if (zeroSync) {


Can we get a bit more logging here? Unless you already have it elsewhere where it's called.

There's already some logging where it's called but I added logging for each provider switch

jmrossy · 2019-10-22T12:49:03Z

packages/mobile/src/web3/saga.ts

+    // Note that if geth is running it saves the key using web3.personal.
+    const privateKey = String(key)
+    account = getAccountAddressFromPrivateKey(privateKey)
+    yield savePrivateKeyToLocalDisk(account, privateKey, pincode)


This makes me nervous. We allowed it for zerosync mode, which I half-assumed was just a temporary thing. but if we're now going to permanently store the user's private key (encrypted by their pincode which could be poor and/or lost), I think we should have a broader discussion about it first. Maybe we already have but I missed it?

Thanks for flagging this. After looking into it, geth also stores the private keys in a file encrypted with the password (for which we use the same pincode), so I will keep this as is for now.
See https://celo-org.slack.com/archives/CL7BVQPHB/p1571763622020000

jmrossy · 2019-10-22T12:49:45Z

packages/mobile/src/web3/saga.ts

-  // TODO(anna) implement switching geth on/off, changing web3 provider
-  return true
+// Stores account and private key in web3 keystore using web3.eth.personal
+export function* addAccountToWeb3Keystore(key: string, currentAccount: string, pincode: any) {


any reason pincode isn't a string?

jmrossy · 2019-10-22T12:51:58Z

packages/mobile/src/web3/saga.ts

+
+export function* switchZeroSyncMode(action: SetIsZeroSyncAction) {
+  Logger.debug(TAG + '@switchZeroSyncMode', ` to: ${action.zeroSyncMode}`)
+  yield call(getConnectedAccount) // Ensure web3 connected before switching provider


why wait for us to be connected first?

jmrossy · 2019-10-22T12:53:04Z

packages/mobile/src/web3/saga.ts

+export function* switchToGethFromZeroSync() {
+  yield call(initGethSaga)
+
+  setZeroSyncMode(false)


are you missing a yield put here?

Yeah thanks for catching this

jmrossy · 2019-10-22T12:53:09Z

packages/mobile/src/web3/saga.ts

+
+export function* switchToZeroSyncFromGeth() {
+  Logger.debug(TAG + 'Switching to zeroSync from geth..')
+  setZeroSyncMode(true)


jmrossy · 2019-10-22T12:54:26Z

packages/mobile/src/web3/saga.ts

+        'Importing account from private key to web3 keystore'
+      )
+      const pincode = yield call(getPincode)
+      const privateKey: string = yield readPrivateKeyFromLocalDisk(currentAccount, pincode)


Nit: yield call?

packages/mobile/src/web3/saga.ts

jmrossy

The code changes lgtm. See my question below about the default value for ZERO_SYNC_ENABLED_INITIALLY and see CI tests, which seem to have failed.

jmrossy · 2019-10-23T12:11:08Z

packages/mobile/.env

-# -1 == ZeroSync, 5 == Ultralight, see src/geth/consts.ts for more info
+# If ZERO_SYNC_ENABLED_INITIALLY, local geth will not run initially. 
+# If toggled on, it will use DEFAULT_SYNC_MODE. See src/geth/consts.ts for more info
+ZERO_SYNC_ENABLED_INITIALLY=true


Thanks for clarifying the this. My next question is then: should this be true? My understanding from my chat with Connor about this a while back was that we'd keep using geth by default during development, so we could catch bugs early (since prod still uses geth)

Good point for usually using geth during development. I'll have this be false for all env files for now.

jmrossy · 2019-10-23T12:15:27Z

packages/mobile/src/web3/saga.ts

+    // Note that if geth is running it saves the key using web3.personal.
+    const privateKey = String(key)
+    account = getAccountAddressFromPrivateKey(privateKey)
+    yield savePrivateKeyToLocalDisk(account, privateKey, pincode)


Most issues addressed

codecov · 2019-10-23T22:44:42Z

Codecov Report

Merging #1419 into master will decrease coverage by 0.38%.
The diff coverage is 39.25%.

@@            Coverage Diff             @@
##           master    #1419      +/-   ##
==========================================
- Coverage   65.86%   65.48%   -0.39%     
==========================================
  Files         271      271              
  Lines        8062     8144      +82     
  Branches      559      560       +1     
==========================================
+ Hits         5310     5333      +23     
- Misses       2639     2697      +58     
- Partials      113      114       +1

Flag	Coverage Δ
#mobile	`65.48% <39.25%> (-0.39%)`	⬇️

Impacted Files	Coverage Δ
packages/mobile/src/account/Account.tsx	`63.41% <ø> (ø)`	⬆️
packages/mobile/src/geth/consts.ts	`100% <ø> (ø)`	⬆️
packages/mobile/src/web3/reducer.ts	`42.85% <0%> (-7.15%)`	⬇️
packages/mobile/src/account/CeloLite.tsx	`100% <100%> (ø)`	⬆️
packages/mobile/src/invite/saga.ts	`80.53% <100%> (+0.39%)`	⬆️
packages/mobile/src/geth/networkConfig.ts	`100% <100%> (ø)`	⬆️
packages/mobile/src/app/ErrorMessages.ts	`100% <100%> (ø)`	⬆️
packages/mobile/src/web3/saga.ts	`42.39% <18.96%> (-7.61%)`	⬇️
packages/mobile/src/transactions/send.ts	`24.48% <35.29%> (+3.55%)`	⬆️
packages/mobile/src/app/saga.ts	`67.18% <44.44%> (-3.73%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a69de8...a615862. Read the comment docs.

* master: [Wallet] Wallet can switch between hosted and local node (#1419) [Wallet] Prevent error from Avatar when name is missing (#1454) [Wallet] Show splash screen until JS is ready on iOS (#1453) Use new segment api keys used by both iOS and Android (#1452) [Wallet] Don't log all props, which includes i18n (#1445) [Helm] Updated the helm package to deploy the upgraded blockscout version (#1129) Tiny copy change (#1429) [contractkit] SortedOraclesWrapper + tests (#1405) [wallet] Refactor leftover thunk to sagas (#1388) [Wallet] Fix repeated QR code scanning and related navigation issues (#1439) [Wallet] Show the currency values with correct rounding. (#1435) [Wallet] Fix firebase initialization error on iOS after reinstalling the app (#1423) [Wallet] Use exit on iOS since we can't restart like Android (#1424) [Wallet] Update local currency styles and layout (#1325) Reset pincode cache if unlock fails (#1430)

annakaz added 30 commits October 11, 2019 10:12

Always write key to disk to allow to switch into zero sync mode

1242a1f

Add celo lite settings component

1e7fa99

Add celo lite settings link

1eaf14d

Update state to keep track of sync mode, account in web3 keystore

5f1443e

Make celo lite settings screen responsive

d170908

WIP switching

e49e37a

Merge branch 'master' into annakaz/zero-sync-option

6e0f4bf

Fixing redux saga logic

304b8c2

Get sagas working

a017e5b

Move zeroSync logic out, refactor assignAccountFromPrivateKey

b34d55d

Moving more logic to web3/zeroSync

b0dc9b3

Merge branch 'master' into annakaz/zero-sync-option

2f9dc16

Yield select whenever zeroSync mode needed

d46e1cf

Switching between providers

8f7db2c

check sync mode via web3 provider

eafce36

switch web3 provider when necessary

e12c0cf

Default to starting in full sync mode

dae95b3

Revert changes to functions using sendTransaction

a9a3dde

Handle isZeroSync mode using web3 object

cfcd1fb

Ensure private key exists/web3 keystore exists for switch

15d7653

Switch to always reading zeroSync state from web3

b71ad0c

Merge branch 'master' into annakaz/zero-sync-toggle

efcfcee

Stop storing account in web3 keystore separately

76abb84

Remove zerosync logic

9b44218

Remove zerosync file

cfa940c

Revert logic changes to save for diffrerent pr

bb210fa

Remove some unused styles

8e5d46b

Comment out celoLite settings for now

d79988c

Add spanish translations, a couple comments

8735f97

Add jest tests

332e24e

annakaz assigned jeanregisser, martinvol, jmrossy and cmcewen Oct 22, 2019

Clean up for review

376920d

jmrossy previously requested changes Oct 22, 2019

View reviewed changes

jmrossy assigned annakaz and unassigned jmrossy Oct 22, 2019

Make changes based on review

9c7fd33

jmrossy reviewed Oct 23, 2019

View reviewed changes

annakaz added 6 commits October 23, 2019 13:01

Merge branch 'master' into annakaz/zero-sync-toggle-logic

f2bf8c1

Remove unnecessary yield puts causing loop"

5bc42c5

Turn geth off, fix tests, use back button in header

30fa17d

Make zeroSyncInitially false for all env files

59ae0f6

Merge branch 'master' into annakaz/zero-sync-toggle-logic

9fbb5c8

Wait for web3 to sync after switching to geth

5faf4b6

PR cleanup

d45f1e6

annakaz unassigned annakaz, jeanregisser and martinvol Oct 23, 2019

jmrossy approved these changes Oct 24, 2019

View reviewed changes

Merge branch 'master' into annakaz/zero-sync-toggle-logic

a615862

annakaz assigned annakaz and unassigned cmcewen Oct 24, 2019

annakaz added the automerge Have PR merge automatically when checks pass label Oct 24, 2019

celo-ci-bot-user merged commit 1c82ae6 into master Oct 24, 2019

celo-ci-bot-user deleted the annakaz/zero-sync-toggle-logic branch October 24, 2019 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Wallet] Wallet can switch between hosted and local node #1419

[Wallet] Wallet can switch between hosted and local node #1419

annakaz commented Oct 21, 2019 •

edited

Loading

jmrossy left a comment

jmrossy Oct 22, 2019

jmrossy Oct 22, 2019

annakaz Oct 22, 2019

jmrossy Oct 22, 2019

annakaz Oct 22, 2019

jmrossy Oct 22, 2019

annakaz Oct 22, 2019

jmrossy Oct 23, 2019

jmrossy Oct 22, 2019

jmrossy Oct 22, 2019

jmrossy Oct 22, 2019

annakaz Oct 22, 2019

jmrossy Oct 22, 2019

jmrossy Oct 22, 2019

annakaz Oct 22, 2019

jmrossy left a comment

jmrossy Oct 23, 2019

annakaz Oct 23, 2019

jmrossy Oct 23, 2019

codecov bot commented Oct 23, 2019 •

edited

Loading

		DEFAULT_SYNC_MODE=5
		ZERO_SYNC_INITIALLY=true

[Wallet] Wallet can switch between hosted and local node #1419

[Wallet] Wallet can switch between hosted and local node #1419

Conversation

annakaz commented Oct 21, 2019 • edited Loading

Description

Tested

Other changes

Related issues

Backwards compatibility

jmrossy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrossy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 23, 2019 • edited Loading

Codecov Report

annakaz commented Oct 21, 2019 •

edited

Loading

codecov bot commented Oct 23, 2019 •

edited

Loading