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

[Flow EVM] Add test for tx with zero fee or dynamic fees #5432

Merged
merged 10 commits into from
Feb 24, 2024

Conversation

ramtinms
Copy link
Contributor

@ramtinms ramtinms commented Feb 21, 2024

This PR adds some tests for the emulator for transactions with gas fees of zero or dynamic fees.

@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 55.48%. Comparing base (8afb152) to head (75e1537).
Report is 72 commits behind head on master.

Files Patch % Lines
fvm/evm/types/call.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5432      +/-   ##
==========================================
- Coverage   56.00%   55.48%   -0.53%     
==========================================
  Files        1024     1000      -24     
  Lines       99327    96203    -3124     
==========================================
- Hits        55632    53380    -2252     
+ Misses      39455    38754     -701     
+ Partials     4240     4069     -171     
Flag Coverage Δ
unittests 55.48% <33.33%> (-0.53%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramtinms ramtinms changed the title [Flow EVM] disable effective gas [Flow EVM] disable effective gas fee calculation Feb 21, 2024
@@ -87,7 +87,7 @@ func defaultConfig() *Config {
CanTransfer: gethCore.CanTransfer,
Transfer: gethCore.Transfer,
GasLimit: BlockLevelGasLimit,
BaseFee: big.NewInt(0),
Copy link
Contributor Author

@ramtinms ramtinms Feb 21, 2024

Choose a reason for hiding this comment

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

It seems the underlying code is dependent on nil than zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just hate go-ethereum for this kind of stuff, I just spent yesterday too much time on realising that I need to return a nil value for something to work, albeit being an anti-pattern in go, since you need to return an error in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we endup not needing this, so this PR just adds more tests :D

@ramtinms ramtinms marked this pull request as ready for review February 21, 2024 23:20
@ramtinms ramtinms requested a review from sideninja February 21, 2024 23:20
@ramtinms ramtinms changed the title [Flow EVM] disable effective gas fee calculation [Flow EVM] Disable Effective Gas Fee Calculation Feb 21, 2024
@sideninja
Copy link
Contributor

I will try to test this in the emulator later today and if it works I would say this would be a nice thing to have deployed on previewnet. Further more I would make this a config option of the EVM. Maybe the latter is a request for change.

@ramtinms ramtinms changed the title [Flow EVM] Disable Effective Gas Fee Calculation [Flow EVM] Add test for tx with zero fee or dynamic fees Feb 22, 2024
@ramtinms ramtinms added this pull request to the merge queue Feb 24, 2024
Merged via the queue into master with commit 9ca5a55 Feb 24, 2024
50 of 51 checks passed
@ramtinms ramtinms deleted the ramtin/flow-evm-disable-base-fee branch February 24, 2024 02:19
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.

4 participants