Skip to content

Use different intrinsic-gas for fee-currencies in gas-estimation#356

Merged
gastonponti merged 15 commits intocelo-rebase-12from
gastonponti/fix-gas-estimation-with-fee-currency
Apr 4, 2025
Merged

Use different intrinsic-gas for fee-currencies in gas-estimation#356
gastonponti merged 15 commits intocelo-rebase-12from
gastonponti/fix-gas-estimation-with-fee-currency

Conversation

@gastonponti
Copy link
Copy Markdown

@gastonponti gastonponti commented Apr 1, 2025 •
edited
Loading

Short-circuit for gasEstimation of txs that transfer Celo, with the gas paid with a different feeCurrency
Avoid unnecessary contract calls for rates (every run was calculating it)
Refactor to have the gasestimator self-contained (have the feeCurrency balance calculated inside the gas estimator)

Solves #246

Also:

  • The ethclient function to make a gasEstimation was not adding the feeCurrency, which was returning a smaller gasEstimation due to missing to add the intrinsic gas from that feeCurrency

palango
palango reviewed Apr 1, 2025
View reviewed changes
Copy link
Copy Markdown

@palango palango left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look good and similar to what's done in celo-blockchain.

A test for this would be nice!

@gastonponti
Copy link
Copy Markdown
Author

gastonponti commented Apr 1, 2025 •
edited
Loading

A test for this would be nice!

I'm trying to understand if there is an easy way to do this without a refactor that will increase the codediff, mostly because we need to mock or set the contracts for erc20 debit and credits
There's no even a test for the gasestimator in geth nor op-geth

@karlb
Copy link
Copy Markdown

karlb commented Apr 2, 2025

Unittests might be hard, but we have fee currencies available in the e2e tests. Maybe you can reproduce the original problem there.

@gastonponti
Copy link
Copy Markdown
Author

gastonponti commented Apr 2, 2025

Unittests might be hard, but we have fee currencies available in the e2e tests. Maybe you can reproduce the original problem there.

Yes, I thought it was going to be harder with the e2e, but found some examples that are similar and that's my take now. Thanks!

@gastonponti gastonponti changed the title Fix gas estimation with fee currency Use different intrinsic-gas for fee-currencies in gas-estimation Apr 2, 2025
palango
palango reviewed Apr 3, 2025
View reviewed changes
palango
palango reviewed Apr 3, 2025
View reviewed changes
@palango palango requested a review from ezdac April 3, 2025 14:53
palango
palango reviewed Apr 4, 2025
View reviewed changes
Copy link
Copy Markdown

@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.

Short-circuit for gasEstimation of txs that transfer Celo, with the gas paid with a different feeCurrency
Avoid unnecessary contract calls for rates (every run was calculating it)

For me it looks the other way around

gastonponti
gastonponti commented Apr 4, 2025
View reviewed changes
palango
palango approved these changes Apr 4, 2025
View reviewed changes
Copy link
Copy Markdown

@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.

Bugfix with a nice performance improvement 🎉

@gastonponti gastonponti merged commit f695a25 into celo-rebase-12 Apr 4, 2025
8 checks passed
@gastonponti gastonponti deleted the gastonponti/fix-gas-estimation-with-fee-currency branch April 4, 2025 14:37
piersy pushed a commit that referenced this pull request May 6, 2025
@gastonponti @piersy
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
ad38c75 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
piersy pushed a commit that referenced this pull request May 6, 2025
@gastonponti @piersy
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
beaabf8 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
piersy pushed a commit that referenced this pull request May 6, 2025
@gastonponti @piersy
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
59ee801 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
piersy pushed a commit that referenced this pull request May 8, 2025
@gastonponti @piersy
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
d57fc56 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
piersy pushed a commit that referenced this pull request May 12, 2025
@gastonponti @piersy
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
9644b9a 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
piersy pushed a commit that referenced this pull request May 22, 2025
@gastonponti @piersy
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
560e7b1 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
piersy pushed a commit that referenced this pull request May 22, 2025
@gastonponti @piersy
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
deeb634 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 20, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
6fff6ef 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 21, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
558b49a 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 26, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
f69940f 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 28, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
526303d 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 30, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
d6d0ee1 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 30, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
6155ad0 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 30, 2025
@gastonponti @Kourin1996
[e2e] Use different intrinsic-gas for fee-currencies in gas-estimation (
7efa702 
#356)

Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 30, 2025
@gastonponti @Kourin1996
[squash] Use different intrinsic-gas for fee-currencies in gas-estima…
58f64e7 
…tion (#356)
Kourin1996 pushed a commit that referenced this pull request Jul 30, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
0461596 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls

[squash] Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
d34a2d2 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
e2e(runner): Add WebSocket support in e2e (#356)
1dad59b
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
e2e(fee_currency): Add gas estimation e2e test (#356)
43c8db0
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
squash: Use different intrinsic-gas for fee-currencies in gas-estimat…
9e8210b 
…ion (#356)

Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
1e8b3db 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
e2e(runner): Add WebSocket support in e2e (#356)
34695a5
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
e2e(fee_currency): Add gas estimation e2e test (#356)
1ff4e64
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
squash: Use different intrinsic-gas for fee-currencies in gas-estimat…
93a06d5 
…ion (#356)

Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
e25fb56 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
e2e(runner): Add WebSocket support in e2e (#356)
1f49080
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
e2e(fee_currency): Add gas estimation e2e test (#356)
d194856
Kourin1996 pushed a commit that referenced this pull request Jul 31, 2025
@gastonponti @Kourin1996
squash: Use different intrinsic-gas for fee-currencies in gas-estimat…
1ef26da 
…ion (#356)

Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Aug 1, 2025
@gastonponti @Kourin1996
Use different intrinsic-gas for fee-currencies in gas-estimation (#356)
f3dba13 
Refactor gasestimator. Cleaner and avoid unnecessary contract calls
Kourin1996 pushed a commit that referenced this pull request Aug 1, 2025
@gastonponti @Kourin1996
e2e(runner): Add WebSocket support in e2e (#356)
8bd346d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@palango palango palango approved these changes

@ezdac ezdac Awaiting requested review from ezdac

Assignees

@palango palango

@ezdac ezdac

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gastonponti @karlb @palango @ezdac