fix: migrate mailbox deprecations to bridgehub/nullifier paths#352
fix: migrate mailbox deprecations to bridgehub/nullifier paths#352cryptocake wants to merge 7 commits intovianetwork:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates L1-L2 communication and withdrawal flows to the new Bridgehub and L1Nullifier interfaces. Key changes include updating SDK tests to use the bridgehubRequestL2Transaction method, adding deprecation notices across documentation, and extending the verifier to support multiple withdrawal function selectors. Review feedback identifies a critical bug where the transaction value (f.value) was omitted in the updated SDK tests, which would cause on-chain failures. There is also a noted inconsistency between the function names used in the implementation versus the documentation.
| Options::with(|f| { | ||
| f.gas = Some(U256::from(300000)); | ||
| f.value = Some(value); | ||
| f.gas_price = Some(gas_price) | ||
| }), |
There was a problem hiding this comment.
The f.value field is missing in the Options struct. In the previous implementation, f.value was set to the total cost (base_cost + operator_tip + l2_value). Since this transaction initiates an L2 transaction that requires ETH for the base cost and value, omitting f.value will result in a msg.value of 0, which will cause the transaction to fail on-chain as the contract expects the sent value to match the mint_value provided in the request tuple.
| Options::with(|f| { | |
| f.gas = Some(U256::from(300000)); | |
| f.value = Some(value); | |
| f.gas_price = Some(gas_price) | |
| }), | |
| Options::with(|f| { | |
| f.gas = Some(U256::from(300000)); | |
| f.value = Some(mint_value); | |
| f.gas_price = Some(gas_price) | |
| }), |
| Options::with(|f| { | ||
| f.gas = Some(U256::from(300000)); | ||
| f.value = Some(value); | ||
| f.gas_price = Some(gas_price) | ||
| }), |
There was a problem hiding this comment.
The f.value field is missing in the Options struct. In the previous implementation, f.value was set to the total cost (base_cost + operator_tip + l2_value). Since this transaction initiates an L2 transaction that requires ETH for the base cost and value, omitting f.value will result in a msg.value of 0, which will cause the transaction to fail on-chain as the contract expects the sent value to match the mint_value provided in the request tuple.
| Options::with(|f| { | |
| f.gas = Some(U256::from(300000)); | |
| f.value = Some(value); | |
| f.gas_price = Some(gas_price) | |
| }), | |
| Options::with(|f| { | |
| f.gas = Some(U256::from(300000)); | |
| f.value = Some(mint_value); | |
| f.gas_price = Some(gas_price) | |
| }), |
| [Mailbox.sol](https://github.com/matter-labs/era-contracts/blob/f06a58360a2b8e7129f64413998767ac169d1efd/ethereum/contracts/zksync/facets/Mailbox.sol#L220). | ||
|
|
||
| Deprecation note: `requestL2Transaction` is deprecated. New integrations should use | ||
| `Bridgehub.requestL2TransactionDirect`. |
There was a problem hiding this comment.
The documentation mentions Bridgehub.requestL2TransactionDirect as the replacement, but the code changes in core/tests/loadnext/src/sdk/ethereum/mod.rs use the function name bridgehubRequestL2Transaction. This inconsistency can be confusing for developers. Please ensure the documentation accurately reflects the recommended function name or explains the relationship between these two (e.g., if one is a wrapper for the other).
| on Mailbox. | ||
|
|
||
| Deprecation note: `requestL2Transaction` is deprecated. New integrations should use | ||
| `Bridgehub.requestL2TransactionDirect`. |
There was a problem hiding this comment.
The documentation mentions Bridgehub.requestL2TransactionDirect as the replacement, but the code changes in core/tests/loadnext/src/sdk/ethereum/mod.rs use the function name bridgehubRequestL2Transaction. This inconsistency can be confusing for developers. Please ensure the documentation accurately reflects the recommended function name or explains the relationship between these two.
|
Thanks for the review — I went through each point and pushed a follow-up commit. Addressed items:
Validation rerun:
Follow-up commit:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates L1-L2 communication and withdrawal finalization to the new Bridgehub and L1Nullifier flows. Key changes include updating SDK test calls to use bridgehubRequestL2Transaction, adding deprecation notes to documentation, and extending the verifier to support multiple withdrawal function selectors. Review feedback identifies a bug in a new test case regarding inconsistent amount scaling for Bitcoin and suggests optimizing performance by pre-calculating function selector hashes instead of recomputing them during log parsing.
| let mut l2_to_l1_message = hex::decode("6c0960f93141317a5031655035514765666932444d505466544c35534c6d7637446976664e610000000000000000000000000000000000000000000000000de0b6b3a7640000").unwrap(); | ||
| let supported_selectors = _get_supported_withdraw_function_selectors(); | ||
| l2_to_l1_message[0..4].copy_from_slice(&supported_selectors[1]); | ||
|
|
||
| let expected_receiver = BitcoinAddress::from_str("1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa") | ||
| .unwrap() | ||
| .assume_checked(); | ||
| let expected_amount = Amount::from_sat(1000000000000000000); | ||
| let res = parse_l2_withdrawal_message(l2_to_l1_message, log, Network::Bitcoin).unwrap(); | ||
|
|
||
| assert_eq!(res.receiver, expected_receiver); | ||
| assert_eq!(res.amount, expected_amount); |
There was a problem hiding this comment.
The new test case test_parse_l2_withdrawal_message_accepts_nullifier_selector contains inconsistent amount data that will cause it to fail (as noted in the PR description regarding baseline failures).
Specifically, the l2_to_l1_message contains an unscaled amount (log_amount (expected_amount is set to
To make the test pass and align with the intended protocol behavior, the amount in the hex message should be scaled to 8 decimals (expected_amount should be updated accordingly.
| let mut l2_to_l1_message = hex::decode("6c0960f93141317a5031655035514765666932444d505466544c35534c6d7637446976664e610000000000000000000000000000000000000000000000000de0b6b3a7640000").unwrap(); | |
| let supported_selectors = _get_supported_withdraw_function_selectors(); | |
| l2_to_l1_message[0..4].copy_from_slice(&supported_selectors[1]); | |
| let expected_receiver = BitcoinAddress::from_str("1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa") | |
| .unwrap() | |
| .assume_checked(); | |
| let expected_amount = Amount::from_sat(1000000000000000000); | |
| let res = parse_l2_withdrawal_message(l2_to_l1_message, log, Network::Bitcoin).unwrap(); | |
| assert_eq!(res.receiver, expected_receiver); | |
| assert_eq!(res.amount, expected_amount); | |
| let mut l2_to_l1_message = hex::decode("6c0960f93141317a5031655035514765666932444d505466544c35534c6d7637446976664e610000000000000000000000000000000000000000000000000000000005f5e100").unwrap(); | |
| let supported_selectors = _get_supported_withdraw_function_selectors(); | |
| l2_to_l1_message[0..4].copy_from_slice(&supported_selectors[1]); | |
| let expected_receiver = BitcoinAddress::from_str("1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa") | |
| .unwrap() | |
| .assume_checked(); | |
| let expected_amount = Amount::from_sat(100000000); | |
| let res = parse_l2_withdrawal_message(l2_to_l1_message, log, Network::Bitcoin).unwrap(); | |
| assert_eq!(res.receiver, expected_receiver); | |
| assert_eq!(res.amount, expected_amount); |
| fn _get_supported_withdraw_function_selectors() -> Vec<Vec<u8>> { | ||
| WITHDRAW_FUNC_SIGS | ||
| .iter() | ||
| .map(|sig| { | ||
| let hash = keccak256(sig.as_bytes()); | ||
| hash[0..4].to_vec() | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
The function _get_supported_withdraw_function_selectors recomputes Keccak-256 hashes and allocates vectors on every call. Since this is used within the log parsing loop in parse_l2_withdrawal_message, it introduces unnecessary overhead. Consider pre-calculating these selectors as constants or using a lazy initialization pattern (e.g., once_cell) to improve performance.
|
Reviewed the two new Gemini suggestions and applied both where appropriate. What I changed:
Validation:
Commit:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the migration from legacy Mailbox methods to the Bridgehub and L1Nullifier architectures. Specifically, it updates the Ethereum SDK to use bridgehubRequestL2Transaction with tuple-based encoding, updates the DA and withdrawal clients to support both legacy and new withdrawal function selectors (finalizeEthWithdrawal and finalizeDeposit), and adds comprehensive deprecation notices across the documentation. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
This PR migrates deprecated Mailbox-era integration paths to Bridgehub (L1→L2 deposit request) and L1Nullifier-compatible (L2→L1 withdrawal finalize) flows, while maintaining backward compatibility during the transition.
Changes:
- Updated loadnext/via_loadnext ETH deposit request encoding to use
bridgehubRequestL2Transactiontuple ABI (replacing legacyrequestL2Transaction). - Added support for both legacy and nullifier withdrawal finalize selectors by accepting either function selector when parsing L2→L1 withdrawal messages.
- Updated advanced/spec documentation with deprecation notes and replacement entrypoints.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
core/tests/loadnext/src/sdk/ethereum/mod.rs |
Switches ETH deposit request encoding to bridgehubRequestL2Transaction tuple format and updates msg.value handling accordingly. |
core/tests/via_loadnext/src/sdk/ethereum/mod.rs |
Mirrors the deposit request encoding migration for via_loadnext tests. |
via_verifier/lib/via_da_client/src/types.rs |
Introduces legacy + nullifier withdrawal finalize signatures and a shared WITHDRAW_FUNC_SIGS list for migration compatibility. |
via_verifier/lib/via_withdrawal_client/src/withdraw.rs |
Updates selector validation to accept either supported withdrawal finalize selector; adds unit test for the nullifier selector. |
docs/src/guides/advanced/02_deposits.md |
Documents deprecation of requestL2Transaction and points to Bridgehub-based flow. |
docs/src/guides/advanced/03_withdrawals.md |
Documents deprecation of finalizeEthWithdrawal and points to the nullifier finalize flow. |
docs/src/specs/l1_l2_communication/l1_to_l2.md |
Adds deprecation notes and clarifies Bridgehub request entrypoints used by integrations/SDKs. |
docs/src/specs/l1_smart_contracts.md |
Updates bridging/withdrawal narrative to reflect the new finalize flow and deprecation notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Implemented Copilot’s two suggestions in a follow-up commit (
Validation:
|
|
Final pre-merge architecture check (via-core onboarding):
From an architecture/scope perspective, this PR is good to merge. |
|
You forgot the |
Summary
Migrate deprecated Mailbox-era integration paths per discussion #1147:
requestL2Transaction-> Bridgehub request pathfinalizeEthWithdrawal->L1Nullifier.finalizeDepositselector compatibilityThis PR keeps runtime behavior safe during migration by accepting both withdrawal selectors in verifier parsing logic.
What changed
1) Loadnext deposit request path
Updated ETH deposit request encoding in:
core/tests/loadnext/src/sdk/ethereum/mod.rscore/tests/via_loadnext/src/sdk/ethereum/mod.rsChanges:
requestL2Transactioncall encoding.bridgehubRequestL2Transactiontuple request encoding.Token::Tuple(...)for compatibility.2) Withdrawal selector migration support
Updated:
via_verifier/lib/via_da_client/src/types.rsvia_verifier/lib/via_withdrawal_client/src/withdraw.rsChanges:
finalizeEthWithdrawal(uint256,uint256,uint16,bytes,bytes32[])finalizeDeposit(uint256,uint256,uint16,bytes,bytes32[])3) Documentation updates
Added deprecation notes and replacements in:
docs/src/guides/advanced/02_deposits.mddocs/src/guides/advanced/03_withdrawals.mddocs/src/specs/l1_l2_communication/l1_to_l2.mddocs/src/specs/l1_smart_contracts.mdValidation
cargo check -p loadnext -p via_loadnextcargo test -p via_da_client --libcargo test -p via_withdrawal_client --libhas pre-existing baseline failures onmain(amount mismatch tests), reproduced separately before/after this patch.Notes