-
Notifications
You must be signed in to change notification settings - Fork 106
feat(flashblocks): add UnifiedReceiptBuilder for seamless deposit receipt handling #345
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
base: main
Are you sure you want to change the base?
Conversation
…eipt handling Add a new UnifiedReceiptBuilder that handles both deposit and non-deposit transactions seamlessly without requiring error handling at the call site. The builder wraps OpRethReceiptBuilder and automatically handles the deposit receipt case internally, eliminating the need for callers to implement the try-catch pattern typically required when using build_receipt followed by build_deposit_receipt. Changes: - Add receipt_builder module with UnifiedReceiptBuilder - Update PendingStateBuilder to use UnifiedReceiptBuilder - Simplify execute_with_evm method by removing manual deposit handling - Remove receipt_builder parameter from PendingStateBuilder::new() Closes base#309
🟡 Heimdall Review Status
|
Removed wrapper around OpRethReceiptBuilder and implemented direct receipt building logic. This fixes type mismatches between OpTxEnvelope and OpTransactionSigned that prevented compilation. - Use direct OpTxType matching instead of build_receipt/build_deposit_receipt pattern - Accept ExecutionResult<E::HaltReason> for proper generic type handling - Remove unused alloy-op-evm dependency
| input, | ||
| &mut self.l1_block_info, | ||
| ) | ||
| .unwrap() |
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.
Can we properly bubble up an error here instead of using .unwrap()? We should have fixed this previously but if we're modifying this, let's cleanup past issues.
| self.cumulative_gas_used, | ||
| self.pending_block.timestamp, | ||
| ) | ||
| .map_err(|_| ExecutionError::DepositAccountLoad)?; |
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.
Let's implement From<ReceiptBuildError> for ExecutionError. This should let us remove the .map_err here and just use the ? operator.
So this would just become:
let receipt = self
.receipt_builder
.build(
&mut self.evm,
&transaction,
result,
&state,
self.cumulative_gas_used,
self.pending_block.timestamp,
)?;| evm: &mut E, | ||
| transaction: &Recovered<OpTxEnvelope>, | ||
| result: ExecutionResult<E::HaltReason>, | ||
| _state: &EvmState, |
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.
Can we remove this argument if it's unused?
| #[test] | ||
| fn test_unified_receipt_builder_creation() { | ||
| let chain_spec = Arc::new(OpChainSpecBuilder::base_mainnet().build()); | ||
| let builder = UnifiedReceiptBuilder::new(chain_spec); | ||
| assert!(std::mem::size_of_val(&builder) > 0); | ||
| } |
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.
Can we add some tests that cover the logic of the UnifiedReceiptBuilder? This test is effectively a no-op
refcell
left a comment
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.
A few changes + nits
Summary
Introduces
UnifiedReceiptBuilder- a wrapper that handles both deposit and non-deposit transaction receipts seamlessly, eliminating the try-catch pattern required byOpReceiptBuilder.Changes
receipt_builder.rswithUnifiedReceiptBuilder<C>structbuild()method handles both tx types internallystate_builder.rs- removed ~40 lines of manual deposit handlingalloy-op-evmdependencyCloses #309