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

Update addTxGasDefaults and _getDefaultGasFees to work with new estimate types, and ensure we correctly handle gas price estimates when on EIP1559 networks #11615

Merged
merged 7 commits into from
Jul 29, 2021

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Jul 26, 2021

This PR ensures that transactions created by dapps will work correctly, on both EIP1559 compatible networks and on networks without compatibility, and both for txes with dapp suggested fees and txes where the dapp does not suggest the fee. It also ensures that if we are on an EIP1559 network, and our request for gas fee estimates fails and so we have to fall back to a eth_gasPrice call, that those estimates are handled correctly as well.

eth_gasPrice estimates on an EIP-1559 network are to be set to the maxFeePerGas and the maxPriorityFeePerGas

For testing, it is recommended to use this branch: MetaMask/test-dapp#119

(I will add some demo videos here a bit later)

To do:

  • Remove controllers version bump, only included here for QA purposes
  • Needs a rebase

@danjm danjm requested a review from a team as a code owner July 26, 2021 13:37
@danjm danjm requested review from PeterYinusa and removed request for a team July 26, 2021 13:37
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

This is excellent Dan! Thank you so much!

@metamaskbot
Copy link
Collaborator

Builds ready [503d625]
Page Load Metrics (638 ± 25 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint51806384
domContentLoaded5337496375225
load5347506385225
domInteractive5337496365225

@metamaskbot
Copy link
Collaborator

Builds ready [8782855]
Page Load Metrics (712 ± 44 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint54856894
domContentLoaded5998847119144
load6008857129244
domInteractive5998847109144

@darkwing darkwing added the EIP-1559 Tasks required to complete 1559 support label Jul 28, 2021
@danjm danjm force-pushed the eip1559-improved-txcontroller-support branch from cbc3cad to 982aa55 Compare July 28, 2021 20:45
@metamaskbot
Copy link
Collaborator

Builds ready [982aa55]
Page Load Metrics (690 ± 29 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint59806773
domContentLoaded6178366896029
load6188446906129
domInteractive6168366896029

@metamaskbot
Copy link
Collaborator

Builds ready [0ce50e8]
Page Load Metrics (632 ± 23 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint49886595
domContentLoaded5517926304722
load5528036324823
domInteractive5517926304722

@danjm danjm force-pushed the eip1559-improved-txcontroller-support branch from 0ce50e8 to a7e4c10 Compare July 29, 2021 16:30
@@ -449,11 +451,13 @@ export default class ConfirmTransactionBase extends Component {
/>,
])}
subTitle={
<GasTiming
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually doesn't need to happen in this PR and will be removed when I correctly rebase. But It is okay to have it here for now, so that the error it throws in not a distraction for QA. It has been fixed properly on another PR that will soon be merged

  • Remove change after rebasing onto develop

@danjm danjm changed the title Update addTxGasDefaults and _getDefaultGasFees to work correctly with all new gas fee estimate types Update addTxGasDefaults and _getDefaultGasFees to work with new estimate types, and ensure we correctly handle gas price estimates when on EIP1559 networks Jul 29, 2021
@danjm danjm force-pushed the send-flow-eip-1559 branch 2 times, most recently from b4e8981 to a1e030d Compare July 29, 2021 19:53
@danjm danjm force-pushed the eip1559-improved-txcontroller-support branch from a7e4c10 to e0c9eec Compare July 29, 2021 19:56
@danjm danjm force-pushed the eip1559-improved-txcontroller-support branch from e0c9eec to 742622f Compare July 29, 2021 20:20
@metamaskbot
Copy link
Collaborator

Builds ready [742622f]
Page Load Metrics (682 ± 36 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint56836794
domContentLoaded5548436807636
load5558456827636
domInteractive5538426797636

@PeterYinusa
Copy link
Contributor

LGTM!

@danjm danjm merged commit df041cf into send-flow-eip-1559 Jul 29, 2021
@danjm danjm deleted the eip1559-improved-txcontroller-support branch July 29, 2021 21:44
@github-actions github-actions bot locked and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
EIP-1559 Tasks required to complete 1559 support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants