Create custom addHexPrefix function #9306

PatrykLucka · 2020-08-25T11:41:03Z

Creates custom addHexPrefix function. The one provided by ethereumjs-util does not handle incorrect eth address hex formats such us starting with - or 0X.

Fixes #9262

app/scripts/lib/util.js

ui/app/hooks/useCancelTransaction.js

github-actions · 2020-09-07T13:58:05Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Gudahtt

Does it make sens to allow negative numbers anywhere we're using addHexPrefix? Addresses can't be negative, and neither can the gas price, gas limit, private key, balance, nonce, etc. Or at least not that I know of... maybe there's some type of network out there that supports negative gas price/limit or balance 🤔

If there are no valid cases for having a negative number, maybe this is better addressed with additional validation.

Gudahtt · 2020-09-08T16:16:08Z

ui/app/hooks/useCancelTransaction.js

@@ -17,7 +17,7 @@ import { getConversionRate, getSelectedAccount } from '../selectors'
 */
 export function useCancelTransaction (transactionGroup) {
  const { primaryTransaction, initialTransaction } = transactionGroup
-  const gasPrice = primaryTransaction.txParams?.gasPrice
+  const gasPrice = parseInt(primaryTransaction.txParams?.gasPrice, 16) < 0 ? '0x0' : primaryTransaction.txParams?.gasPrice


Is this necessary for cancellation transactions? These should only be possible by using our UI, and I didn't think we allowed a negative gas price there 🤔

I would have to check that, but I'm pretty sure I added this change to allow users to "repair" their extension after getting error. I had to reinstall MM to make it work again, even with other updates, but with this change, when opening MM gas price of pending transaction was changed to 0 (instead of negative number) and it was fixing the problem. Does it make sense?

PatrykLucka · 2020-09-08T18:09:08Z

It was reported in #9262. Even if in theory there shouldn't be such situation, there is an option in our UI to set gas price < 0. Additional validation sounds good, but there could be some other ways to make one of the processed numbers negative and as result crashing whole app. The only way to make it work again is to reinstall extension.

Gudahtt · 2020-09-08T18:13:58Z

The reproduction example in #9262 shows the negative gas price being set by a dapp, not via our UI. We already reject incoming messages outright in some cases where they fail validation, so this seems like a case where this validation should be expanded

app/scripts/lib/util.js

Co-authored-by: Mark Stacey <[email protected]>

app/scripts/lib/util.js

ui/app/hooks/useCancelTransaction.js

rekmarks · 2020-11-05T21:16:05Z

ui/app/hooks/useCancelTransaction.js

+  const gasPrice =
+    primaryTransaction.txParams?.gasPrice?.startsWith('-')
+      ? '0x0'
+      : primaryTransaction.txParams?.gasPrice


I changed this. Compare: https://github.com/MetaMask/metamask-extension/compare/2ba32e896f545ed432488d3d04a91ee42eacd9c3...89a6720?expand=1

parseInt is not safe to use with Ethereum quantities.

This is only labeled as outdated due to linting.

rekmarks · 2020-11-05T21:16:21Z

app/scripts/lib/util.js

+/**
+ * Prefixes a hex string with '0x' or '-0x' and returns it. Idempotent.
+ *
+ * @param {string} str - The string to prefix.
+ * @returns {string} The prefixed string.
+ */
+const addHexPrefix = (str) => {
+  if (typeof str !== 'string' || str.match(/^-?0x/u)) {
+    return str
+  }
+
+  if (str.match(/^-?0X/u)) {
+    return str.replace('0X', '0x')
+  }
+
+  if (str.startsWith('-')) {
+    return str.replace('-', '-0x')
+  }
+
+  return `0x${str}`
+}


I changed this implementation. Comparison: https://github.com/MetaMask/metamask-extension/compare/2ba32e896f545ed432488d3d04a91ee42eacd9c3...89a6720?expand=1

rekmarks

I think this is good to go, but since I made some edits I'll ask for a second review.

danjm · 2020-11-06T19:46:29Z

app/scripts/lib/util.js

@@ -173,13 +162,47 @@ function isPrefixedFormattedHexString(value) {
  return /^0x[1-9a-f]+[0-9a-f]*$/iu.test(value)
 }

+/**
+ * Prefixes a hex string with '0x' or '-0x' and returns it. Idempotent.


Very nice use of the word "Idempotent" 🙌

danjm

This code looks good. Approving.

But I think the original issue that motivated this also requires us to make a change so that gas price validation happens on gas prices suggested by the dapp. At least on ethereum mainnet, we should make sure no transaction can be submitted with a gas price we know will fail. We do prevent this when the user is manually editing gas, but we don't on the submit button on the confirm screen. This should be handled in another PR.

Gudahtt · 2020-11-06T20:13:58Z

we should make sure no transaction can be submitted with a gas price we know will fail.

I don't think this was the case here. It's not that we knew the price would fail - it's that it was an invalid price, and it crashed our UI. The validation we need here is in our dapp request handling, and it should have been rejected outright without showing the user anything, just as we do with other malformed requests.

I think we could avoid all of this work by some combination of request validation and migrating the state of pending transaction confirmations to remove negative gas prices. I'm not against this being merged in the meantime, but handling invalid types mid-operation is not a great approach in general - we should verify our assumptions about this data at the boundaries rather than within the app. This is only one place where a negative gas price caused things to blow up after all - there may be others that remain unaddressed.

danjm · 2020-11-06T20:19:52Z

Well if we did ensure that 'no transaction can be submitted with a gas price we know will fail', this particular case would have been avoided. This doesn't invalidate anything else @Gudahtt said in the above comment, but this issue also points to the need to check whether gas prices that come from dapps are too low.

danjm · 2020-11-06T20:33:20Z

Actually, my comment that "Well if we did ensure that 'no transaction can be submitted with a gas price we know will fail', this particular case would have been avoided" was wrong. This issue happens on render of confirmation screen, not after submit which I previously thought.

However, I think that "this issue also points to the need to check whether gas prices that come from dapps are too low" holds true. I didn't realize, or had forgotten, that gas price set via rpc methods would be the gas price used in our confirmation screen. I now realize that this comment is tangential to this PR, but we should also add front end validation and error messaging for gas prices that are valid prices, but guaranteed to fail.

PatrykLucka requested a review from a team as a code owner August 25, 2020 11:41

PatrykLucka requested a review from rekmarks August 25, 2020 11:41

Gudahtt reviewed Aug 26, 2020

View reviewed changes

app/scripts/lib/util.js Outdated Show resolved Hide resolved

app/scripts/lib/util.js Outdated Show resolved Hide resolved

ui/app/hooks/useCancelTransaction.js Outdated Show resolved Hide resolved

PatrykLucka requested a review from Gudahtt August 28, 2020 09:30

PatrykLucka force-pushed the create-custom-addHexPrefix branch from 2e716ed to 5732479 Compare September 7, 2020 13:57

Gudahtt reviewed Sep 8, 2020

View reviewed changes

danjm added this to the v8.1.next? milestone Nov 2, 2020

danjm assigned danjm and rekmarks Nov 3, 2020

PatrykLucka force-pushed the create-custom-addHexPrefix branch from 5732479 to 061a71d Compare November 4, 2020 11:38

rekmarks reviewed Nov 5, 2020

View reviewed changes

app/scripts/lib/util.js Show resolved Hide resolved

PatrykLucka and others added 7 commits November 5, 2020 12:30

create custom addHexPrefix function

9a3d164

switch to custom addHexPrefix

b9a01b7

Update app/scripts/lib/util.js

6cc1d3a

Co-authored-by: Mark Stacey <[email protected]>

fix jsDoc and gasPrice comparison

6b66f6a

fix lint error

25fbdc3

Update util.js

98fff60

fix non string case

2ba32e8

PatrykLucka force-pushed the create-custom-addHexPrefix branch from 9de17f7 to 2ba32e8 Compare November 5, 2020 11:40

rekmarks reviewed Nov 5, 2020

View reviewed changes

app/scripts/lib/util.js Outdated Show resolved Hide resolved

Update addHexPrefix

eb720e0

rekmarks reviewed Nov 5, 2020

View reviewed changes

app/scripts/lib/util.js Outdated Show resolved Hide resolved

Update app/scripts/lib/util.js

d8a6d82

rekmarks reviewed Nov 5, 2020

View reviewed changes

app/scripts/lib/util.js Outdated Show resolved Hide resolved

Update app/scripts/lib/util.js

0af5619

rekmarks reviewed Nov 5, 2020

View reviewed changes

app/scripts/lib/util.js Outdated Show resolved Hide resolved

Update app/scripts/lib/util.js

a11a616

rekmarks reviewed Nov 5, 2020

View reviewed changes

app/scripts/lib/util.js Outdated Show resolved Hide resolved

Update app/scripts/lib/util.js

75da02e

rekmarks reviewed Nov 5, 2020

View reviewed changes

ui/app/hooks/useCancelTransaction.js Outdated Show resolved Hide resolved

Update ui/app/hooks/useCancelTransaction.js

89a6720

rekmarks reviewed Nov 5, 2020

View reviewed changes

rekmarks previously approved these changes Nov 5, 2020

View reviewed changes

Lint

336a2d3

rekmarks dismissed their stale review via 336a2d3 November 5, 2020 21:38

rekmarks approved these changes Nov 5, 2020

View reviewed changes

danjm reviewed Nov 6, 2020

View reviewed changes

danjm approved these changes Nov 6, 2020

View reviewed changes

rekmarks merged commit ee205b8 into MetaMask:develop Nov 6, 2020

github-actions bot locked and limited conversation to collaborators Nov 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create custom addHexPrefix function #9306

Create custom addHexPrefix function #9306

PatrykLucka commented Aug 25, 2020

github-actions bot commented Sep 7, 2020

Gudahtt left a comment •

edited

Loading

Gudahtt Sep 8, 2020

PatrykLucka Sep 8, 2020

PatrykLucka commented Sep 8, 2020

Gudahtt commented Sep 8, 2020

rekmarks Nov 5, 2020 •

edited

Loading

rekmarks Nov 6, 2020

rekmarks Nov 5, 2020 •

edited

Loading

rekmarks left a comment

danjm Nov 6, 2020

danjm left a comment

Gudahtt commented Nov 6, 2020

danjm commented Nov 6, 2020

danjm commented Nov 6, 2020

Create custom addHexPrefix function #9306

Create custom addHexPrefix function #9306

Conversation

PatrykLucka commented Aug 25, 2020

github-actions bot commented Sep 7, 2020

Gudahtt left a comment • edited Loading

Choose a reason for hiding this comment

Gudahtt Sep 8, 2020

Choose a reason for hiding this comment

PatrykLucka Sep 8, 2020

Choose a reason for hiding this comment

PatrykLucka commented Sep 8, 2020

Gudahtt commented Sep 8, 2020

rekmarks Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

rekmarks Nov 6, 2020

Choose a reason for hiding this comment

rekmarks Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

rekmarks left a comment

Choose a reason for hiding this comment

danjm Nov 6, 2020

Choose a reason for hiding this comment

danjm left a comment

Choose a reason for hiding this comment

Gudahtt commented Nov 6, 2020

danjm commented Nov 6, 2020

danjm commented Nov 6, 2020

Gudahtt left a comment •

edited

Loading

rekmarks Nov 5, 2020 •

edited

Loading

rekmarks Nov 5, 2020 •

edited

Loading