Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/custom nonce #2371

Merged
merged 82 commits into from
Apr 14, 2021
Merged

Feature/custom nonce #2371

merged 82 commits into from
Apr 14, 2021

Conversation

rickycodes
Copy link
Member

@rickycodes rickycodes commented Mar 11, 2021

Description

Opening this as a Draft as it relies on MetaMask/core#381

This adds a new setting under Advanced that lets you edit the nonce rather than getting it from the network:

image

Displays suggested nonce when you do a new send:

image

which can then be edited:

image

image

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #2126

@rickycodes rickycodes added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 12, 2021
@cjeria
Copy link

cjeria commented Mar 17, 2021

For the warning copy, instead of repeating what is already mentioned in the default copy below the warning, I'm now thinking we should mention what could happen if they continue with the higher than suggested nonce.

How about something like this instead? cc @rickycodes @omnat @jakehaugen
image

package.json Outdated Show resolved Hide resolved
@rickycodes rickycodes force-pushed the feature/custom-nonce branch 4 times, most recently from 18b4c99 to 76d4466 Compare March 24, 2021 15:21
@rickycodes rickycodes marked this pull request as ready for review March 24, 2021 15:53
@rickycodes rickycodes requested a review from a team as a code owner March 24, 2021 15:53
app/core/Engine.js Outdated Show resolved Hide resolved
@omnat omnat added the design-review Any feature that needs feedback from the design team label Mar 29, 2021
* develop:
  v2.1.0 (#2481)
  Analytics v2 (priority 1) (#2456)
  Fix/gas estimations (#2408)
@rickycodes rickycodes added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed design-review Any feature that needs feedback from the design team labels Apr 8, 2021
@rickycodes
Copy link
Member Author

I've gone ahead and removed the design qa label since I've gone over this with Christian and he's okay'd it all. re: #2371 (review) I've addressed all QA feedback except for issue 3 as discussed. this is ready for qa again.

export const getNetworkNonce = async ({ from }) => {
const { TransactionController } = Engine.context;
const networkNonce = await util.query(TransactionController.ethQuery, 'getTransactionCount', [from, 'pending']);
return parseInt(networkNonce, 16);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice and clean 👌

rickycodes and others added 3 commits April 8, 2021 13:24
…on/index.js

Co-authored-by: Esteban Miño <efmino@uc.cl>
…obile into feature/custom-nonce

* 'feature/custom-nonce' of github.com:MetaMask/metamask-mobile:
  Update app/components/UI/TransactionReview/TransactionReviewInformation/index.js
@estebanmino estebanmino removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 8, 2021
Copy link
Contributor

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@ibrahimtaveras00 ibrahimtaveras00 added next release QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 12, 2021
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All issues resolved, QA Passed 👍🏽

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Apr 14, 2021
@rickycodes rickycodes merged commit 7b167b8 into develop Apr 14, 2021
@rickycodes rickycodes deleted the feature/custom-nonce branch April 14, 2021 19:22
rickycodes added a commit that referenced this pull request Jan 31, 2022
* Add custom nonce switch

* update snapshot

* Use props

* Get edit nonce button in place

* Add edit nonce modal

* Add translation strings

* Add CustomNonceModal component

* Add proposedNonce

* undo

* Update styles and translation strings

* Use a hook

* Save nonce to state

* Add IncrementDecrementSvg component

* Add comment for newly added prop

* Fix width @ 80

* Add component tests

* Add tests

* Display warning

* Update tests

* Add getProposedNonce

* Merge some utils and add tests

* Add constants

* Use object property shorthand

* Fix displayWarning to account for String

* Add ModalDragger

* Add save and cancel

* Use the TransactionController from "@metamask/controllers"

* Only use custom nonce if showCustomNonce

* Update snapshot

* Start using Base/Text

* Add colors to Base/Text component

* Account for NaN values

* Use Base/Text

* Move increment decrement buttons down

* Update tests

* Add showSoftInputOnFocus={false}

* Update tests

* Add icon

* Use link

* Update tests

* prep for QA

* Update warning text

* Use 6.2.0 controllers

* Update translation strings

* update snapshot

* Remove TODO

* Use EvilIcons

* Update snapshot

* Beef up tests for toLowerCaseCompare

* Remove comment

* update snapshot

* Get nonce from network

* Update snapshot

* fix lint nits

* fix copy

* Update snapshot

* Move edit nonce functionality into TransactionReviewFeeCard

* Add custom nonce to dapp send

* Use redux actions

* Continue using actions

* Set custom nonce on confirm

* Update snapshot

* Move nonce and proposedNonce off transaction object

* Bump controllers

* Only getNetworkNonce if proposedNonce is not set

* fix props

* delete tgz

* Add getNetworkNonce to networks utility

* document props

* Update snapshots

* Update app/components/UI/TransactionReview/TransactionReviewInformation/index.js

Co-authored-by: Esteban Miño <efmino@uc.cl>

* use nonce directly instead of passing it in render

Co-authored-by: Esteban Miño <efmino@uc.cl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release QA Passed A successful QA run through has been done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Advanced feature: Add ability to submit a transaction with custom nonce
7 participants