-
Notifications
You must be signed in to change notification settings - Fork 79
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
refactor: rework opcodes definitions and control flow #896
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## next #896 +/- ##
==========================================
+ Coverage 86.87% 86.89% +0.02%
==========================================
Files 127 127
Lines 23695 23714 +19
==========================================
+ Hits 20585 20607 +22
+ Misses 3110 3107 -3
|
ddd6e5f
to
b76083c
Compare
/// Indicates the "outcome" of the execution. | ||
pub outcome: Outcome, | ||
/// The return data. | ||
pub return_data: Bytes, | ||
} | ||
|
||
/// Message status code. | ||
#[must_use] | ||
#[derive(Clone, Debug, Display, PartialEq, Eq)] | ||
pub enum StatusCode { |
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.
Note: I'm planning on renaming this in a follow up PR (to Error), but I don't want that much churn in this PR.
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.
SGTM.
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.
This is becoming almost pretty :)
b76083c
to
d6448db
Compare
My goal here was simplify the logic around "exiting" so that I could reason about it. Specifically, so I could fix some issues with SELFDESTRUCT. We had multiple error and success paths and that made me nervous. In the process, I ended up changing a bunch of other things: 1. Exit early by setting the PC. 2. Go back to propagating the error through `step()` (makes things easier to read). 3. Fix some rust unsafety things. 4. Move _all_ of the instruction specific logic into the instruction definitions. This will let us eventually remove one of these tables. - This means that instructions manage their own PCs. 5. Aggressively inline.
d6448db
to
a441f44
Compare
My goal here was simplify the logic around "exiting" so that I could reason about it. Specifically, so I could fix some issues with SELFDESTRUCT. We had multiple error and success paths and that made me nervous. In the process, I ended up changing a bunch of other things: 1. Exit early by setting the PC. 2. Go back to propagating the error through `step()` (makes things easier to read). 3. Fix some rust unsafety things. 4. Move _all_ of the instruction specific logic into the instruction definitions. This will let us eventually remove one of these tables. - This means that instructions manage their own PCs. 5. Aggressively inline.
My goal here was simplify the logic around "exiting" so that I could reason about it. Specifically, so I could fix some issues with SELFDESTRUCT. We had multiple error and success paths and that made me nervous.
In the process, I ended up changing a bunch of other things:
step()
(makes things easier to read).