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: implement the test suite for 0x52 - MSTORE opcode #289

Closed
wants to merge 3 commits into from

Conversation

Quentash
Copy link
Contributor

@Quentash Quentash commented Sep 6, 2023

I have implemented the tests found at https://github.com/ethereum/tests/blob/develop/src/GeneralStateTestsFiller/VMTests/vmIOandFlowOperations/mstoreFiller.yml which include those from kakarot v0.

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: #247

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

Snapshot Comparison Report:

No changes in gas consumption.

let result = ctx.exec_mstore();

// Then
assert(result.is_ok(), 'should have succeed');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert(result.is_ok(), 'should have succeed');
assert(result.is_ok(), 'should have succeeded');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I have also made this mistake in #263 , do you mind me creating an issue and the related PR to resolve that asaic ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

just modify it in this PR aswell, that'll be fine :)

// Given
let mut ctx = setup_execution_context();

ctx.stack.push(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff);
Copy link
Collaborator

Choose a reason for hiding this comment

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

BoundedInt::<u256>::max()

(don't forget to import integer::BoundedInt


// Then
assert(result.is_ok(), 'should have succeed');
let (stored, _) = ctx.memory.load(0);
Copy link
Collaborator

@enitrat enitrat Sep 6, 2023

Choose a reason for hiding this comment

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

perhaps we should also load(2) to verify that it's 1 aswell?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't load(2) be 0xFF00 since we pushed 0xFF at slot 1 ?
It is a good idea to check the results deeper anyway so I added it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realised that you also did the tests in #290 - in that case, is it fine if you just continue everything from there and close this one? I don't want to duplicate the reviews :)

Don't forget to modify your PR comment by adding Closes #289 to the other PR

@Quentash Quentash mentioned this pull request Sep 6, 2023
9 tasks
@Quentash
Copy link
Contributor Author

Quentash commented Sep 6, 2023

Closing PR to continue in #290 and avoid duplicated review.

@Quentash Quentash closed this Sep 6, 2023
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.

feat: implement the test suite for 0x52 - MSTORE opcode
2 participants