Add advanced setting to enable editing nonce on confirmation screens by rickycodes · Pull Request #7089 · MetaMask/metamask-extension

rickycodes · 2019-08-31T16:04:10Z

This gives us a new nonce toggle in settings:

Which enables adding one to the transaction:

rickycodes

This is almost done, just a couple things need to be tied together

rickycodes · 2019-09-02T21:31:13Z

ui/app/pages/confirm-transaction-base/confirm-transaction-base.container.js

I tried doing: nonce: customNonceValue to see if i could get the nonce into the transaction that way, but it didn't appear to work as expected...

This is due to how the nonce is set when a transaction is approved. See line 379 of the transactions controller: https://github.com/MetaMask/metamask-extension/pull/7089/files/5e653e49f9376c12a33b77ce5813bb30468c2c33#diff-97c997fde8231c768b7f2393fd3012e1L379

It looks like you were on your way to updating that code based on your comment in that file

rickycodes · 2019-09-02T21:32:21Z

app/scripts/controllers/transactions/index.js

I'd assume this property exists at this point, but I don't see this log anywhere? not sure what makes the most sense to debug this?

Were you looking in the background console for the log? To see it you need to:

go to chrome://extensions/ or the Brave equivalent

turn "Developer Mode" on

under the MetaMask app you are building, inspect the "Background page"

click the "console" tab

Okay, I got things sorta working... was able to kick off a transaction with a custom nonce of 4:

however it's just been pending this whole time... I'm not sure what the requirements of nonce are... trying strings like 'testing123' (as an example) just seems to break things (transaction doesn't go through and I get a bunch of errors).

In fact, I think what you have is working. That the tx is in pending the whole time is due to how transactions are handled by the ethereum blockchain. If the network only has record of nonces 1 and 2 for your account, and you custom edit a new transaction to nonce 5, that new one will remain pending until transactions with nonces 3 and 4 are successfully confirmed.

Read this for a helpful explanation: https://www.reddit.com/r/ethereum/comments/6ihw6p/can_someone_please_explain_nonce_to_me/dj6r1lk/

You are right, however, that a nonce cannot just be any arbitrary string. It must be a natural number >= 0. So we should prevent other values from being entered in this input field.

Ensure inputs can only be submitted if nonce is a whole number >= 0, or ensure those are the only input possible in the nonce field

danjm · 2019-09-05T05:11:42Z

We should move the toggle to the "Advanced" settings page.

move the toggle to the "Advanced" settings page.

rickycodes · 2019-09-06T06:07:44Z

I've moved the nonce toggle to the Advanced settings page, I've added some additional styles and things, but I am sure there is still more to do around where to reset the customNonceValue... I've got it resetting when you toggle and when the transaction goes through, but it might need to be sprinkled out a bit more?

rickycodes · 2019-09-06T06:11:00Z

Oh also there might need to be more of a sanity check on customNonceValue when it's getting set, i don't know if parseInt would suffice in this case? 0 is valid, but 001 is invalid (if I understand things correctly)... maybe there's already a thing in the wild for validating nonce? anyway, i don't think setting the input to type=number really satisfies the requirements?

fabtho · 2019-09-11T20:48:32Z

Thanks for this! I'am looking forward to use it. I got a stuck tx since more then 4 days, metamask not showing speed up or cancel on this one. (Browser freeze, so tx somewhat lost)

danjm · 2019-09-19T12:22:18Z

Updated demo: https://streamable.com/noij3

And screen shot:

cjeria · 2019-09-19T23:58:05Z

The nonce input field is very wide. Probably could reduce the width by 1/2 it's current size. I noticed the placeholder text "automatically calculate", however, I'd suggest populating the custom nonce field with the nonce calculated by MM as opposed to an empty field w/placeholder.

I'd update the text in settings to something like this instead.

cc @danjm

danjm · 2019-09-21T16:31:23Z

@cjeria Made changes according to your suggestions:

app/_locales/en/messages.json

ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js

ui/app/store/actions.js

app/scripts/controllers/transactions/index.js

cjeria · 2019-09-24T20:43:53Z

There's a lot of emphasis on the component "nonce field" in the title. Titles should help answer user questions like what the feature will enable them to do. In this case, the "nonce field" lets users "customize transaction nonces". More broadly this feature will let users "Edit transaction nonces on confirm screens".

Also, remove the period at the end of the title. WDYT?

…roveTransaction

frankiebee · 2019-09-26T07:17:27Z

app/scripts/controllers/transactions/index.js

add this value to nonce details

frankiebee · 2019-09-26T07:30:35Z

app/scripts/controllers/transactions/index.js

revert this change

frankiebee · 2019-09-26T07:31:16Z

app/scripts/controllers/transactions/index.js

id like it if we still got are calculated nonce for debug purposes

app/scripts/controllers/transactions/index.js

frankiebee · 2019-09-26T08:25:18Z

QUESTION: should we address the showing of a warning if the custom nonce is distance one or greater from our suggested nonce

feel free to ignore this question this is mainly a reminder that the question was brought up

…ansaction.

…once

Gudahtt · 2019-09-26T19:07:33Z

ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js

+                        this.setState({ submitWarning: '' })
+                      }
+                      updateCustomNonce(String(newCustomNonce))
+                      getNextNonce()


If this results in the nextNonce changing to one that would invalidate the custom nonce, it won't result in a warning. I think this case could be caught by re-validating the custom nonce field in the lifecycle function when the nextNonce prop changes

frankiebee

Thanks looks great

