Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

Implement ExecutionState::EndTx #312

Merged
merged 3 commits into from
Mar 14, 2022

Conversation

han0110
Copy link
Contributor

@han0110 han0110 commented Jan 30, 2022

This PR aims to implement ExecutionState::EndTx. It also does:

It depends on #292.

@han0110 han0110 requested a review from ed255 January 30, 2022 10:32
@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jan 30, 2022
Copy link
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Some nits and suggestions.

Other than that. AMAZING work mate! <3

/// Return the index and context of the current call (the last call in the
/// call stack).
fn call_index(&self) -> usize {
let (index, _) = self.call_stack.last().expect("call_stack is empty");
let index = self.call_stack.last().expect("call_stack is empty");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not returning Option<usize> instead of unwrapping here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, invalid geth trace might still be to cause this to panic. I try with returning Result, otherwise we need to call ok_or a lot in other places. Is this a acceptable solution?

let index = self
.call_stack
.get(self.call_stack.len() - 2)
.expect("call_stack only has 1 call");
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Comment on lines 567 to 584
fn reverse(&self) -> Self {
let mut rev = self.clone();
swap(&mut rev.value, &mut rev.value_prev);
rev
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing that you always impl reverse on the same way. You can directly impl this function in the trait definition.

On that way, there's only one place with this code and it's always consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently each struct has their own value and value_prev type, I can't think a way to make this into trait's common function, would you like to give me a example to follow and improve this?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can define associate types for traits. But don't want to block on this. So if it's not something that is obvious it might very well be the case that I'm missing something! :)

Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

I've done a first review round. I still haven't completed reviewed the changes under the zkevm-circuits subcrate.

Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me! Great work!
I've added a few notes mainly to discuss decisions and better understand the changes applied in this PR.

@CPerezz
Copy link
Contributor

CPerezz commented Mar 2, 2022

I'll wait to re-review once @han0110 you re-request it if you want! :)

@han0110 han0110 requested review from CPerezz and ed255 March 4, 2022 06:03
@han0110 han0110 force-pushed the feature/end-tx branch 2 times, most recently from fdc27e5 to 3834dc4 Compare March 11, 2022 16:50
Copy link
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM! Great work!!!

@han0110 han0110 merged commit f2799bc into privacy-scaling-explorations:main Mar 14, 2022
@han0110 han0110 deleted the feature/end-tx branch March 14, 2022 10:56
@han0110 han0110 mentioned this pull request Mar 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-opcode Type: opcode-related and focused PR/Issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants