Resubmit by frankiebee · Pull Request #1695 · MetaMask/metamask-extension

frankiebee · 2017-06-29T02:22:38Z

create a button for resubmitting transactions with the same nonce

…nored

…e nonce-tracker does not include them

frankiebee · 2017-06-29T23:34:44Z

Dapps should not be allowed to specify nonce only metamask. insure that this is the case.

kumavis · 2017-07-05T17:44:52Z

app/scripts/controllers/transactions.js

      await this.publishTransaction(txId, rawTx)
      // must set transaction to submitted/failed before releasing lock
-      nonceLock.releaseLock()
+      if (nonceLock) nonceLock.releaseLock()


why would we not have a nonceLock here

kumavis · 2017-07-05T17:46:33Z

app/scripts/controllers/transactions.js

+    return new Promise((resolve, reject) => {
+      this.addUnapprovedTransaction(txMeta.txParams, (err, newTxMeta) => {
+        if (err) {
+          delete txMeta.ignore


why would we have txMeta.ignore here

we agreed to remove

kumavis · 2017-07-05T17:47:19Z

app/scripts/controllers/transactions.js

+        if (err) {
+          delete txMeta.ignore
+          this.updateTx(txMeta)
+          reject(err)


should we return here or do you intend to keep going and set the ignore to true and resolve the tx

we agreed to return here

kumavis · 2017-07-05T17:48:18Z

app/scripts/controllers/transactions.js

    })
  }

+  _updateNonceDuplicates () {


please add comments explaining what this does

we agreed that it would be easier to not maintain this list but just create in on demand
so we should create a new fn for nullifying txs from the same address with the same nonce

kumavis · 2017-07-05T17:50:00Z

app/scripts/lib/nonce-tracker.js

+    const pendingTransactions = this.getPendingTransactions(address)
    // await lock free
-    await this.lockMap[address]
+    if (pendingTransactions.length) await this.lockMap[address]


use brackets for if-else statements
prolly should put this.lockMap[address] into a var before using it since you look it up many times

this looks suspect

await this.lockMap[address] vs await this.lockMap[address]()

lockMap is promise, shouldnt be called

we agreed to remove the pendingTransactions check before awaiting the nonce lock
(but you didnt remember why you put this here)
basically this (line 17-18) should look as it did before, simply await this.lockMap[address]

kumavis · 2017-07-05T17:51:02Z

ui/app/components/transaction-list-item-icon.js


 TransactionIcon.prototype.render = function () {
  const { transaction, txParams, isMsg } = this.props
+  const isIgnored = (transaction.ignore && transaction.status === 'submitted')


why is the status relevant if the flag is set to ignore?

kumavis · 2017-07-05T17:51:26Z

ui/app/components/transaction-list-item.js

  const nonce = txParams.nonce ? numberToBN(txParams.nonce).toString(10) : ''

  const isClickable = ('hash' in transaction && isLinkable) || isPending
+  const isIgnored = (transaction.ignore && transaction.status === 'submitted')


same as above,

why is the status relevant if the flag is set to ignore?

kumavis · 2017-07-05T18:09:03Z

app/scripts/controllers/transactions.js

+      this.addUnapprovedTransaction(txMeta.txParams, (err, newTxMeta) => {
+        if (err) {
+          delete txMeta.ignore
+          this.updateTx(txMeta)


wont need to update the tx either

kumavis · 2017-07-05T18:56:36Z

app/scripts/lib/nonce-tracker.js

-    const blockNumber = currentBlock.number
-    const pendingTransactions = this.getPendingTransactions(address)
-    const baseCount = await this._getTxCount(address, blockNumber)
+    const baseCount = await this._getTxCount(address)


the order of requests here is really important:

get current block

get pending transactions

get base tx count @ block

calculate new nonce
we should add comments explaining why this is:

we need to make sure our base count and pending count are from the same block

…etwork coverage on extension (#22618) ## **Description** - What's new copy should be updated to: > Steer clear of known scams while still preserving your privacy with security alerts powered by Blockaid. This feature is available on Arbitrum, Avalanche, BNB chain, Ethereum Mainnet, Linea, Optimism and Polygon. > > Always do your own due diligence before approving requests. - Settings copy should be updated to: > Privacy preserving - no data is shared with third parties. Available on Arbitrum, Avalanche, BNB chain, Ethereum Mainnet, Linea, Optimism and Polygon. ## **Related issues** Fixes: [#1695](https://github.com/MetaMask/MetaMask-planning/issues/1695) Blocked By: #22070 ## **Screenshots/Recordings** ### **Before** https://github.com/MetaMask/metamask-extension/assets/44811/0e6b6103-efb0-4ed8-94c2-44ac0f281ff8 ### **After** https://github.com/MetaMask/metamask-extension/assets/44811/36740992-0af2-44c5-b62b-0ac522df4d17 ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

…etwork coverage on extension (#22618) ## **Description** - What's new copy should be updated to: > Steer clear of known scams while still preserving your privacy with security alerts powered by Blockaid. This feature is available on Arbitrum, Avalanche, BNB chain, Ethereum Mainnet, Linea, Optimism and Polygon. > > Always do your own due diligence before approving requests. - Settings copy should be updated to: > Privacy preserving - no data is shared with third parties. Available on Arbitrum, Avalanche, BNB chain, Ethereum Mainnet, Linea, Optimism and Polygon. ## **Related issues** Fixes: [#1695](MetaMask/MetaMask-planning#1695) Blocked By: #22070 ## **Screenshots/Recordings** ### **Before** https://github.com/MetaMask/metamask-extension/assets/44811/0e6b6103-efb0-4ed8-94c2-44ac0f281ff8 ### **After** https://github.com/MetaMask/metamask-extension/assets/44811/36740992-0af2-44c5-b62b-0ac522df4d17 ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

…etwork coverage on extension (#22618) ## **Description** - What's new copy should be updated to: > Steer clear of known scams while still preserving your privacy with security alerts powered by Blockaid. This feature is available on Arbitrum, Avalanche, BNB chain, Ethereum Mainnet, Linea, Optimism and Polygon. > > Always do your own due diligence before approving requests. - Settings copy should be updated to: > Privacy preserving - no data is shared with third parties. Available on Arbitrum, Avalanche, BNB chain, Ethereum Mainnet, Linea, Optimism and Polygon. ## **Related issues** Fixes: [#1695](https://github.com/MetaMask/MetaMask-planning/issues/1695) Blocked By: #22070 ## **Screenshots/Recordings** ### **Before** https://github.com/MetaMask/metamask-extension/assets/44811/0e6b6103-efb0-4ed8-94c2-44ac0f281ff8 ### **After** https://github.com/MetaMask/metamask-extension/assets/44811/36740992-0af2-44c5-b62b-0ac522df4d17 ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

frankiebee added 2 commits June 27, 2017 16:46

nonce-tracker: only check transactions that are not supposed to be ig…

690685d

…nored

Mark transactions as "ignore" so they dont get rebrodcasted and so th…

20b1240

…e nonce-tracker does not include them

frankiebee added the in progress label Jun 29, 2017

Add a "resubmit button for transactions"

0026d65

frankiebee force-pushed the resubmit branch from 0765d6a to 0026d65 Compare June 29, 2017 02:24

prevent dapps from setting the nonce

640f283

frankiebee force-pushed the resubmit branch from c39ae5d to 640f283 Compare June 30, 2017 01:53

kumavis reviewed Jul 5, 2017

View reviewed changes

kumavis requested changes Jul 5, 2017

View reviewed changes

kumavis reviewed Jul 5, 2017

View reviewed changes

frankiebee added the DO-NOT-MERGE Pull requests that should not be merged label Jul 5, 2017

frankiebee closed this Jul 11, 2017

frankiebee deleted the resubmit branch July 11, 2017 19:09

frankiebee removed the in progress label Jul 11, 2017

Conversation

frankiebee commented Jun 29, 2017

Uh oh!

frankiebee commented Jun 29, 2017

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants