fix: Fixes #361. Restrict user from clicking "Scan" to process tx before gas is calculated #386

ltfschoen · 2019-01-22T23:04:07Z

Update TxForm.js to add isEstimatingTxFee with simple boolean return value so we can check if all values including gas are available without actually having to calculate if we don't need to.
Update TxForm.js to display and disable the "Checking..." button if the isEstimatingTxFee returns false (i.e. gas still undefined)
Update TxDetails.js adding additional early exits if prop values are undefined. Note that these changes do not appear to have been the cause of the bug, but just doing extra due diligence
Note: The bug associated appears to be Parity Signer-specific. If the user was allowed to click "Scan" before the gas has been calculated (i.e. because we weren't disabling the "Scan" button on the form, and the "Scan" button would become enabled before the gas had finished being calculated, so if the user clicked "Scan" too quickly after it became enabled it would trigger the error), then when it tries to go to the paritytech/fether/packages/fether-react/src/Send/TxQrCode/TxQrCode.js, where it calls getRlp() to get the value for the rlp prop of the QrSigner component, which calls transactionToRlp(this.tx) to get the RLP of the unsigned tx that is provided as an argument paritytech/fether/packages/fether-react/src/stores/sendStore.js, it crashes if this.tx contains a gas property that's undefined.
So we needed to prevent the user from being able to click "Scan" before the gas property has been determined.
We do this by disabling the button and displaying "Checking..." until isEstimatingTxFee returns true, and only then do we display "Scan" and enable the button

…ore gas is calculated * Add `isEstimatingTxFee` so we can check if all values including `gas` are available without actually having to calculate the tx fee and incorporate into estimateTxFee method * Display and disable the "Checking..." button if the `isEstimatingTxFee` returns false (i.e. `gas` still undefined) * Note: The bug associated with #361 appears to be Parity Signer-specific. If the user can click "Scan" before the `gas` has been calculated, then when it tries to go to the paritytech/fether/packages/fether-react/src/Send/TxQrCode/TxQrCode.js, where it calls `getRlp()` to get the value for the rlp prop of the QrSigner component, which calls `transactionToRlp(this.tx)` to get the RLP of the unsigned tx that is provided as an argument paritytech/fether/packages/fether-react/src/stores/sendStore.js, it crashes if `this.tx` contains a `gas` property that's undefined. So we need to prevent the user from being able to click "Scan" before the `gas` property has been determined. We do this by disabling the button and displaying "Checking..." until `isEstimatingTxFee` returns true, and only then do we display "Scan" and enable the button

Tbaut

Small comments.
I had an annoying bug, it's maybe not related (Kovan is quite ill at the moment, the node doesn't stay in sync), still if we could handle such events it's be nice, but that's prob. for a dedicated issue.

packages/fether-react/src/Send/TxForm/TxDetails/TxDetails.js

Tbaut · 2019-01-23T09:37:09Z

packages/fether-react/src/Send/TxForm/TxForm.js

                            className='button'
                          >
