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(evm): add exec_return opcode #328

Merged
merged 3 commits into from
Sep 13, 2023
Merged

feat(evm): add exec_return opcode #328

merged 3 commits into from
Sep 13, 2023

Conversation

0xLucqs
Copy link
Contributor

@0xLucqs 0xLucqs 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: #301

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

crates/evm/src/instructions/system_operations.cairo Outdated Show resolved Hide resolved
/// RETURN
/// # Specification: https://www.evm.codes/#f3?fork=shanghai
fn exec_return(ref self: ExecutionContext) -> Result<(), EVMError> {
let offset = self.stack.pop()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we have a "safe cast" that returns an error if the typecast fails

prefer something like

 let popped = self.stack.pop_n(3)?;
        let dest_offset: u32 = Into::<u256, Result<u32, EVMError>>::into((*popped[0]))?;
        let offset: u32 = Into::<u256, Result<u32, EVMError>>::into((*popped[1]))?;
        let size: u32 = Into::<u256, Result<u32, EVMError>>::into((*popped[2]))?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this looks very bad, a try_into seems to make more sense

Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you think so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it's the purpose of a try_into output an error if it fails

Copy link
Collaborator

@enitrat enitrat Sep 10, 2023

Choose a reason for hiding this comment

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

Yes, I agree, the only problem is that the corelib trait returns an Option and not a Result<T, E>. Now that I think about it it's super weird that it was designed this way

Using a Try Into would not let us propagate an error. Or we would need to use an intermediate conversion from Option to Result.

Copy link
Member

Choose a reason for hiding this comment

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

we could ask them to either transform try_into or implement ok_or which converts an option into a result in Rust

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont want to add one extra step. I would prefer implementing TryIntoErr or something like this

Copy link
Collaborator

Choose a reason for hiding this comment

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

@LucasLvy Can you implement the recommendation pls, this will likely be refactored later anyway

Copy link
Collaborator

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@enitrat enitrat added this pull request to the merge queue Sep 13, 2023
Merged via the queue into kkrt-labs:main with commit 61383b4 Sep 13, 2023
2 checks passed
@enitrat enitrat mentioned this pull request Sep 13, 2023
9 tasks
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: F3 - RETURN opcode
3 participants