-
Notifications
You must be signed in to change notification settings - Fork 185
feat: update both optimistic submission methods to use adjustment data v3 #860
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: develop
Are you sure you want to change the base?
Conversation
|
@claude review |
|
Claude finished @metachris's task in 4m 46s —— View job Code Review Complete
SummaryThis PR simplifies the bid adjustment code by consolidating Key findings:
I've posted a detailed summary comment and 3 inline comments on specific code locations. |
| .executed_tx_infos | ||
| .last() | ||
| .map(|tx_info| tx_info.space_used.gas) | ||
| .expect("payout transaction must exist"); |
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.
Potential panic in edge cases
This expect() is now evaluated unconditionally, whereas previously the bid adjustment data was constructed lazily inside the iterator. If executed_tx_infos is ever empty (e.g., in testing or edge cases), this will panic.
Consider deferring this calculation to only when bid_adjustment_state_proofs is non-empty, or using unwrap_or(0) if a default is acceptable:
| .expect("payout transaction must exist"); | |
| .expect("payout transaction must exist"); |
Alternatively, you could compute this lazily:
let placeholder_gas_used = LazyCell::new(|| {
self.executed_tx_infos
.last()
.map(|tx_info| tx_info.space_used.gas)
.expect("payout transaction must exist")
});This preserves the original behavior where the expect would only be evaluated when bid adjustments are actually created.
| /// adjusting payments to contract addresses. | ||
| pub placeholder_receipt_proof: Vec<Bytes>, | ||
| /// New in V2: Logs bloom accrued until but not including the last (payment) transaction. | ||
| pub el_placeholder_receipt_proof: Vec<Bytes>, |
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.
Field naming change
The field was renamed from placeholder_receipt_proof to el_placeholder_receipt_proof. While this naming is more consistent with the EL/CL prefix convention used elsewhere in V3, ensure this matches the exact field name expected by the relay API. The UltraSound docs should be the source of truth for field names.
The el_ prefix makes sense as this is an execution layer proof (vs consensus layer), similar to el_transactions_root and cl_placeholder_transaction_proof.
| } | ||
| } | ||
| /// Gas used by the placeholder (payout) transaction. Required for V3 to relax the | ||
| /// gas_limit == gas_used requirement. |
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.
New V3 field - verify relay compatibility
The placeholder_gas_used field is the key addition for V3 format. This is correctly documented as relaxing the gas_limit == gas_used requirement.
Please verify:
- This field name matches exactly what the relay expects
- The relay version you're targeting supports this field (ensure relay is updated to accept V3 format if not already)
Code Review SummaryOverviewThis PR simplifies the bid adjustment code by unifying optimistic v1 and v3 submissions to use a single What Changed
Strengths
Items to AddressI've left inline comments on the following:
Questions
VerdictThe changes look good overall. The simplification is well-motivated and the implementation is clean. Just verify the inline comments are addressed and confirm relay compatibility before merging. |
📝 Summary
Unify optimistic v1 and v3 submissions to use BidAdjustmentDataV3 format, removing BidAdjustmentDataV1 and intermediate conversion methods.
💡 Motivation and Context
For payout transactions, gas_used may differ from gas_limit. BidAdjustmentDataV3(https://docs.ultrasound.money/builders/optimistic-v3) format relaxes the gas_limit == gas_used assumption by explicitly providing placeholder_gas_used, allowing the relay to use the actual gas consumed.
In addition to optimistic v3 submissions, there already is a support for using the BidAdjustmentDataV3 with optimistic v1 submissions. Currently in rbuilder codebase, v1 submissions used BidAdjustmentDataV1 while v3 submissions used BidAdjustmentDataV2. Changing those submission types to use v3 data allows relays to use the actual gas consumed, preventing adjustment failures when gas_used differs from gas_limit, and will increase builder adjustment revenues.
Therefore we can simplify the codebase by:
✅ I have completed the following steps:
make lintmake test