Parity Signer #1349

maciejhirsz · 2018-03-20T17:06:15Z

Description

Introduces Parity Signer as another secure wallet option on the main screen. Account is unlocked by scanning a QR code of the address from the mobile app. Signing transactions is done in a modal overlaying the UI that first displays the QR code of the RLP of the transaction and then scans for the signature QR code from the mobile app.

Changes

New secure wallet has been added.
New modal has been added to allow signing of the transaction while the Promise is being resolved.
New reducers and actions have been added in order to pipe through the transaction from the UI to the Promise required by the IFullWallet interface.
Added @parity/qr-signer as a new dependency.

Steps to Test

Download the Parity Signer mobile app (iOS / Android). Create an account on it, send some ETH to the account (works with testnets).
Click on the "Parity Signer" option on the main screen.
Scan the account QR code from Parity Signer to unlock the wallet.
Create any kind of a transaction.
Sign the transaction using the app.
Make sure that the transaction goes through and is mined on chain.

Due to interactions with the webcam, integration tests might be tricky unless a lot of the stuff gets mocked away.

TODO / Known issues

Needs a proper icon on the home screen.
ParitySigner.tsx needs to be moved to the containers.
UI needs some love (things are off-center, some paddings would be nice, some copy explaining what it is with links to the app would be nice).
Parity Signer needs a help website (linking to README.md for now...).

Screenshots

…igner

dternyak · 2018-03-21T18:04:20Z

Exciting to see this in action! 😁

@maciejhirsz Are there any plans to support testnets on the parity mobile signer?

Additionally, we'll want to disable Parity when non-ethereum networks are selected (e.g. disable parity option when network is ETC).

We'll also want to disable Parity Signer as an option for signing messages, similar to how we disable Trezor (I see there is a red alert once parity is selected to sign a message, but it would be better disable the option altogether)

maciejhirsz · 2018-03-22T14:43:10Z

Testnets are already supported, or did you run into issues? The UI doesn't separate accounts by network, but the chainId is correctly handled when signing the transaction.

Signing messages should be possible with the Parity Signer, I haven't investigated that yet, the UI (on the mobile app) might be a bit dumb about it, so will disable the panel there for now. Also will look at disabling it for non-Ethereum networks.

dternyak · 2018-03-22T20:31:57Z

Got it. I didn't attempt a testnet tx as I didn't see a way to select testnet on the app, but it's good to see that's supported.

👍 to message signing as a later functionality.

maciejhirsz · 2018-03-26T13:13:56Z

@dternyak I've been looking into it, and I don't really see any issues why the signer shouldn't be able to handle ETC and other networks as well. Basically anything that can be handled by keystore file / privatekey currently, should work with the app, unless there are some differences in how those networks sign txs/messages that I'm unaware of?

…igner

maciejhirsz · 2018-03-26T15:01:43Z

Updated the PR:

All UI stuff is fixed, latest develop merged back.
Disabled Parity Signer for signing transactions (subject to future PR).
Removed the [WIP] 🎉.

FYI: there will be a nicer logo / icon in near future.

wbobeirne

Definitely let me know if any of this feedback seems off base!

wbobeirne · 2018-03-26T15:33:01Z

common/components/Footer/index.tsx

