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

Add DefaultFVMOptions to verification builder #5439

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Feb 22, 2024

... so that VNs have the same context as ENs.

We should clean this up to avoid similar mistakes in the future. See issue: #5249

... so that it also affects VNs.

We should clean this up to avoid similar mistakes in the future. See issue: #5249
@janezpodhostnik janezpodhostnik requested a review from a team February 22, 2024 18:52
@janezpodhostnik janezpodhostnik self-assigned this Feb 22, 2024
@@ -240,7 +240,6 @@ func DefaultFVMOptions(chainID flow.ChainID, cadenceTracing bool, extensiveTraci
AttachmentsEnabled: chainID != flow.Mainnet,
},
)),
fvm.WithEVMEnabled(true),
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the option above, does that mean that attachments are currently broken on VNs too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, good catch.... yes.

let me address that.

@turbolent
Copy link
Member

Should this target master, or is this really Cadence 1.0-ony?

@janezpodhostnik
Copy link
Contributor Author

I assume we are going to merge the two together soon rigth? Since we are deploying from here I thought I would just put it here. @j1010001

@janezpodhostnik janezpodhostnik changed the title Move WithEVMEnabled higher up Add DefaultFVMOptions to verification builder Feb 22, 2024
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

👍

Let's also fix this on master, but we can already get this in on this feature branch to unblock a new image and update of the previewnet deployment

@janezpodhostnik
Copy link
Contributor Author

master PR #5440

@janezpodhostnik janezpodhostnik merged commit f6d50be into feature/stable-cadence Feb 22, 2024
50 of 51 checks passed
@janezpodhostnik janezpodhostnik deleted the janez/fix-fvm-ctx-evm-setting branch February 22, 2024 20:57
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