Skip to content

feat: eip1559 fees#4324

Merged
gomesalexandre merged 32 commits intodevelopfrom
eip1559-fees
Apr 25, 2023
Merged

feat: eip1559 fees#4324
gomesalexandre merged 32 commits intodevelopfrom
eip1559-fees

Conversation

@kaladinlight
Copy link
Contributor

@kaladinlight kaladinlight commented Apr 20, 2023

Description

Update to use EIP-1559 fee where possible in swapper

  • approvals (zrx, thorswap, cowswap, lifi, univ2, fox-farming)
  • sends/swaps (zrx, thorswap, cowswap, univ2, fox-farming)
  • using average eip1559 gas price and fast legacy gas price

Lifi will be a follow up for the rest of the flow outside of approval.

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

Risk

high - this touches a lot of the gas logic which is very fragmented across the code base with possibility of a missed edge case

Testing

  • zrx full functionality
  • thorswap full functionality
  • cowswap full functionality
  • lifi full functionality
  • uniV2 full functionality
  • fox-farming full functionality

Engineering

☝️

Operations

☝️

Screenshots (if applicable)

ZRX:

Thor:

FoxFarming:

UniV2:

@0xdef1cafe 0xdef1cafe changed the title Eip1559 fees feat: eip1559 fees Apr 21, 2023
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Untested but conceptually looks excellent so far

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tested with a lil help from Tenderly:

UNI-V2 approvals

FOX

image

UNI (More logic in their approve() method which makes us require a bit of buffer currently, see https://github.com/Uniswap/governance/blob/eabd8c71ad01f61fb54ed6945162021ee419998e/contracts/Uni.sol#L151-L155)

image

FOX Farming

https://etherscan.io/tx/0xbb33a4111850fda35d3811fc9d9d483081e47332b3eca09494de34d0561d8fcb

ZRX Swapper using the 100000 default APPROVAL_GAS_LIMIT

image

Not for this PR, but we may want to not hardcode a gas limit for approvals anymore in the future, now that we have more sane estimations

CoW swapper using the default APPROVAL_GAS_LIMIT

image

THOR Swapper

image

Sends/swaps

THOR Swapper using the 100000 THOR_EVM_GAS_LIMIT

image

CoW swapper

Unable to test with Tenderly since this uses signatures and would imply broadcasting an actual Tx - this diff should produce no changes here

ZRX

Unable to test without testing funds, though running through the gas limit / usage of previous similar calls over the last minutes for the sellToUniswap() method, our gasLimit has enough buffer here ✅ will obviously need to be tested by ops

Didn't notice any regressions here, according to Tenderly which gives us the same actual deterministic guarantees as broadcasting Txs.
Will obviously need some ops love of legacy/EIP-1559 after this goes in, and ideally shapeshift/hdwallet#614 to go in and be bumped here as well to ensure MM uses EIP-1559 fees, but conceptually looks good to me functionally so far!

@kaladinlight kaladinlight marked this pull request as ready for review April 25, 2023 17:18
@kaladinlight kaladinlight requested a review from a team as a code owner April 25, 2023 17:18
Copy link
Contributor

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

LGTM after the latest changes and the UNI-V2 addLiquidityEth ETH value fix - re-tested in a poile

@gomesalexandre gomesalexandre merged commit 1047a1a into develop Apr 25, 2023
@gomesalexandre gomesalexandre deleted the eip1559-fees branch April 25, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants