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

Unify passphrase input in the whole app #482

Merged
merged 1 commit into from Dec 28, 2017

Conversation

Projects
None yet
2 participants
@Nasicus
Contributor

Nasicus commented Dec 23, 2017

While doing "a little" testing for my other PR I saw that the input (password) field of the passphrases is different depending on where you are:

  • sometimes you can toggle "view password" (eye icon)
  • sometimes there is a qr-scanner symbol

Also the same code was duplicated throughout the solution.

I extracted the logic into an own component and used this in the whole app.
Now everytime the user has to input the first or the second passphrase.
The field with the visibility togglet button and the qrcode scanner is displayed.

While doing that I one question came up:

  • Somehow the md-required class was not set automatically anymore on the label (because it's an own component now). So I did this manually. I don't understand why it is not set anymore and I wonder if I'm doing something wrong...

While doing the refactor I also detected 2 anonmalies which may or may not be bugs:

  • In the timestampDocument.html class the secondpasphrase field is only shown if there is no ledger (!send.data.ledger && send.data.secondSignature). However in ALL other places, the ledger check is only added to the normal passphrase field.
  • Signing messages only requires the first passphrase (it's the only "action" in the app) - is this correct? (it works though I've tested it)

I tested ALL actions where I replaced this field on the DARKNET.

Note:
Also hadd to do some changes in the qr-scanner.directive.js. Since I also have changes there on another PR (#456) I adjusted the file so that it's identical on both PR's and should hopefully not give any conflicts :)

@luciorubeens

This comment has been minimized.

Show comment
Hide comment
@luciorubeens

luciorubeens Dec 27, 2017

Member

Great PR!

!send.data.ledger and send.data.secondSignature means the same thing, since the ledger wallets can't register a second signature. Just keep the standard.

Yeah, we only need the original passphrase to sign messages. The second passphrase is used to sign transactions, it's useful to the owner of the wallet because it is an extra layer of security.

Member

luciorubeens commented Dec 27, 2017

Great PR!

!send.data.ledger and send.data.secondSignature means the same thing, since the ledger wallets can't register a second signature. Just keep the standard.

Yeah, we only need the original passphrase to sign messages. The second passphrase is used to sign transactions, it's useful to the owner of the wallet because it is an extra layer of security.

@luciorubeens luciorubeens merged commit 9ed84d5 into ArkEcosystem:master Dec 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@luciorubeens

This comment has been minimized.

Show comment
Hide comment
@luciorubeens

luciorubeens Dec 28, 2017

Member

+5 👍

Member

luciorubeens commented Dec 28, 2017

+5 👍

@Nasicus Nasicus deleted the Nasicus:feat/passphrase-field branch Dec 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment