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

feat: l2 gas cli argument #287

Merged
merged 1 commit into from
May 22, 2024
Merged

Conversation

zeapoz
Copy link
Contributor

@zeapoz zeapoz commented May 22, 2024

What πŸ’»

Introduces a new CLI argument for specifying the L2 gas price.

Why βœ‹

This is the first step towards dynamic gas parameter fetching. With this PR it's now possible to avoid having to modify the source code when one wants to adjust the L2 gas price.

Evidence πŸ“·

New output of cargo run --release -- --help:
image

Notes πŸ“

As the hardcoded L2 gas value can now be overridden it's possible to start a running node with a custom set gas value and run the e2e tests on that node, resulting in a failure of the zks_estimateFee test. Maybe we should consider updating e2e-tests/README.md to reflect this change.

@zeapoz zeapoz requested a review from a team as a code owner May 22, 2024 08:41
@MexicanAce
Copy link
Collaborator

As the hardcoded L2 gas value can now be overridden it's possible to start a running node with a custom set gas value and run the e2e tests on that node, resulting in a failure of the zks_estimateFee test. Maybe we should consider updating e2e-tests/README.md to reflect this change.

I feel like the e2e test like zks_estimateFee should be built to pass against a node with the default configuration/values, so this is fine πŸ‘

Copy link
Collaborator

@MexicanAce MexicanAce left a comment

Choose a reason for hiding this comment

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

LGTM

@MexicanAce MexicanAce merged commit e009070 into matter-labs:main May 22, 2024
10 checks passed
@zeapoz zeapoz deleted the feat/gas-cli-argument branch May 22, 2024 10:08
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.

None yet

2 participants