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: store_byte memory method #329

Merged
merged 7 commits into from
Sep 25, 2023
Merged

feat: store_byte memory method #329

merged 7 commits into from
Sep 25, 2023

Conversation

enitrat
Copy link
Collaborator

@enitrat enitrat commented Sep 8, 2023

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Resolves: #325

What is the new behavior?

  • Adds a store_byte method to memory for MSTORE8 opcode
  • Fixes total gas accounting in python script.

Does this introduce a breaking change?

  • Yes
  • No

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

Snapshot Comparison Report:

�[92mIMPROVEMENTS�[0m
�[92mevm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_last_uint8_offset_31 635534 --> 462222 | -27.27 %�[0m
�[92mevm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_last_uint8_offset_63 928364 --> 755052 | -18.67 %�[0m
�[92mevm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_uint8_offset_31 635534 --> 462222 | -27.27 %�[0m
�[92mevm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_uint8_offset_31_then_uint8_offset_30 1022258 --> 867314 | -15.16 %�[0m

�[91mWORSENED�[0m
�[91mevm::tests::test_instructions::test_memory_operations::test_exec_mstore8_should_store_uint8_offset_30 635534 --> 653902 2.89 %�[0m
�[92mOverall gas change: performance improvement, gas consumption-0.24 %�[0m

Copy link
Member

@ClementWalter ClementWalter left a comment

Choose a reason for hiding this comment

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

see comments

crates/evm/src/memory.cairo Outdated Show resolved Hide resolved
crates/evm/src/memory.cairo Outdated Show resolved Hide resolved
scripts/compare_snapshot.py Outdated Show resolved Hide resolved
@enitrat enitrat force-pushed the issue-325 branch 3 times, most recently from b0dcf87 to 0c31bb1 Compare September 14, 2023 06:41
@enitrat
Copy link
Collaborator Author

enitrat commented Sep 14, 2023

Addressed comments

Eikix
Eikix previously approved these changes Sep 18, 2023
Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

looks good to me overall

crates/evm/src/memory.cairo Outdated Show resolved Hide resolved
Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

lgtm

@Eikix Eikix dismissed ClementWalter’s stale review September 25, 2023 03:37

approved after changes

@Eikix Eikix added this pull request to the merge queue Sep 25, 2023
Merged via the queue into main with commit 1440c69 Sep 25, 2023
2 checks passed
@Eikix Eikix deleted the issue-325 branch September 25, 2023 03:37
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.

dev: add store_byte method to memory
4 participants