@@ -219,6 +220,7 @@ export default class Footer extends React.PureComponent<Props, State> {
          </div>
        </footer>

+        <QrSignerModal />


Would prefer this component only be rendered where it contextually makes sense, somewhere in the send form.

wbobeirne · 2018-03-26T15:33:57Z

common/components/WalletDecrypt/disables.ts

    reasons: {
      [SecureWalletName.TREZOR]: 'This wallet can’t sign messages',
+      [SecureWalletName.PARITY_SIGNER]: 'This wallet can’t sign messages',


Not your responsibility, but just a reminder that we should be translating this.

wbobeirne · 2018-03-26T15:35:36Z

common/containers/ParitySignerDecrypt/index.scss

+        margin: 0 0.4em;
+    }
+
+    .qr-bounds {


We use SuitCSS naming convention to avoid class name conflicts. Likewise, the a img above would be better suited by class names.

wbobeirne · 2018-03-26T15:36:02Z

common/containers/ParitySignerDecrypt/index.tsx

+        </div>
+        <p>{translate('ADD_PARITY_2')}</p>
+        <p>
+          <a href="https://itunes.apple.com/us/app/parity-signer/id1218174838" rel="nofollow">


Use the NewTabLink component here.

wbobeirne · 2018-03-26T15:37:09Z

common/containers/ParitySignerDecrypt/index.tsx

+            styles={{ display: 'inline-block' }}
+          />
+        </div>
+        <p>{translate('ADD_PARITY_2')}</p>


At some point we should put together a help article that explains this process, or have some additional descriptive copy here.

We are working on a proper help page that describes how to use the signer, I'll add a link to it here once it's done (ideally before publicity but can be done after the PR is merged).

wbobeirne · 2018-03-26T15:38:16Z

common/containers/ParitySignerDecrypt/index.tsx

+    if (!isValidETHAddress(address)) {
+      console.error('Invalid address!');
+      return;
+    }


What's the user experience for when this happens? Does the signer keep watching for a potentially valid address?

Yep, it will keep on scanning until a QR code with a valid address is presented.

wbobeirne · 2018-03-26T15:38:41Z

common/containers/QrSignerModal/index.scss

@@ -0,0 +1,8 @@
+.QrSignerModal {
+  .qr-bounds {


Prefer SuitCSS naming convention.

wbobeirne · 2018-03-26T15:42:38Z

common/containers/QrSignerModal/index.tsx

+    // values in: wallets/non-deterministic/parity.ts
+    tx.v = Buffer.from([tx._chainId]);
+    tx.r = Buffer.from([0]);
+    tx.s = Buffer.from([0]);


Looks like you're mutating the tx props object here. If you want to alter something passed in as props, you'll need to copy it and save it in state. Something like...

public componentDidMount() { if (this.props.tx) { this.makeStateTx(this.props); } } public componentWillReceiveProps(nextProps: Props) { if (nextProps.tx && this.props.tx !== nextProps.tx) { this.makeStateTx(nextProps); } } private makeStateTx(props: Props) { const tx = { ...props.tx }; tx.v = Buffer.from([tx._chainId]); tx.r = Buffer.from([0]); tx.s = Buffer.from([0]); this.setState({ tx }); }

The values aren't read anywhere down the line, so it shouldn't really matter. Ideally there would be a way to serialize a transaction to RLP in ethereumjs without including the signature (tx.serialize(false) to match tx.hash(false)), but that's not the option. I can make a helper function for it here, and make a PR to ethereumjs.

The problem is it makes for non-deterministic redux state, makes it so there's no way to determine if the tx object has changed (without checking deep equality) and in general is just advised against in React.

If you want to avoid the state boilerplate, it could at least do

const tx = { ...this.props.tx };

in render.

wbobeirne · 2018-03-26T15:46:42Z

common/libs/wallet/non-deterministic/parity.ts

+        resolve(tx.serialize());
+      };
+
+      this.setWalletQrTransaction(tx, from, onSignature, onCancel);


Is there a reason this is a callback argument in constructor, instead of just resolving the promise with these variables? Would be nice if the wallet was more agnostic to where it gets used, so that you could use the same wallet instance to sign transactions with two different implementations.

Yeah, this is the part I'm least happy about, and it goes along with how QrSignerModal is done. The callbacks go up to the container and are in turn placed in redux state for the modal to use.

The component that instantiates the wallet (ParitySignerDecrypt container in this case) has no knowledge of the modal, and is unmounted after unlocking so it cannot host the QrSignerModal itself, hence I'm passing these things over redux, which works but also feels over-engineered for my tastes.

Ideally I'd be able to pass the callbacks from the wallet to the modal as props without going through all these gymnastics, but I haven't been able to figure out how to do it any cleaner so far (my understanding of the existing code base is still pretty shallow).

We came up with a migration strategy to move these things to Redux via slack DMs. Those changes probably won't land for a little while though.

wbobeirne · 2018-03-26T15:48:49Z

common/reducers/wallet.ts

 export const INITIAL_STATE: State = {
  inst: null,
  config: null,
+  signViaQr: null,


IMO (Would definitely like additional opinions) this should probably be its own reducer with its own set of actions / sagas. I think this is pretty separate from the other wallet processes, and is specific to only the parity signer.

I definitely see maintainability improvements with such a separation of concerns 👍

…igner

coveralls · 2018-04-06T17:06:47Z

Coverage decreased (-0.4%) to 33.438% when pulling 3f4a578 on maciejhirsz:paritysigner into c65296d on MyCryptoHQ:develop.

wbobeirne

Great refactor, tested on https://mycryptobuilds.com/047b7a9ff762bb9b7634a1cd886ce9af53f05cfa and it still works great.

wbobeirne · 2018-04-06T16:58:59Z

common/actions/paritySigner/actionCreators.ts

+export type TFinalize = typeof finalize;
+export function finalize(signature: string | null): types.FinalizeAction {
+  return {
+    type: TypeKeys.PARITY_SIGNER_FINALIZE,


Very minor, but I think it would improve code understanding if the word "signature" was a part of this action as well.

wbobeirne · 2018-04-06T16:59:22Z

common/containers/QrSignerModal/index.tsx

+  isOpen: true;
+  rlp: string;
+  from: string;
+  finalize: TFinalize;


We usually split out props into props that get specified by parent component (OwnProps), props that are state from redux (StateProps), and props that are actions (ActionProps), so that we can properly type everything. See something like TXMetaDataPanel for example.

wbobeirne · 2018-04-06T17:00:34Z

common/libs/wallet/non-deterministic/parity.ts


-export default class ParityWallet implements IFullWallet {
+export default class ParitySignerWallet implements IFullWallet {


Appreciate the consistent nomenclature 👍

wbobeirne · 2018-04-06T17:03:47Z

common/components/WalletDecrypt/WalletDecrypt.tsx

@@ -177,7 +177,7 @@ const WalletDecrypt = withRouter<Props>(
        component: ParitySignerDecrypt,
        initialParams: {},
        unlock: this.props.setWallet,
-        helpLink: 'https://github.com/paritytech/parity-signer/blob/master/README.md'
+        helpLink: 'https://wiki.parity.io/Parity-Signer-Mobile-App-MyCrypto-tutorial.md'


I'm assuming this will go online after launch?

… and ismore robust about adding / removing cameras.

Paritysigner adjustments

…igner

maciejhirsz added 3 commits March 20, 2018 16:39

Parity Signer Squashed

b9a6319

ParitySigner to be a container

83feb9a

Merge branch 'develop' of github.com:MyCryptoHQ/MyCrypto into paritys…

8749a8a

…igner

maciejhirsz added 2 commits March 26, 2018 16:19

Parity Signer: style and polish

ac6149b

Merge branch 'develop' of github.com:MyCryptoHQ/MyCrypto into paritys…

5ee0013

…igner

maciejhirsz changed the title ~~[WIP] Parity Signer integration~~ Parity Signer integration Mar 26, 2018

maciejhirsz mentioned this pull request Mar 26, 2018

Sort accounts by networks novasamatech/parity-signer#87

Closed

target blank on appstore links

b73a4b4

wbobeirne suggested changes Mar 26, 2018

View reviewed changes

PR fixes

4a7dd0f

dternyak changed the title ~~Parity Signer integration~~ Parity Signer Mar 26, 2018

maciejhirsz added 5 commits March 28, 2018 15:18

Move QrSignerModal to SendTransaction container

ecdfab7

Rework redux, use signing saga

e04e2e6

Cleanup

a7380d0

Merge branch 'develop' of github.com:MyCryptoHQ/MyCrypto into paritys…

1979c68

…igner

Use new logo, change helpLink

047b7a9

wbobeirne previously approved these changes Apr 6, 2018

View reviewed changes

wbobeirne mentioned this pull request Apr 6, 2018

No Private Keys Online #1466

Merged

3 tasks

wbobeirne and others added 3 commits April 6, 2018 15:29

Rework finalize actions and types a bit

89397e9

Merge branch 'develop' into paritysigner-adjustments

42323be

Webcam info + wiki link on unlock screen

0753ce8

maciejhirsz dismissed wbobeirne’s stale review via 0753ce8 April 6, 2018 19:55

wbobeirne added 2 commits April 6, 2018 16:46

Make the Parity QR Signer its own component, that has error messaging…

5c4396d

… and ismore robust about adding / removing cameras.

Merged paritysigner back in.

e64aa22

Unneded l10n

05419a6

wbobeirne mentioned this pull request Apr 6, 2018

Paritysigner adjustments maciejhirsz/MyCrypto#2

Merged

Merge pull request #2 from MyCryptoHQ/paritysigner-adjustments

234d199

Paritysigner adjustments

wbobeirne previously approved these changes Apr 6, 2018

View reviewed changes

Merge branch 'develop' of github.com:MyCryptoHQ/MyCrypto into paritys…

3f4a578

…igner

maciejhirsz dismissed wbobeirne’s stale review via 3f4a578 April 6, 2018 21:22

dternyak approved these changes Apr 6, 2018

View reviewed changes

dternyak merged commit 307e941 into MyCryptoHQ:develop Apr 6, 2018

lexfrl mentioned this pull request Jul 8, 2018

Parity Signer integration openethereum/fether#146

Closed

lexfrl mentioned this pull request Oct 18, 2018

[Parity Signer] Do not ask to scan an address twice for a known account #2220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parity Signer #1349

Parity Signer #1349

maciejhirsz commented Mar 20, 2018 •

edited

Loading

dternyak commented Mar 21, 2018 •

edited

Loading

maciejhirsz commented Mar 22, 2018

dternyak commented Mar 22, 2018

maciejhirsz commented Mar 26, 2018 •

edited

Loading

maciejhirsz commented Mar 26, 2018 •

edited

Loading

wbobeirne left a comment

wbobeirne Mar 26, 2018

wbobeirne Mar 26, 2018

wbobeirne Mar 26, 2018

wbobeirne Mar 26, 2018

wbobeirne Mar 26, 2018

maciejhirsz Mar 28, 2018

wbobeirne Mar 26, 2018

maciejhirsz Mar 26, 2018

wbobeirne Mar 26, 2018

wbobeirne Mar 26, 2018

maciejhirsz Mar 26, 2018

wbobeirne Mar 26, 2018

wbobeirne Mar 26, 2018

maciejhirsz Mar 26, 2018 •

edited

Loading

wbobeirne Mar 26, 2018

wbobeirne Mar 26, 2018

dternyak Mar 26, 2018

coveralls commented Apr 6, 2018 •

edited

Loading

wbobeirne left a comment

wbobeirne Apr 6, 2018

wbobeirne Apr 6, 2018

wbobeirne Apr 6, 2018

wbobeirne Apr 6, 2018

maciejhirsz Apr 6, 2018


		export default class ParityWallet implements IFullWallet {
		export default class ParitySignerWallet implements IFullWallet {

Parity Signer #1349

Parity Signer #1349

Conversation

maciejhirsz commented Mar 20, 2018 • edited Loading

Description

Changes

Steps to Test

TODO / Known issues

Screenshots

dternyak commented Mar 21, 2018 • edited Loading

maciejhirsz commented Mar 22, 2018

dternyak commented Mar 22, 2018

maciejhirsz commented Mar 26, 2018 • edited Loading

maciejhirsz commented Mar 26, 2018 • edited Loading

wbobeirne 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

maciejhirsz Mar 26, 2018 • edited Loading

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

coveralls commented Apr 6, 2018 • edited Loading

wbobeirne 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

maciejhirsz commented Mar 20, 2018 •

edited

Loading

dternyak commented Mar 21, 2018 •

edited

Loading

maciejhirsz commented Mar 26, 2018 •

edited

Loading

maciejhirsz commented Mar 26, 2018 •

edited

Loading

maciejhirsz Mar 26, 2018 •

edited

Loading

coveralls commented Apr 6, 2018 •

edited

Loading