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

[EVM] Fix the setup process #5069

Merged
merged 3 commits into from
Nov 29, 2023
Merged

[EVM] Fix the setup process #5069

merged 3 commits into from
Nov 29, 2023

Conversation

sideninja
Copy link
Member

Closes: #5068

This PR fixes an issue with EVM setup. It moves the setup in the preprocess of the transaction invoker, since the Cadence env needs to have EVMInternal values set during the preprocess as it loads the transaction program and the checker fails otherwise.

@@ -12,7 +14,6 @@ import (

"github.com/onflow/flow-go/fvm/environment"
"github.com/onflow/flow-go/fvm/errors"
"github.com/onflow/flow-go/fvm/evm"
Copy link
Contributor

Choose a reason for hiding this comment

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

This didn't need to move.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah damn goland

@janezpodhostnik
Copy link
Contributor

Thanks for finding this!

@turbolent
Copy link
Member

turbolent commented Nov 28, 2023

Thank you for the fix. I'm not familiar with the FVM internals, what was broken? Can you please add a test that fails without the fix? That prevents future regressions

@sideninja
Copy link
Member Author

Thank you for the fix. I'm not familiar with the FVM internals, what was broken? Can you please add a test that fails without the fix? That prevents future regressions

I'm just doing that yeah.

@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (10b0fcb) 56.23% compared to head (c7ffbbb) 57.50%.

Files Patch % Lines
fvm/transactionInvoker.go 76.92% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5069      +/-   ##
==========================================
+ Coverage   56.23%   57.50%   +1.27%     
==========================================
  Files         977      786     -191     
  Lines       91105    74606   -16499     
==========================================
- Hits        51232    42904    -8328     
+ Misses      36066    28478    -7588     
+ Partials     3807     3224     -583     
Flag Coverage Δ
unittests 57.50% <76.92%> (+1.27%) ⬆️

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.

@sideninja sideninja added this pull request to the merge queue Nov 28, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 28, 2023
@sideninja sideninja added this pull request to the merge queue Nov 29, 2023
Merged via the queue into master with commit 8a10cbb Nov 29, 2023
54 checks passed
@sideninja sideninja deleted the gregor/evm/fix-setup branch November 29, 2023 11:12
@sideninja sideninja mentioned this pull request Jan 10, 2024
This pull request was closed.
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.

[EVM] Transaction execution fails
5 participants