-
Notifications
You must be signed in to change notification settings - Fork 840
Implement ExecutionState::EndTx
#312
Implement ExecutionState::EndTx
#312
Conversation
a827410
to
f1fce5b
Compare
There was a problem hiding this 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"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
bus-mapping/src/operation.rs
Outdated
fn reverse(&self) -> Self { | ||
let mut rev = self.clone(); | ||
swap(&mut rev.value, &mut rev.value_prev); | ||
rev | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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! :)
f1fce5b
to
733400a
Compare
733400a
to
364aa48
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
I'll wait to re-review once @han0110 you re-request it if you want! :) |
fdc27e5
to
3834dc4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great work!!!
This PR aims to implement
ExecutionState::EndTx
. It also does:ExecutionState
transition described in EnforceExecutionState
transition zkevm-specs#110geth-utils
bus-mapping
It depends on #292.