sms verification modal #3336

derhuerst · 2016-11-10T10:59:15Z

This PR adds the ability for users to prove for a single account that they control an associated phone number. An account can only be verified once, just like a phone number can be used once.

Check out the diagram on how it works, over at the sms-verification repo. The server code is deployed at sms-verification.parity.io.

~~Note that this PR should not get merged before TJ provides a legal text, as we process potentially sensitive user data. Also, I need to do a few minor cleanups.~~ The legal text is in place.

Edit: In 4931381, this PR also adds a maxConfirmations property to TxHash.

derhuerst · 2016-11-10T11:07:43Z

I'm not happy that, right now, a significant part of the logic happens in the React components of the modal dialog. If i close it, reload the page, or something fails/crashes, the process will get aborted. This may, in the worst case, lead to the user not being able to request an SMS.

This introduces another "Done" step, making clear the process has finished. Also, steps 2 ("Request") and 4 ("Confirm") now have a pending indicator.

jacogr · 2016-11-16T22:14:43Z

js/src/modals/SMSVerification/SMSVerification.js

+      setNumber, setConsentGiven, setCode
+    } = this.props.store;
+
+    if (phase === 4) {


Any reason why we are not using a switch or else if here? (As it stands, single blocks should probably be on seperate lines, unless it follows the chain with an else if)

jacogr · 2016-11-16T22:15:29Z

js/src/modals/SMSVerification/SendConfirmation/sendConfirmation.js

+import styles from './sendConfirmation.css';
+
+// TODO: move this to a better place
+const nullable = (type) => PropTypes.oneOfType([ PropTypes.oneOf([ null ]), type ]);


Should probably put this in util - that way everybody can benefit. (And will clean up the same TODO comments everywhere)

jacogr · 2016-11-16T22:18:46Z

js/src/modals/SMSVerification/SendRequest/sendRequest.js

+import styles from './sendRequest.css';
+
+// TODO: move this to a better place
+const nullable = (type) => PropTypes.oneOfType([ PropTypes.oneOf([ null ]), type ]);


As per above util comment.

jacogr · 2016-11-16T22:19:35Z

js/src/modals/SMSVerification/SendRequest/sendRequest.js

+    const { step, tx } = this.props;
+
+    if (step === POSTING_REQUEST) {
+      return (<p>A verification request will be sent to the contract. Please authorize this using the Parity Signer.</p>);


()'s here would make sense if it was split into new lines for readability (see the bulk of the codebase), on a single line they are not needed.

jacogr · 2016-11-16T22:19:47Z

js/src/modals/SMSVerification/SendRequest/sendRequest.js

+      return (<p>A verification request will be sent to the contract. Please authorize this using the Parity Signer.</p>);
+    }
+
+    if (step === POSTED_REQUEST) {


jacogr · 2016-11-16T22:20:49Z

js/src/modals/SMSVerification/check-if-requested.js

+// You should have received a copy of the GNU General Public License
+// along with Parity.  If not, see <http://www.gnu.org/licenses/>.
+
+const checkIfRequested = (contract, account) => {


This would probably make more sense in js/contracts like all the other contract interfaces are wrapped.

jacogr · 2016-11-16T22:21:29Z

js/src/modals/SMSVerification/check-if-tx-failed.js

+// You should have received a copy of the GNU General Public License
+// along with Parity.  If not, see <http://www.gnu.org/licenses/>.
+
+const checkIfTxFailed = (api, tx, gasSent) => {


As per above - instead of single-file-functions, bundle them up and move to contracts so it is accessible.

jacogr · 2016-11-16T22:22:09Z

js/src/modals/SMSVerification/check-if-verified.js

+// You should have received a copy of the GNU General Public License
+// along with Parity.  If not, see <http://www.gnu.org/licenses/>.
+
+const checkIfVerified = (contract, account) => {


As per above - single file functions. Rather follow the structure used/implemented elsewhere for the sake of consistency and discoverability.

jacogr · 2016-11-16T22:22:58Z

js/src/modals/SMSVerification/store.js

+import checkIfRequested from './check-if-requested';
+import checkIfTxFailed from './check-if-tx-failed';
+import waitForConfirmations from './wait-for-confirmations';
+import postToVerificationServer from './post-to-verification-server';


Single file functions + contract discovery inside js/contracts as prompted.

jacogr · 2016-11-16T22:23:51Z

js/src/modals/SMSVerification/store.js

+      return false;
+    }
+
+    if (this.step === GATHERED_DATA) {


Any reason not using a switch for consistency? Also split block or use else-if for follow-on blocks - readability.

jacogr · 2016-11-16T22:24:07Z

js/src/modals/SMSVerification/store.js

+  @action setNumber = (number) => {
+    this.number = number;
+  }
+  @action setConsentGiven = (consentGiven) => {


blank lines between functions.

jacogr · 2016-11-16T22:24:24Z

js/src/modals/SMSVerification/store.js

+    this.code = code;
+  }
+
+  constructor (api, account) {


constructor first, then @actions, etc.

What about @observables and @computeds? The reason I put it below is that they read kind of like an interface description.

Following from what we've done in the React classes, I always put the variables first (@observable), then the initialisation (constructor) then the actual @computed values (functions) then the actual @action (interact) & then the internals.

Valid question, suggestions welcome.

@ngotchac Your views?

jacogr · 2016-11-16T22:25:17Z

js/src/modals/SMSVerification/store.js

+      });
+
+    Promise.all([ fee, isVerified, hasRequested ])
+    .then(() => {


indent. Consider .all on same level. (As it stands there is stylign inconsistency with the Promise chain just above this one.)

jacogr · 2016-11-16T22:25:41Z

js/src/modals/SMSVerification/store.js

+        .then((txHash) => {
+          this.requestTx = txHash;
+          return checkIfTxFailed(api, txHash, options.gas)
+          .then((hasFailed) => {


indent .then (as per the promise it is actually in)

jacogr · 2016-11-16T22:25:58Z

js/src/modals/SMSVerification/store.js

+            if (hasFailed) {
+              throw new Error('Transaction failed, all gas used up.');
+            }
+            this.step = POSTED_REQUEST;


block around if() {}

jacogr · 2016-11-16T22:26:17Z

js/src/modals/SMSVerification/store.js

+      .then((txHash) => {
+        this.confirmationTx = txHash;
+        return checkIfTxFailed(api, txHash, options.gas)
+        .then((hasFailed) => {


jacogr · 2016-11-16T22:27:22Z

js/src/ui/TxHash/txHash.js

    }

    return (
      <div className={ styles.confirm }>
        <LinearProgress
          className={ styles.progressbar }
-          min={ 0 } max={ 10 } value={ value }
+          min={ 0 } max={ maxConfirmations } value={ value }


attribute per line (inconsistency here, I know not introduced here, but since we are fixing it, readability counts.)

- checkIfTxFailed & waitForBlockConfirmations are both general-purpose - checkIfVerified, checkIfRequested & postToServer are sms verification-specific

GitCop · 2016-11-17T13:31:55Z

There were the following issues with your Pull Request

Commit: afdda48
- Your commit message body contains a line that is longer than 72 characters

Guidelines are available at https://github.com/ethcore/parity

This message was auto-generated by https://gitcop.com

derhuerst added 22 commits November 9, 2016 16:54

put SMS verification into action bar

7cb724b

SMS verification modal

f96e693

sms verification: basic stepper

fc76fa9

query phone number & consent

1a24540

SendRequest -> GatherData

435d3c2

sms verification: check if certified

14da2d2

sms verification: collect data

4c199ef

boilerplate for third step

89a2917

sms verification: post request transaction

d0d737d

sms verification: wait for tx confirmations

3ef54f9

txHash component: add maxConfirmations prop

4931381

sms verification: request SMS

a56a1d3

sms verification: update contract address & ABI

ebf742f

sms verification: refactor contract call

e84531f

sms verification: show fee in consent form

e7113e7

sms verification: polish UI, fix linting issues 👕

7996ac4

sms verification: query code

f08e2a4

sms verification: send confirmation tx

a42140e

sms verification: merge step 1 & 2

01cf88d

sms verification: check if already requested

8028c0f

sms verification: polish UI

cde9f45

merge master into sms-verification-modal

89c1d9c

derhuerst added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M6-ui labels Nov 10, 2016

derhuerst added 5 commits November 10, 2016 13:37

sms verification: refactor checks & confirmation

ac80276

sms verification: polish UI, linting issues 👕

2309e53

sms verification: polish UI

afdda48

This introduces another "Done" step, making clear the process has finished. Also, steps 2 ("Request") and 4 ("Confirm") now have a pending indicator.

sms verification: polish UI, linting issues 👕

bd839d0

merge master into sms-verification-modal

01f1490

jacogr reviewed Nov 16, 2016

View reviewed changes

jacogr added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 16, 2016

derhuerst added 6 commits November 17, 2016 12:31

sms verification: fix styling ✨

1344893

merge master into sms-verification-modal

a595260

sms verification: move helper functions

3f0053f

- checkIfTxFailed & waitForBlockConfirmations are both general-purpose - checkIfVerified, checkIfRequested & postToServer are sms verification-specific

sms verification: fetch contract address from Registry

fff8743

sms verification: loading step

179078d

move nullable PropType helper

4ae58f7

jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 17, 2016

gavofyork merged commit 25a7ff1 into master Nov 17, 2016

gavofyork deleted the sms-verification-modal branch November 17, 2016 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sms verification modal #3336

sms verification modal #3336

derhuerst commented Nov 10, 2016 •

edited

Loading

derhuerst commented Nov 10, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

derhuerst Nov 17, 2016

jacogr Nov 17, 2016

jacogr Nov 17, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

jacogr Nov 16, 2016

GitCop commented Nov 17, 2016

sms verification modal #3336

sms verification modal #3336

Conversation

derhuerst commented Nov 10, 2016 • edited Loading

derhuerst commented Nov 10, 2016

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

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

GitCop commented Nov 17, 2016

derhuerst commented Nov 10, 2016 •

edited

Loading