more permissive verification process #4317

derhuerst · 2017-01-26T17:16:14Z

Hopefully addresses #4280. 😛

Let's assume you requested email verification using foo@bar and then aborted the process. The request has been sent to the contract, but the verification server hasn't issued any email.

Right now, the verification process assumes that in this case you will want to continue verifying foo@bar. Trying to verify foo@baz will fail, because the UI happily accepts it, but doesn't send a new request.

The PR makes the specific stores able to determine wether a new request shall be sent, using a method called shallRequestAgain . For email verification, it compares the emailHash of the past request and the values now entered in the UI.

For sms verification, we have no such information in the request tx, so it will always send another request. This may cost the user more money, but given that it fails otherwise, it seems like a reasonable tradeoff.

All of this goes along with commits at ethcore/sms-verification and at ethcore/email-verification to make sure the server really picks the latest Requested event to verify the emailHash.

jacogr · 2017-02-02T10:19:07Z

js/src/contracts/verification.js

  let subId = null;
  let resolved = false;

  return new Promise((resolve, reject) => {
    contract
      .subscribe('Requested', {
-        fromBlock: 0, toBlock: 'pending'
+        fromBlock: 0, toBlock: 'pending', limit: 1,


Always prefer single property per line.

jacogr · 2017-02-02T10:19:45Z

js/src/modals/Verification/GatherData/gatherData.js

              defaultMessage='Your account is already verified.'
            />
          </p>
        </div>
      );
-    } else if (isVerified === false) {
+    } else if (accountIsVerified === false) {


else should be sufficient - prop set as bool.

it's a nullable bool. i know this is ugly, but spamming the props didn't feel right either. not happy with this 😕

jacogr · 2017-02-02T10:20:21Z

js/src/modals/Verification/GatherData/gatherData.js

            />
          </p>
        </div>
      );
-    } else if (hasRequested === false) {
+    } else if (accountHasRequested === false) {


else should be sufficient.

it's a nullable bool. i know this is ugly, but spamming the props didn't feel right either. not happy with this 😕

jacogr · 2017-02-02T10:20:52Z

js/src/modals/Verification/email-store.js


 import EmailVerificationABI from '~/contracts/abi/email-verification.json';
 import VerificationStore, {
  LOADING, QUERY_DATA, QUERY_CODE, POSTED_CONFIRMATION, DONE
 } from './store';
 import { isServerRunning, hasReceivedCode, postToServer } from '~/3rdparty/email-verification';
+const ZERO20 = '0x0000000000000000000000000000000000000000';


empty line before const.

jacogr · 2017-02-02T10:22:14Z

js/src/modals/Verification/email-store.js

+        if (address === ZERO20) {
+          this.isAbleToRequest = true;
+        } else {
+          this.isAbleToRequest = new Error('Another account has been verified using this e-mail.');


Would have preferred another way instead of a bool + Error type mixed. (e.g. set to bool in previous statement, as Error here)

jacogr · 2017-02-02T10:23:06Z

js/src/modals/Verification/email-store.js

+          this.isAbleToRequest = new Error('Another account has been verified using this e-mail.');
+        }
+      })
+      .catch((err) => {


Why not error? We are not gaining anything by not typing 2 letters.

It's a common pattern in the Node.js ecosystem, from the Node.js docs to MDN to lots of well-known modules. I'm fine with moving to error if you mind.

jacogr · 2017-02-02T10:24:00Z

js/src/modals/Verification/sms-store.js

@@ -67,6 +67,18 @@ export default class SMSVerificationStore extends VerificationStore {
    return hasReceivedCode(this.number, this.account, this.isTestnet);
  }

+  // SMS verification events don't contain the phone number, so we will have to
+  // send a new request every single time. See below.
+  @action checkIfAbleToRequest = () => {


checkIf? Doesn't sound like something is being set? (Maybe switch or set?)

Hm, don't like switch, setIfAbleToRequest is ok. isAbleToRequest sounds like it's fully pure, so check seemed like a compromise. will change.

assert might also work.

jacogr · 2017-02-02T10:24:59Z

js/src/modals/Verification/store.js

-        this.isVerified = isVerified;
+    const accountIsVerified = checkIfVerified(contract, account)
+      .then((accountIsVerified) => {
+        this.accountIsVerified = accountIsVerified;


Just be aware that MobX will complain here (since it is async) as soon as switched to strict mode.

Will mobx recognise load as an async action if we return the Promise.all below?

jacogr · 2017-02-02T10:25:39Z

js/src/modals/Verification/store.js

+            return request.postTransaction(options, values);
+          })
+          .then((handle) => {
+            // TODO: The "request rejected" error doesn't have any property to


Not sure why this is a TODO? Possibly rather a note?

We still need to find a better solution, although we're technically limited right now.

jacogr · 2017-02-02T10:26:12Z

js/src/modals/Verification/verification.js

@@ -231,20 +231,22 @@ class Verification extends Component {
        } else if (method === 'email') {
          fields.push({
            key: 'email',
-            label: 'email address',
+            label: 'e-mail address',


Would obviously have preferred FormattedMessage here - it is coming more and more to the fore.

jacogr · 2017-02-02T10:26:50Z

Style & coding issues that detracts a bit from maintainability and ease of understanding, however nothing functional broken nor buggy.

derhuerst added 5 commits January 26, 2017 18:04

style fixes ✨

cf31028

verification: find last request

51c27b9

verification: don't request again if same input

d0575cc

didRequestWithSameValues -> shallRequestAgain

3860951

bugfixes 🐛, update SMS verification ABI

adf1724

derhuerst added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Jan 26, 2017

derhuerst requested a review from gavofyork January 26, 2017 17:16

derhuerst added 6 commits January 30, 2017 14:04

merge master into jr-permissive-verification

db2232a

verification: hasRequested -> accountHasRequested

8efc4b4

verification: hasIsVerified -> accountIsVerified

c69f898

verification: shallRequestAgain -> shallSkipRequest

41758b6

verification: show if unable to send req

2016d20

email verification: check if email already used

922c52f

derhuerst added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 1, 2017

derhuerst removed the request for review from gavofyork February 1, 2017 18:08

jacogr reviewed Feb 2, 2017

View reviewed changes

jacogr added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 2, 2017

address style grumbles 🎨

aee33c2

gavofyork merged commit 1547af1 into master Feb 2, 2017

gavofyork deleted the jr-permissive-verification branch February 2, 2017 15:01

derhuerst mentioned this pull request Feb 2, 2017

Verify email fail #4280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more permissive verification process #4317

more permissive verification process #4317

derhuerst commented Jan 26, 2017

jacogr Feb 2, 2017

jacogr Feb 2, 2017

derhuerst Feb 2, 2017

jacogr Feb 2, 2017

derhuerst Feb 2, 2017

jacogr Feb 2, 2017

jacogr Feb 2, 2017

jacogr Feb 2, 2017

derhuerst Feb 2, 2017

jacogr Feb 2, 2017

derhuerst Feb 2, 2017

derhuerst Feb 2, 2017

jacogr Feb 2, 2017

derhuerst Feb 2, 2017

jacogr Feb 2, 2017

derhuerst Feb 2, 2017

jacogr Feb 2, 2017

jacogr commented Feb 2, 2017 •

edited

more permissive verification process #4317

more permissive verification process #4317

Conversation

derhuerst commented Jan 26, 2017

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

jacogr commented Feb 2, 2017 • edited

jacogr commented Feb 2, 2017 •

edited