Implement ExecutionState::EndTx#312
Conversation
github-actions
Bot
added
crate-bus-mapping
han0110
force-pushed
the
feature/end-tx
branch
from
a827410 to
f1fce5b
Compare
CPerezz
left a comment
CPerezz
left a comment
There was a problem hiding this comment.
Some nits and suggestions.
Other than that. AMAZING work mate! <3
| /// 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.
Why not returning Option<usize> instead of unwrapping here?
There was a problem hiding this comment.
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"); |
| 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.
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.
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.
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! :)
han0110
force-pushed
the
feature/end-tx
branch
from
f1fce5b to
733400a
Compare
han0110
force-pushed
the
feature/end-tx
branch
from
733400a to
364aa48
Compare
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
I've done a first review round. I still haven't completed reviewed the changes under the zkevm-circuits subcrate.
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
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! :) |
han0110
force-pushed
the
feature/end-tx
branch
2 times, most recently
from
fdc27e5 to
3834dc4
Compare
3 participants
This PR aims to implement
ExecutionState::EndTx. It also does:ExecutionStatetransition described in EnforceExecutionStatetransition zkevm-specs#110geth-utilsbus-mappingIt depends on #292.