-                            {validating
+                            {validating || !this.isEstimatedTxFee(values)


This makes the button appear as "Checking..." when Fether isn't actually checking anything (e.g when no field is filled). This is confusing Ux-wise I think.

Let's keep the validation logic inside validate()

// If the gas hasn't been calculated yet, then we don't show any errors, // just wait a bit more if (!this.isEstimatedTxFee(values)) { return {gasPrice: "Estimating gas..."}; }

does the trick for me

I'm not sure why, but it only displays the validation popup "Estimating gas..." (shown below) if you've changed the slider value of the "Transaction Speed". If you only entered a recipient address and an amount then it doesn't show it.

i.e. try changing the code temporarily to the following so it should shows that validation error as soon as there is an amount and recipient, but after entering a recipient address and an amount it still doesn't appear. It'll only appear after you also change the "Transaction Speed".

// if (!this.isEstimatedTxFee(values)) { return { gasPrice: 'Estimating gas...' }; // }

axelchalon · 2019-01-23T17:45:29Z

packages/fether-react/src/Send/TxForm/TxForm.js

@@ -396,7 +407,7 @@ class TxForm extends Component {

      // If the gas hasn't been calculated yet, then we don't show any errors,
      // just wait a bit more
-      if (!this.estimatedTxFee(values)) {
+      if (!this.isEstimatedTxFee(values)) {


replace with

// If the gas hasn't been calculated yet, then we don't show any errors, // just wait a bit more if (!this.isEstimatedTxFee(values)) { return {gasPrice: "Estimating gas..."}; }

if the gas wasn't estimated yet, then the form isn't valid

the only con is that this will show up as a disabled "Send" button rather than a "Verifying" button (while the gas is being estimated), but since this is for a short amount of time I think it's ok

Yeah I think that's fine as a workaround for now, we can see later if that's a problem on mainnet later.

axelchalon · 2019-01-23T17:57:00Z

#361 (comment)

ltfschoen · 2019-01-23T22:05:37Z

I've pushed changes addressing your review comments. Although I need help trying to figure out #386 (comment)

Tbaut · 2019-01-24T10:14:48Z

Although I need help trying to figure out #386 (comment)

I can't reproduce (with Signer account), can you walk me through the steps to reproduce?

ltfschoen · 2019-01-24T10:40:35Z

@Tbaut

Comment out just the if statement so it runs return { gasPrice: 'Estimating gas...' }; (to simulate a situation when it's estimating gas and should show the validation popup with the message 'Estimating gas...' when both a valid recipient and amount are entered:

      // if (!this.isEstimatedTxFee(values)) {
        return { gasPrice: 'Estimating gas...' };
      // }

Go to the Send Ether page to send a transaction
Enter a valid recipient and a valid amount
Expect to see a popup validation error stating 'Estimating gas...' above the Transaction Speed slider, however it doesn't appear as expected
Change the value of the "Transaction Speed" by sliding it
Observe the popup validation error stating 'Estimating gas...' appear above the Transaction Speed slider

Tbaut · 2019-01-24T16:50:31Z

packages/fether-react/src/Send/TxForm/TxForm.js

-      if (!this.estimatedTxFee(values)) {
-        return;
+      if (!this.isEstimatedTxFee(values)) {
+        return { gasPrice: 'Estimating gas...' };


Got it, this seems to be how the validate props of final-form works, it would only display an error once the fields has actually been modified (makes sense).
A simple workaround would be to show the error on the amount field, (that users must fill). I think this is fine because the gas estimation isn't particularly related to the gas price.

Suggested change

return { gasPrice: 'Estimating gas...' };

return { amount: 'Estimating gas...' };

Agreed, yes only difference is that the popup will appear about the Amount field

@Tbaut I've pushed a commit that adopts the workaround that you mentioned of showing the errors on the amount field instead

amaury1093

Looks good to me overall

amaury1093 · 2019-02-04T11:40:10Z

packages/fether-react/src/Send/TxForm/TxForm.js

+      values.gas &&
+      values.gasPrice &&
+      !isNaN(values.amount) &&
+      !isNaN(values.gas) &&


Just seeing this. values.gas is a BigNumber, so isNaN(values.gas) is always true?

We need values.gas to be defined and !isNaN(values.gas) to be true for it to estimate the gas

When the Account page is loaded:
values.gas is undefined
!isNaN(values.gas) is false
!values.gas.isNaN() is undefined

So it doesn't estimate the gas

After entering just Recipient address or just an Amount:
values.gas is null
!isNaN(values.gas) is true
!values.gas.isNaN() is null

So it doesn't estimate the gas

After entering BOTH a Recipient address and an Amount:
values.gas is BigNumber (i.e. 21000)
!isNaN(values.gas) is true
!values.gas.isNaN() is true

So it DOES estimate the gas

==============

So it's better to use the .isNaN() method provided for BigNumber values

amaury1093 · 2019-02-04T11:41:11Z

packages/fether-react/src/Send/TxForm/TxForm.js

@@ -394,10 +402,14 @@ class TxForm extends Component {
        return preValidation;
      }

+      if (values.gas && values.gas.toString() === '-1') {


I'd say values.gas && values.gas.eq(-1) looks slightly better :)

ltfschoen · 2019-02-04T16:21:58Z

@amaurymartiny I've pushed commits that address your review comments

Tbaut

👍

ltfschoen added 2 commits January 23, 2019 09:53

fix: Early exit from rendering TxDetails if prop values undefined

1065d60

ltfschoen added the A0-pleasereview label Jan 22, 2019

Merge branch 'master' into luke-361-prevent-scan-before-gas-calculated

8d80045

Tbaut reviewed Jan 23, 2019

View reviewed changes

Tbaut added A5-grumble and removed A0-pleasereview labels Jan 23, 2019

axelchalon reviewed Jan 23, 2019

View reviewed changes

ltfschoen added 3 commits January 24, 2019 07:40

fix: Restore linebreak so message is centered

9e88c86

review-fix: Move validation login inside validate function

2e24732

review-fix: Handle when unable to estimate gas

4bd367d

Merge branch 'master' into luke-361-prevent-scan-before-gas-calculated

3bc9a2a

ltfschoen added A0-pleasereview and removed A5-grumble labels Jan 23, 2019

Tbaut added A0-pleasereview A4-gotissues and removed A0-pleasereview labels Jan 24, 2019

Tbaut reviewed Jan 24, 2019

View reviewed changes

review-fix: Show gas alerts above amount field instead of gasPrice field

1085c6c

ltfschoen removed the A4-gotissues label Feb 3, 2019

amaury1093 approved these changes Feb 4, 2019

View reviewed changes

ltfschoen added 2 commits February 5, 2019 02:48

Merge branch 'master' into luke-361-prevent-scan-before-gas-calculated

064116c

review-fix: Use BigNumber methods for BigNumber values

25e87f8

ltfschoen added 2 commits February 9, 2019 02:47

Merge branch 'master' into luke-361-prevent-scan-before-gas-calculated

e19c9fb

Merge branch 'master' into luke-361-prevent-scan-before-gas-calculated

caa8e93

Tbaut approved these changes Feb 11, 2019

View reviewed changes

amaury1093 approved these changes Feb 11, 2019

View reviewed changes

amaury1093 added A8-looksgood and removed A0-pleasereview labels Feb 11, 2019

Merge branch 'master' into luke-361-prevent-scan-before-gas-calculated

3d68c22

amaury1093 merged commit 0dd96a7 into master Feb 11, 2019

amaury1093 deleted the luke-361-prevent-scan-before-gas-calculated branch February 11, 2019 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fixes #361. Restrict user from clicking "Scan" to process tx before gas is calculated #386

fix: Fixes #361. Restrict user from clicking "Scan" to process tx before gas is calculated #386

ltfschoen commented Jan 22, 2019 •

edited

Loading

Tbaut left a comment

Tbaut Jan 23, 2019

axelchalon Jan 23, 2019 •

edited

Loading

ltfschoen Jan 23, 2019 •

edited

Loading

axelchalon Jan 23, 2019 •

edited

Loading

axelchalon Jan 23, 2019

Tbaut Jan 24, 2019

axelchalon commented Jan 23, 2019

ltfschoen commented Jan 23, 2019

Tbaut commented Jan 24, 2019

ltfschoen commented Jan 24, 2019 •

edited by Tbaut

Loading

Tbaut Jan 24, 2019 •

edited

Loading

ltfschoen Jan 26, 2019

ltfschoen Feb 3, 2019

amaury1093 left a comment

amaury1093 Feb 4, 2019

ltfschoen Feb 4, 2019

amaury1093 Feb 4, 2019

ltfschoen Feb 4, 2019

ltfschoen commented Feb 4, 2019

Tbaut left a comment

	return { gasPrice: 'Estimating gas...' };
	return { amount: 'Estimating gas...' };

fix: Fixes #361. Restrict user from clicking "Scan" to process tx before gas is calculated #386

fix: Fixes #361. Restrict user from clicking "Scan" to process tx before gas is calculated #386

Conversation

ltfschoen commented Jan 22, 2019 • edited Loading

Tbaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelchalon Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

ltfschoen Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

axelchalon Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelchalon commented Jan 23, 2019

ltfschoen commented Jan 23, 2019

Tbaut commented Jan 24, 2019

ltfschoen commented Jan 24, 2019 • edited by Tbaut Loading

Tbaut Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 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

ltfschoen commented Feb 4, 2019

Tbaut left a comment

Choose a reason for hiding this comment

ltfschoen commented Jan 22, 2019 •

edited

Loading

axelchalon Jan 23, 2019 •

edited

Loading

ltfschoen Jan 23, 2019 •

edited

Loading

axelchalon Jan 23, 2019 •

edited

Loading

ltfschoen commented Jan 24, 2019 •

edited by Tbaut

Loading

Tbaut Jan 24, 2019 •

edited

Loading