-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: rename BundleStateWithReceipts
to BlockExecutionOutcome
#8730
Conversation
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.
hmm, this is slightly more nuanced because a ton of variables still refer to this as state
and other types also refer to this as state.
I very much prefer this new name because it's accurate.
but this clashes with how we refer to it, referring to this as state is even more confusing...
not sure how to proceed...
crates/blockchain-tree/src/bundle.rs
Outdated
@@ -18,7 +18,7 @@ pub struct BundleStateDataRef<'a> { | |||
} | |||
|
|||
impl<'a> BundleStateDataProvider for BundleStateDataRef<'a> { | |||
fn state(&self) -> &BundleStateWithReceipts { | |||
fn state(&self) -> &BlockExecutionOutcome { |
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.
hmm, while naming is an improvement, this does not fit if we keep using it like 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.
Yes I thought we were saving it for another PR as there were a lot of changes to make but it's true that it doesn't make much sense to merge like that.
So I tried changing the names around BlockExecutionOutcome
for overall consistency, hope I didn't forget any.
Let me know what you think about 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.
I like this a lot more than Bundlestate,
after looking at this, I would even consider dropping the Block
prefix and just do ExecutionOutput
this way it's not tied to Block
because it could this can represent multiple blocks.
but I'd like more feedback here @Rjected @gakonst @shekhirin @rkrasiuk @rakita
|
I like |
|
ExecutionOutput it is then |
actually, can we roll this back to |
ee30a63
to
1bd67d3
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.
ty 🫡
This PR:
BundleStateWithReceipts
toBlockExecutionOutcome
with_receipts
andwith_requests
Should close #8727.