Ensure correct tx category when sending to contracts without tx data #7252
Conversation
…re is no txParams data
Builds ready [6157275]
|
ui/app/pages/send/send.utils.js
Outdated
| @@ -243,12 +244,21 @@ async function estimateGas ({ | |||
| } | |||
|
|
|||
| // if not, fall back to block gasLimit | |||
| if (!blockGasLimit) { | |||
| try { | |||
| blockGasLimit = await this.query.getBlockByNumber('latest', false) | |||
There was a problem hiding this comment.
Won't getBlockByNumber return a block object, not a number string like we probably expect here?
There was a problem hiding this comment.
I believe it's impossible for blockGasLimit to ever be falsey, because it's given a default value of MIN_GAS_LIMIT_HEX
There was a problem hiding this comment.
I've updated this to something simpler. We don't really need a strictly accurate blockGasLimit at this point.
Meanwhile, I think blockGasLimit can be falsey if the value passed to this function is defined but falsey.
e.g.
danjm@pop-os:~/kyokan/metamask-extension$ node
> const t = ({ a = 10, b }) => console.log(`${a} | ${b}`)
undefined
> t({ a: null, b: 5})
null | 5
undefined
> t({ a: '', b: 5})
| 5
undefined
> t({ b: 5})
10 | 5
undefined
Builds ready [11c5b15]
|
| @@ -243,12 +244,17 @@ async function estimateGas ({ | |||
| } | |||
|
|
|||
| // if not, fall back to block gasLimit | |||
| if (!blockGasLimit) { | |||
| blockGasLimit = ARBITRARY_HIGH_BLOCK_GAS_LIMIT | |||
There was a problem hiding this comment.
I'm not entirely sure how the problem this is solving (the account tracker not having a valid current block gas limit) relates to the bug this PR is meant to address. That problem might also be better solved in the background, though I'm not sure how... maybe by having a valid default instead of empty string, and/or exposing the 'loading' state to the UI so it can wait for the first block to get read, and/or dealing with errors in a different manner.
I suppose setting this default here for the time being is fine though; maybe it's the quickest way to fix this for now.
There was a problem hiding this comment.
Oh, yeah, sorry. This solves an additional bug that was discovered while QAing the fix to the primary bug. It relates in the sense that it is a bug in our gas estimation logic.
I would say a background fix may also be needed, but as it stands, if this function receives a falsey but defined blockGasLimit, it will break as such a value will cause an error when passed to multiplyCurrencies below.
…7252) * Ensure correct transaction category when sending to contracts but there is no txParams data * Update gas when pasting address in send * Gracefully fall back is send.util/estimateGas when blockGasLimit from background is falsy * Remove network request frontend fallback for blockGasLimit * Add some needed slow downs to e2e tests
…7252) * Ensure correct transaction category when sending to contracts but there is no txParams data * Update gas when pasting address in send * Gracefully fall back is send.util/estimateGas when blockGasLimit from background is falsy * Remove network request frontend fallback for blockGasLimit * Add some needed slow downs to e2e tests
* master: (34 commits) Update changelog for v7.2.3 Fix e2e tests and gas default (#7267) Do not transate on seed phrases test:integration - fix renamed test data file lint fix test:e2e - fix bail condition test:e2e - fix responsie argument test:e2e - refactor missed spec file test:e2e - only overwrite window.fetch once per session test:e2e - rework fetch-mocks test:e2e - add extra delay before closing popups test:e2e - factor out prepareExtensionForTesting test - e2e - dedupe fetchMocking + compose script as fn Ensure correct tx category when sending to contracts without tx data (#7252) Version v7.2.3 Add v7.2.2 to changelog Update minimum Firefox verison to 56.0 (#7213) Version v7.2.2 Update changelog for v7.2.1, v7.2.0, and v7.1.1 Add `appName` message to each locale ...
* origin/master: (34 commits) Update changelog for v7.2.3 Fix e2e tests and gas default (#7267) Do not transate on seed phrases test:integration - fix renamed test data file lint fix test:e2e - fix bail condition test:e2e - fix responsie argument test:e2e - refactor missed spec file test:e2e - only overwrite window.fetch once per session test:e2e - rework fetch-mocks test:e2e - add extra delay before closing popups test:e2e - factor out prepareExtensionForTesting test - e2e - dedupe fetchMocking + compose script as fn Ensure correct tx category when sending to contracts without tx data (#7252) Version v7.2.3 Add v7.2.2 to changelog Update minimum Firefox verison to 56.0 (#7213) Version v7.2.2 Update changelog for v7.2.1, v7.2.0, and v7.1.1 Add `appName` message to each locale ...
Fixes #7115