fix(loadnext): use requestL2TransactionBridgehub in ETH deposit path#349
fix(loadnext): use requestL2TransactionBridgehub in ETH deposit path#349cryptocake wants to merge 1 commit 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 updates the Ethereum provider in the loadnext and via_loadnext test suites by renaming the L2 transaction request method to requestL2TransactionBridgehub and updating related TODO comments. Feedback indicates a potential mismatch with the contract ABI, specifically regarding the function name and the use of positional arguments instead of a single request struct.
| let value = base_cost + operator_tip + l2_value; | ||
| let tx_data = self.client().encode_tx_data( | ||
| "requestL2Transaction", | ||
| "requestL2TransactionBridgehub", |
There was a problem hiding this comment.
The function name requestL2TransactionBridgehub appears to be a typo. In the ZKsync Era Mailbox facet (the main contract), the Bridgehub-compatible method is typically named bridgehubRequestL2Transaction. Furthermore, that method usually expects a single L2TransactionRequestDirect struct as an argument (which includes a chainId), whereas this code continues to pass 7 positional arguments. If the intention was to align with the Bridgehub contract directly, the method name in the provided IBridgehub.json is requestL2TransactionDirect. Please verify the correct function name and signature against the updated ABI.
| let value = base_cost + operator_tip + l2_value; | ||
| let tx_data = self.client().encode_tx_data( | ||
| "requestL2Transaction", | ||
| "requestL2TransactionBridgehub", |
There was a problem hiding this comment.
The function name requestL2TransactionBridgehub appears to be a typo. In the ZKsync Era Mailbox facet (the main contract), the Bridgehub-compatible method is typically named bridgehubRequestL2Transaction. Furthermore, that method usually expects a single L2TransactionRequestDirect struct as an argument (which includes a chainId), whereas this code continues to pass 7 positional arguments. If the intention was to align with the Bridgehub contract directly, the method name in the provided IBridgehub.json is requestL2TransactionDirect. Please verify the correct function name and signature against the updated ABI.
cryptocake
force-pushed
the
fix/issue-347-loadnext-bridgehub
branch
from
abea8bb to
b9a1886
Compare
|
Good catch on the ABI mismatch. Updated this PR to match the hyperchain ABI shape. Changes made:
Patch is pushed on the same branch. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Ethereum SDK used in load tests to use the bridgehubRequestL2Transaction method instead of requestL2Transaction, adjusting the transaction data encoding accordingly. However, the removal of the f.value assignment in the transaction options will cause these transactions to revert on-chain, as no ETH will be sent to cover the required mint_value (base cost, operator tip, and L2 value).
| 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 has been removed from the transaction options. In the ETH deposit path, the transaction must include msg.value to cover the mint_value (which includes the base cost, operator tip, and L2 value). Without setting f.value, the transaction will be sent with 0 ETH, causing it to revert on the L1 contract.
| 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 transaction options. This is required to send the necessary ETH (mint_value) for the L2 transaction request. Removing it will lead to transaction failure as no ETH will be sent to the contract.
| 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) | |
| }), |
|
Closing this PR due to migration-scope and ABI-semantics ambiguity. Will replace with a stricter, minimal, ABI-verified change. |
1 participant
Summary
Migrate the loadnext ETH deposit request path away from deprecated
requestL2TransactiontorequestL2TransactionBridgehubin both loadtest SDK variants.Changes
core/tests/loadnext/src/sdk/ethereum/mod.rsrequestL2Transaction->requestL2TransactionBridgehubcore/tests/via_loadnext/src/sdk/ethereum/mod.rsrequestL2Transaction->requestL2TransactionBridgehubWhy
This keeps the loadnext request path aligned with the Bridgehub-compatible method exposed in current ABI surfaces and reduces continued reliance on deprecated Mailbox method naming.
Validation
requestL2TransactionBridgehubrequestL2Transactionfunction string remains in those two filesLinked issues