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

chore(fw): add docstrings to all opcodes in opcodes enum #424

Merged
merged 8 commits into from
Feb 27, 2024

Conversation

ThreeHrSleep
Copy link
Contributor

@ThreeHrSleep ThreeHrSleep commented Feb 4, 2024

πŸ—’οΈ Description

Added docstrings to most of the opcodes in the Opcodes Enum.
Left TSTORE,TLOAD,SENDALL,RJUMPI,RJUMPV incomplete due to confusion

πŸ”— Related Issues

#383

βœ… Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Looks amazing! Thanks for this. I added some minor comments before we can merge.

src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
@ThreeHrSleep
Copy link
Contributor Author

ThreeHrSleep commented Feb 6, 2024

Thanks for reviewing!
Implemented the suggested changes.
Please lmk if there's anything else that needs to be modified :)

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

This is looking really great, thanks a lot for adding this! πŸ’―

In general, tox is really helpful to make sure no errors have crept in. The framework-specific checks (tox -e framework) and the tests checks (tox -e tests) caught the error in the PUSH opcode values (see below). Docs here: https://ethereum.github.io/execution-spec-tests/v2.1.0/writing_tests/verifying_changes/

You will probably come across all sort of whitespace / max-line-length errors when you try to run tox...

These could be either handled in your script, by wrapping the new docstring lines at the relevant column (99) or by using VS Code and plugins. In addition to the repo's VS Code setup you might need a plugin like https://stkb.github.io/Rewrap/ to help with bullet point formatting/line-wrapping.

If you feel it's worth getting VS code setup to handle these, see https://ethereum.github.io/execution-spec-tests/v2.1.0/getting_started/setup_vs_code/.

If not (or if your having other issues), feel free to ping me, and I'm happy to help with formatting to get this over the finish line! πŸ™‚

src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
@ThreeHrSleep
Copy link
Contributor Author

Thanks a lot for pointing out the problems @danceratopz !! ❀️
I'll try to get the VS Code setup done,handle the whitespaces & fix the issue with PUSH
will definitely let you know if I get stuck somewhere :)

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Some more comments thanks!

I think you should be able to just batch-commit all the comments directly from here.

src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
@ThreeHrSleep
Copy link
Contributor Author

Thanks !
and sorry for making so many mistakes in the PR πŸ˜…

@marioevz
Copy link
Member

Hey @danceratopz I think we are unable to merge unless you approve the requested changes, if you'd like to give this another look πŸ‘

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Sorry for the hold up! Looking good, thank you once again!

I replaced the unnecessary UTF8 characters in 6864f5d (’ -> '). Otherwise, a few fixes/suggestions below!

src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Outdated Show resolved Hide resolved
Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Couple of acknowledgements.

Any thoughts @marioevz?

docs/CHANGELOG.md Outdated Show resolved Hide resolved
src/ethereum_test_tools/vm/opcode.py Show resolved Hide resolved
@spencer-tb spencer-tb added type:chore Type: Chore scope:fw Scope: Framework (evm|tools|forks|pytest) labels Feb 27, 2024
@spencer-tb spencer-tb changed the title Add docstrings to all opcodes in the Opcodes Enum chore(fw): add docstrings to all opcodes in opcodes enum Feb 27, 2024
@danceratopz
Copy link
Member

Hey @danceratopz I think we are unable to merge unless you approve the requested changes, if you'd like to give this another look πŸ‘

Hey @marioevz, @spencer-tb's doing a last pass πŸ˜… and will give some more input sooon, but I've approved the requested changes to avoid blocking!

@spencer-tb
Copy link
Collaborator

I added some additional tweaks in: 035821f

  • General clean-up, removing .'s and newlines etc,
  • Fixed forkname mismatches,
  • Clean up of swap and push opcodes,
  • Gas formatting clean up (the markdown wasn't rendering nicely in vscode),
  • Ordered the opcodes,
  • Removed SENDALL as we don't use it anywhere,
  • Tox fixes.

Copy link
Collaborator

@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.

Cheeky approve after my commit πŸ’₯

@danceratopz
Copy link
Member

Amazing. The most commented PR in the history of execution-spec-tests! Thanks again @ThreeHrSleep!

@danceratopz danceratopz merged commit bc1a154 into ethereum:main Feb 27, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:fw Scope: Framework (evm|tools|forks|pytest) type:chore Type: Chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants