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

Verify tx version on execute #162

Merged
merged 3 commits into from Apr 6, 2023

Conversation

zediogoviana
Copy link
Contributor

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
  • Testing
  • Other (please describe):

What is the current behavior?

The current execute method doesn't check if the transaction version being passed is valid or not.
Transaction.version uses a U256 as the type.

Issue Number: #127

What is the new behavior?

This PR adds a transaction version validation similar to what happens in the blockifier.
It also replaces the usage of U256 for the version type in the Transaction struct to be an u8, as specified in the issue.

Existing tests were updated, and new ones were added.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@zediogoviana
Copy link
Contributor Author

Hey @abdelhamidbakhta 👋 This should be ready to be reviewed

Copy link
Contributor

@AbdelStark AbdelStark left a comment

Choose a reason for hiding this comment

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

left some comments

Copy link
Contributor

@AbdelStark AbdelStark left a comment

Choose a reason for hiding this comment

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

lgtm

@EvolveArt
Copy link
Collaborator

hey @zediogoviana, could you add one test within starknet/lib.rs ? Just to make sure it fails correctly when given two non-consecutive nonces.

@zediogoviana
Copy link
Contributor Author

Hey @EvolveArt, I can do that, no problem. However, I'm not understanding how the nonce verification test impacts this PR. Just checking if you aren't referring to PR #129 instead.

@EvolveArt
Copy link
Collaborator

Hey @EvolveArt, I can do that, no problem. However, I'm not understanding how the nonce verification test impacts this PR. Just checking if you aren't referring to PR #129 instead.

Sorry I meant tx version you're right, got confused between PRs

Copy link
Collaborator

@EvolveArt EvolveArt left a comment

Choose a reason for hiding this comment

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

lgtm

@AbdelStark AbdelStark merged commit 3d37413 into keep-starknet-strange:main Apr 6, 2023
2 of 3 checks passed
LucasLvy added a commit that referenced this pull request Apr 6, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants