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

Adapt gasPrice to post-London/EIP1559 fees parameters #2953

Merged
merged 4 commits into from
Sep 21, 2021
Merged

Conversation

andrevmatos
Copy link
Contributor

Fixes #2952

Short description
London hardfork replaced the previous guess-based gasPrice parameter for transactions with a maxFeePerGas and maxPriorityFeePerGas pair. Recent ethers versions (5.4+) supports this through a provider.getFeeData() method and the respective tx overrides parameters. We update the Latest.gasPrice observable to receive an object (possibly undefined) to be spread into overrides, containing either legacy (if needed) or new properties, and now config.gasPriceFactor (used by CLI to set priority txs) acts only upon maxPriorityFeePerGas.

An undefined gas price overrides should ensure ethers is left to do the right thing™ on its own, and we try to meddle with these values as little as possible through only the priority fee on CLI only.

This should make suggested tx fee parameters more deterministic and also less wasteful, as the legacy gasPrice-only value often caused issues when trying to guesstimate a the price for a new tx.

Possibly, this may also help with #2914, but we may want a mechanism to force the prices there to be zero, or whatever Arbitrum nodes require. At least we remove the hardcoded minimum of 1 Gwei (although ethers still hardcodes a default maxPriorityFeePerGas of 2.5 Gwei, but this is going to get smarter in the near future).

Definition of Done

  • Steps to manually test the change have been documented
  • Acceptance criteria are met
  • Change has been manually tested by the reviewer (dApp)

Steps to manually test the change (dApp)

  1. Issue doesn't happen anymore.

Latest.gasPrice becomes an object (or undefined) to be spread into tx's
`overrides`, and by default should contain post-london `maxFeePerGas`
and `maxPriorityFeePerGas`, as given by `Provider.getFeeData`;
`config.gasPriceFactor` then gets applied only over
`maxPriorityFeePerGas`, to tweak priority fees only. Default leaves it
as `undefined`, and let Metamask/provider guess the best gas parameters.
since now this is an object containing overrides (or undefined), instead
of a fixed value.
@andrevmatos andrevmatos added enhancement New feature or request sdk 🖥 cli 🔤 Command-line SDK-based Node.js Client issues labels Sep 20, 2021
@andrevmatos andrevmatos self-assigned this Sep 20, 2021
@codecov
Copy link

codecov bot commented Sep 20, 2021

Codecov Report

Merging #2953 (a6c8b0c) into master (490b7b4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2953      +/-   ##
==========================================
+ Coverage   93.32%   93.34%   +0.01%     
==========================================
  Files         207      207              
  Lines        8830     8841      +11     
  Branches     1382     1389       +7     
==========================================
+ Hits         8241     8253      +12     
+ Misses        488      487       -1     
  Partials      101      101              
Flag Coverage Δ
dapp 88.95% <ø> (ø)
dapp.unit 88.95% <ø> (ø)
sdk 95.90% <100.00%> (+0.02%) ⬆️
sdk.e2e 72.25% <38.23%> (+0.16%) ⬆️
sdk.integration 79.87% <97.05%> (+0.07%) ⬆️
sdk.unit 48.89% <14.70%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
raiden-ts/src/channels/epics/close.ts 93.84% <ø> (ø)
raiden-ts/src/channels/epics/deposit.ts 91.66% <ø> (ø)
raiden-ts/src/config.ts 94.59% <ø> (ø)
raiden-ts/src/transfers/epics/withdraw.ts 94.96% <ø> (ø)
raiden-ts/src/types.ts 100.00% <ø> (ø)
raiden-ts/src/channels/actions.ts 100.00% <100.00%> (ø)
raiden-ts/src/channels/epics/block.ts 100.00% <100.00%> (ø)
raiden-ts/src/channels/epics/open.ts 100.00% <100.00%> (ø)
raiden-ts/src/channels/epics/settle.ts 94.11% <100.00%> (ø)
raiden-ts/src/channels/utils.ts 100.00% <100.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 490b7b4...a6c8b0c. Read the comment docs.

@agatsoh
Copy link
Contributor

agatsoh commented Sep 21, 2021

Tested it on the mainnet with a deposit. Works
https://etherscan.io/tx/0xa2704ed647a8751bb69745c05ba22a097bede88ee889d13139629c22350a2345

@agatsoh
Copy link
Contributor

agatsoh commented Sep 21, 2021

Ok this observation also needs to be taken care of, the transactions take almost 15 to 20 minutes to get confirmed and this approve is yet to be confirmed.
https://etherscan.io/tx/0x8d85ff76a12e682e772e6af79de18ef7825ddf375dc1102bad6c155a5037764a
@andrevmatos FYI

@andrevmatos
Copy link
Contributor Author

@agatsoh you sure you didn't edit the gas price on Metamask? Your pending TX contains a quite low maxFeePerGas, and your maxPriorityFeePerGas is only 1.5 Gwei, while my PR defaults to 2.5 Gwei, so it doesn't seem it was the plain gas price set by it. Can you check, please?

@agatsoh
Copy link
Contributor

agatsoh commented Sep 21, 2021

@andrevmatos I absolutely did not edit anything on metamask. I have seen that is pretty odd actually. Mostly in other transaction the maxFeePerGas and maxPriorityFeePerGas is practically copied over and is the same. But in these two transactions it is different.
https://etherscan.io/tx/0x8d85ff76a12e682e772e6af79de18ef7825ddf375dc1102bad6c155a5037764a
https://etherscan.io/tx/0x1acdf335e69defa3ea71dd56d0a921c69cc3bc073a181b8c38864c838fcb05cf

raiden-ts/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@palango palango left a comment

Choose a reason for hiding this comment

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

Looks good, only a small formatting fix in the changelog necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli 🔤 Command-line SDK-based Node.js Client issues enhancement New feature or request sdk 🖥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

err: max fee per gas less than block base fee mainnet
3 participants