* origin/develop: (56 commits) Add advanced setting to enable editing nonce on confirmation screens (MetaMask#7089) Add migration on 3box imports and remove feature flag (MetaMask#7209) ci - install deps - limit install scripts to whitelist (MetaMask#7208) Add a/b test for full screen transaction confirmations (MetaMask#7162) Update minimum Firefox verison to 56.0 (MetaMask#7213) mesh-testing - submit infura rpc requests to mesh-testing container (MetaMask#7031) obs-store/local-store should upgrade webextension error to real error (MetaMask#7207) sesify-viz - bump dep for visualization enhancement (MetaMask#7175) address book entries by chainId (MetaMask#7205) Optimize images only during production build (MetaMask#7194) Use common test build during CI (MetaMask#7196) Report missing `en` locale messages to Sentry (MetaMask#7197) Verify locales on CI (MetaMask#7199) updated ganache and addons-linter (MetaMask#7204) fixup! add user rejected errors add user rejected errors update json-rpc-engine use eth-json-rpc-errors Remove unused locale messages (MetaMask#7190) Remove unused components (MetaMask#7191) ...

tbenr · 2020-06-17T07:48:43Z

@danjm this is not implemented for ERC20 "approve" transactions, am I right?

rickycodes requested review from Gudahtt, danjm and whymarrh as code owners August 31, 2019 16:04

rickycodes requested a review from frankiebee as a code owner September 2, 2019 20:44

rickycodes changed the title ~~WIP: Add UseNonce toggle~~ Add advanced setting to enable editing nonce on confirmation screens Sep 2, 2019

rickycodes commented Sep 2, 2019

View reviewed changes

fabtho mentioned this pull request Sep 11, 2019

add (editable) nonce to confirm screen #1410

Closed

danjm added this to the UI Sprint 20 [Sept 16] milestone Sep 16, 2019

danjm self-assigned this Sep 16, 2019

Gudahtt requested changes Sep 24, 2019

View reviewed changes

app/_locales/en/messages.json Outdated Show resolved Hide resolved

Gudahtt reviewed Sep 24, 2019

View reviewed changes

ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js Outdated Show resolved Hide resolved

Gudahtt reviewed Sep 24, 2019

View reviewed changes

ui/app/store/actions.js Outdated Show resolved Hide resolved

Gudahtt reviewed Sep 24, 2019

View reviewed changes

app/scripts/controllers/transactions/index.js Outdated Show resolved Hide resolved

rickycodes added 9 commits September 26, 2019 03:26

Add UseNonce toggle

ba1d1b0

Get the toggle actually working and dispatching

d4b17cb

Display nonce field on confirmation page

02d74d9

Remove console.log

a148345

Add placeholder

568669a

Set customNonceValue

477c792

Add nonce key/value to txParams

e5d5e6f

remove customNonceValue from component state

e2d93fa

Use translation file and existing CSS class

386815d

danjm added 6 commits September 26, 2019 03:30

Cast custom nonce to string in confirm-transaction-base.component

8f74cd5

Handle string conversion and invalid values for custom nonces in handler

1c0ef4c

Don't call getNonceLock in tx controller if there is a custom nonce

745c815

Set nonce details for cases where nonce is customized

2ddca98

Fix incorrectly use value for deciding whether to getnoncelock in app…

d35fa41

…roveTransaction

Default nonceLock to empty object in approveTransaction

d485615

frankiebee suggested changes Sep 26, 2019

View reviewed changes

danjm added 3 commits September 26, 2019 11:51

Reapply use on nonceLock in cases where customNonceValue in approveTr…

7bdcda3

…ansaction.

Show warning message if custom nonce is higher than MetaMask's next n…

e4977ce

…once

Fix e2e test failure caused by custom nonce and 3box toggle conflict

7062cad

Gudahtt reviewed Sep 26, 2019

View reviewed changes

danjm added 9 commits September 26, 2019 15:29

Update nonce warning message to include the suggested nonce

a050526

Handle nextNonce comparison and update logic in lifecycle

9aced78

Default nonce field to suggested nonce

be05b3a

Clear custom nonce on reject or confirm

01955c4

Fix bug where nonces are not shown in tx list on self sent transactions

96ead04

Ensure custom nonce is reset after tx is created in background

2fea607

Convert customNonceValue to number in approve tranasction controller

67408f9

Lint fix

57d5951

Call getNextNonce after updating custom nonce

ee70842

Gudahtt approved these changes Sep 27, 2019

View reviewed changes

frankiebee approved these changes Sep 27, 2019

View reviewed changes

danjm merged commit 5f254f7 into MetaMask:develop Sep 27, 2019

bbondy mentioned this pull request Dec 16, 2019

Rebase Crypto Wallets to MetaMask upstream brave/brave-browser#7408

Closed

rickycodes deleted the add-advanced-setting-to-enable-editing-nonce branch February 11, 2020 23:56

Conversation

rickycodes commented Aug 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickycodes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danjm commented Sep 5, 2019

Uh oh!

rickycodes commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickycodes commented Sep 6, 2019

Uh oh!

fabtho commented Sep 11, 2019

Uh oh!

danjm commented Sep 19, 2019

Uh oh!

cjeria commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danjm commented Sep 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjeria commented Sep 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frankiebee commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankiebee left a comment

Choose a reason for hiding this comment

Uh oh!

tbenr commented Jun 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rickycodes commented Aug 31, 2019 •

edited

Loading

rickycodes commented Sep 6, 2019 •

edited

Loading

cjeria commented Sep 19, 2019 •

edited

Loading

frankiebee commented Sep 26, 2019 •

edited

Loading