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(levm): implement Transaction Report #930

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

JereSalo
Copy link
Contributor

@JereSalo JereSalo commented Oct 22, 2024

Motivation

Implement interface transaction report and make call work with it.

Description

  • Implement interface transaction report and fixed problems with users of that interface.
  • Refactor of returndata. Created sub_return_data for subcontext and left return_data for current context.
  • Modify opcodes RETURNDATASIZE and RETURNDATACOPY to use sub_return_data
  • Changes in generic_call()'s logic

Out of scope but implemented:

  • Fix revm comparison tests display (levm results weren't being displayed before)

Closes #926
Closes #923 - partially addressed, there are still changes to make to generic_call()
Closes #922
Closes #938 - Even though this issue is out of scope, the fix was only a one line change.

@JereSalo JereSalo added the levm Lambda EVM implementation label Oct 22, 2024
@JereSalo JereSalo self-assigned this Oct 22, 2024
@JereSalo JereSalo changed the title Implement Transaction Report feat(levm): Implement Transaction Report Oct 22, 2024
@JereSalo JereSalo changed the title feat(levm): Implement Transaction Report feat(levm): implement Transaction Report Oct 22, 2024
@JereSalo JereSalo marked this pull request as ready for review October 22, 2024 19:34
@JereSalo JereSalo requested a review from a team as a code owner October 22, 2024 19:34
&mut self,
current_call_frame: &mut CallFrame,
) -> Result<OpcodeSuccess, VMError> {
pub fn execute(&mut self, current_call_frame: &mut CallFrame) -> TransactionReport {
Copy link
Member

@juanimedone juanimedone Oct 22, 2024

Choose a reason for hiding this comment

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

Shouldn't transact return a TransactionReport instead? execute is supposed to be called only from inside of VM.

Copy link
Member

Choose a reason for hiding this comment

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

I've just seen that transact is indeed returning a TransactionReport, but do you think execute should be returning the same?

Copy link
Contributor Author

@JereSalo JereSalo Oct 22, 2024

Choose a reason for hiding this comment

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

Yeah it may not be the most accurate representation but it makes it more simple to use the same, the attributes we need in both scenarios are almost identical.

Maybe the other option would be to have another struct for that purpose (ContextResult?) and then on transact we should execute the VM and generate the TransactionReport based on the ContextResult returned by execute and the final state of the VM. It doesn't sound like a bad idea.

Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
levm Lambda EVM implementation
Projects
None yet
2 participants