add gas conditional if simple transaction #7043
Conversation
Bundle StatsHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark
| Benchmark suite | Current: 37455eb | Previous: 2f73aa5 | Ratio |
|---|---|---|---|
processingTx |
9318 ops/sec (±3.60%) |
9204 ops/sec (±3.98%) |
0.99 |
processingContractDeploy |
38185 ops/sec (±7.60%) |
38039 ops/sec (±7.10%) |
1.00 |
processingContractMethodSend |
20016 ops/sec (±6.64%) |
19205 ops/sec (±6.35%) |
0.96 |
processingContractMethodCall |
40554 ops/sec (±5.76%) |
40094 ops/sec (±5.17%) |
0.99 |
abiEncode |
45972 ops/sec (±6.71%) |
43919 ops/sec (±6.46%) |
0.96 |
abiDecode |
30557 ops/sec (±7.62%) |
29754 ops/sec (±7.79%) |
0.97 |
sign |
1582 ops/sec (±3.25%) |
1568 ops/sec (±0.52%) |
0.99 |
verify |
376 ops/sec (±0.52%) |
372 ops/sec (±0.68%) |
0.99 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.x #7043 +/- ##
=======================================
Coverage 91.97% 91.98%
=======================================
Files 215 215
Lines 8291 8294 +3
Branches 2277 2278 +1
=======================================
+ Hits 7626 7629 +3
Misses 665 665
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -122,7 +122,14 @@ export class SendTxHelper< | |||
|
|
|||
| public async checkRevertBeforeSending(tx: TransactionCall) { | |||
| if (this.options.checkRevertBeforeSending !== false) { | |||
| const reason = await getRevertReason(this.web3Context, tx, this.options.contractAbi); | |||
| let formatTx = tx; | |||
| if (isNullish(tx.data) && isNullish(tx.input) && isNullish(tx.gas)) { // eth.call runs into error if data isnt filled and gas is not defined, its a simple transaction so we fill it with 21000 | |||
There was a problem hiding this comment.
when data is populated, is gas limit always populated in that case?
There was a problem hiding this comment.
and its only missing in case of value transfers eth_calls only?
There was a problem hiding this comment.
additionally, if tx has gas limit only for eth_call rpc( for revert check) and not in actual Tx that will be sent after it,
eth_call may pass while actual Tx may fail in some cases, as both will differ, so I think gas limit should be applied in all cases
There was a problem hiding this comment.
eth_call with no data and no gas will fail from provider, but send_transaction gas will be filled automatically. i belive this is just a weird edge case, and with our tests passing i dont believe this will be an issue.
Description
#7021
There seems to be a bug when using sendTransaction with no gas property in the rpc method eth_call in
checkRevertBeforeSending. Testing with both metamask and an infura provider, with a simple transaction (with no data or input so no contract call)eth_call will revert and say there is insufficient funds when there should be easily enough funds to cover the gas and value.
The transaction looks like
Solution
Within
checkRevertBeforeSending, A simple fix to this is to check if the transaction does not have data set we explicitly set the gas to 21000Please include a summary of the changes and be sure to follow our Contribution Guidelines.
Type of change
Checklist:
npm run lintwith success and extended the tests and types if necessary.npm run test:unitwith success.npm run test:coverageand my test cases cover all the lines and branches of the added code.npm run buildand testeddist/web3.min.jsin a browser.CHANGELOG.mdfile in the root folder.