Skip to content

Conversation

LouisTsai-Csie
Copy link
Collaborator

@LouisTsai-Csie LouisTsai-Csie commented Jul 4, 2025

πŸ—’οΈ Description

Add different gas cost test cases for CLZ opcode

πŸ”— Related Issues or PRs

Issue #1795

βœ… Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).

@LouisTsai-Csie LouisTsai-Csie self-assigned this Jul 4, 2025
@LouisTsai-Csie LouisTsai-Csie added fork:osaka Osaka hardfork type:test Type: Add/refactor fw unit tests; no fw or el client test case changes labels Jul 4, 2025
Copy link
Contributor

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

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

Thanks! Just some small changes. I wondered if it was worth combining this test with test_clz_gas initially, but happy to have this as its own case.

Could we also rename test_clz_gas to test_clz_gas_cost?

@LouisTsai-Csie
Copy link
Collaborator Author

Thanks! Just some small changes. I wondered if it was worth combining this test with test_clz_gas initially, but happy to have this as its own case.

I explored this idea before implementation, but based on my understanding, the CodeGasMeasure object is responsible for verifying the gas usage specified in its code field. Therefore, we can’t use it to test cases involving insufficient or excessive gas.

Copy link
Contributor

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

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

Nice! LGTM :)

@spencer-tb spencer-tb added scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature and removed type:test Type: Add/refactor fw unit tests; no fw or el client test case changes labels Jul 4, 2025
@LouisTsai-Csie LouisTsai-Csie force-pushed the clz-gas-cost branch 2 times, most recently from b9ca764 to 1313b65 Compare July 7, 2025 12:02
@spencer-tb
Copy link
Contributor

Thanks for updating and the comment @jochem-brouwer!

@spencer-tb spencer-tb merged commit e88a211 into ethereum:main Jul 7, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork:osaka Osaka hardfork scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants