Sign up + LockScreen #28

brunobar79 · 2018-08-07T22:01:56Z

Description
This PR relates to account creation and listing.

User password stored in the keychain and can be unlocked with FaceId / TouchId / Device Pin code
Autolock the app once the app enters into background mode
Unlock keychain when the app goes into foreground
Vault creation
Login
Account listing
Add account

DEMO:

Checklist

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

Issue

bitpshr

Looks great! Very clean PR, mostly nit comments. Nothing should hold this up, so I'll approve. Feel free to fix or not fix these comments.

bitpshr · 2018-08-08T13:39:48Z

app/components/AccountList/index.js

+	onAccountChange = async newIndex => {
+		try {
+			this.setState({ selectedAccountIndex: newIndex });
+			await Engine.api.preferences.update({ selectedAddress: Object.keys(this.props.accounts)[newIndex] });


I worry about the order of these operations. I understand we're updating the UI instantly for speed then calling the background update, but maybe we should revert the UI to the previous address in the catch. That way we don't run the risk of this component's selectedAccountIndex getting out of sync with the preference controller's selectedAddress.

Whatever we decide, I think this should be a pattern throughout when interacting with the Engine.

That was a good call. I feel like we should do this everywhere

bitpshr · 2018-08-08T13:59:28Z

app/components/AccountList/index.js

+	addAccount = async () => {
+		try {
+			await Engine.api.keyring.addNewAccount();
+			this.setState({ selectedAccountIndex: Object.keys(this.props.accounts).length - 1 });


Same comment re: catching & reverting when interacting with the engine.

In this case isn't necessary because if the engine blows up it will never change the index.

bitpshr · 2018-08-08T14:00:19Z

app/components/AccountList/index.js

+		}
+	};
+
+	accountSettings = () => false;


Nit: can we name this so it's obvious it's a method? openAccountSettings or something similar?

bitpshr · 2018-08-08T14:01:31Z

app/components/AccountList/index.js

+	icon: {
+		height: 50,
+		width: 10,
+		backgroundColor: colors.concrete
 	}
 });

 /**
 * View contains the list of all the available accounts
 */



Nit: I don't think components can have a newline between the component description and the class for documentation generation. Not 100% sure, but that's been the case in the past.

Shit, I've been doing this all over the place!

bitpshr · 2018-08-08T14:03:45Z

app/components/App/index.js

 import Main from '../Main';
 import AccountList from '../AccountList';
-
+import Engine from '../../core/Engine';
 /**
 * Root application component responsible for instantiating


Nit: Can we update this to be more descriptive of the conditional rendering logic and move this description to the exported class, not the Nav component?

bitpshr · 2018-08-08T14:11:19Z

app/core/Encryptor.js

+
+	_decryptWithKey = (encryptedData, key) => Aes.decrypt(encryptedData.cipher, key, encryptedData.iv);
+
+	encrypt = async (password, object) => {


Can we document this since it's public?

bitpshr · 2018-08-08T14:11:23Z

app/core/Encryptor.js

+		return JSON.stringify(result);
+	};
+
+	decrypt = async (password, encryptedString) => {


Can we document this since it's public?

bitpshr · 2018-08-08T14:11:32Z

app/core/Encryptor.js

+import { NativeModules } from 'react-native';
+const Aes = NativeModules.Aes;
+
+export default class Encryptor {


Can we add a simple description to this class?

bitpshr · 2018-08-08T14:12:07Z

app/core/Engine.js


 /**
 * Core controller responsible for composing other GABA controllers together
 * and exposing convenience methods for common wallet operations.
 */
+


Nit: can we move this description to right above the Engine class? Otherwise documentation may get generated incorrectly.

bitpshr · 2018-08-08T14:13:12Z

shim.js

@@ -1,3 +1,5 @@
+import {decode, encode} from 'base-64'
+


Let's chat as to why this needs to be included int the repo (if it's generated at least; if it's not generated, we should format it and lint it properly.) Not a big deal, just curious.

If you move the lines 1-15 out of the shim.js to the./ index.js you'll see you get an error: self is not defined. I think it's related to the way "import" works.

Bumps [node-notifier](https://github.com/mikaelbr/node-notifier) from 8.0.0 to 8.0.1. - [Release notes](https://github.com/mikaelbr/node-notifier/releases) - [Changelog](https://github.com/mikaelbr/node-notifier/blob/v8.0.1/CHANGELOG.md) - [Commits](mikaelbr/node-notifier@v8.0.0...v8.0.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* ready to create accounts * account generation working * added login screen * avoid setstate when unmounted * clean up * more clean up * account list using real addresses * account list using real addresses * clean up * update tests * clean up * fix android builds * android and ios are working * android view looking good * UI refactor * code review stuff * update * more code review stuff

As all runtimes now support atob and btoa, the base-64 shim is not used. Originally introduced in #28 (b08e380). - https://developer.mozilla.org/en-US/docs/Web/API/atob#browser_compatibility - https://developer.mozilla.org/en-US/docs/Web/API/btoa#browser_compatibility

brunobar79 added 13 commits August 6, 2018 23:19

ready to create accounts

fa81112

account generation working

56c9dc9

added login screen

8a7ee2c

avoid setstate when unmounted

a544302

fix merge conflicts

d25872f

clean up

1a300ec

more clean up

0f1801c

fix merge conflicts

4263fab

account list using real addresses

844630a

account list using real addresses

322e4cb

clean up

12ba618

update tests

e85ee0d

clean up

29f9802

brunobar79 changed the title ~~[DO NOT MERGE] - Sign up + LockScreen~~ Sign up + LockScreen Aug 8, 2018

brunobar79 requested a review from bitpshr August 8, 2018 04:55

fix android builds

ab4db10

bitpshr approved these changes Aug 8, 2018

View reviewed changes

brunobar79 added 6 commits August 8, 2018 20:04

android and ios are working

99d28fc

android view looking good

49c8324

UI refactor

410debf

code review stuff

acc9a60

update

c9a7910

more code review stuff

8eac921

brunobar79 merged commit b08e380 into master Aug 9, 2018

This was referenced Aug 9, 2018

Use Keychain for private key storage #10

Closed

Lock app and require authentication while coming from background #17

Closed

This was referenced Nov 19, 2018

Create Password screen #71

Closed

Login / Unlock screen #70

Closed

Account creation & import #69

Closed

brunobar79 deleted the lockscreen branch February 11, 2019 21:39

brunobar79 mentioned this pull request Feb 19, 2019

Temp fix for build #29 #409

Merged

legobeat mentioned this pull request May 13, 2024

chore: remove unused base-64 shim #9617

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sign up + LockScreen #28

Sign up + LockScreen #28

brunobar79 commented Aug 7, 2018 •

edited

Loading

bitpshr left a comment

bitpshr Aug 8, 2018

brunobar79 Aug 9, 2018

bitpshr Aug 8, 2018

brunobar79 Aug 9, 2018

bitpshr Aug 8, 2018

bitpshr Aug 8, 2018

brunobar79 Aug 9, 2018

bitpshr Aug 8, 2018

bitpshr Aug 8, 2018

bitpshr Aug 8, 2018

bitpshr Aug 8, 2018

bitpshr Aug 8, 2018

bitpshr Aug 8, 2018

brunobar79 Aug 9, 2018


		_decryptWithKey = (encryptedData, key) => Aes.decrypt(encryptedData.cipher, key, encryptedData.iv);

		encrypt = async (password, object) => {

Sign up + LockScreen #28

Sign up + LockScreen #28

Conversation

brunobar79 commented Aug 7, 2018 • edited Loading

bitpshr 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

brunobar79 commented Aug 7, 2018 •

edited

Loading