[Gas Mechanics] Implement Gas history; Migrate to Arbitrum Gas model #1714
Conversation
WalkthroughThe recent changes across the codebase primarily focus on gas estimation and fee handling for Layer 1 (L1) and Layer 2 (L2) transactions. A Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- tools/walletextension/api/static/favicon.ico
Files selected for processing (11)
- go/common/types.go (2 hunks)
- go/enclave/components/batch_executor.go (7 hunks)
- go/enclave/enclave.go (5 hunks)
- go/enclave/evm/evm_facade.go (4 hunks)
- go/enclave/gas/oracle.go (2 hunks)
- go/host/rpc/clientapi/client_api_eth.go (1 hunks)
- integration/common/constants.go (1 hunks)
- integration/eth2network/eth2_configs.go (1 hunks)
- integration/networktest/tests/helpful/spin_up_local_network_test.go (1 hunks)
- tools/walletextension/README.md (1 hunks)
- tools/walletextension/container_run.sh (1 hunks)
Files skipped from review due to trivial changes (3)
- integration/eth2network/eth2_configs.go
- tools/walletextension/README.md
- tools/walletextension/container_run.sh
Additional comments: 17
go/enclave/gas/oracle.go (2)
17-17: The addition of
EstimateL1CostForMsgto theOracleinterface and its implementation in theoraclestruct is a significant enhancement. Ensure that all parts of the codebase that implement theOracleinterface are updated to include this new method.56-66: The implementation of
EstimateL1CostForMsgappears to correctly calculate the L1 gas cost based on the provided transaction arguments and block information. Ensure that the calculation aligns with the expected gas pricing rules and that the method is tested thoroughly, especially for edge cases and error handling.integration/networktest/tests/helpful/spin_up_local_network_test.go (1)
- 32-35: The removal of
networktest.TestOnlyRunsInIDEfromTestRunLocalNetworksuggests that this test is now intended to run in environments other than just an IDE. Verify that this change aligns with the intended testing strategy and that the test is equipped to run in a continuous integration environment if applicable.integration/common/constants.go (1)
- 86-86: The change in the initialization of the
BaseFeefield to useparams.InitialBaseFee / 100aligns with a more dynamic gas pricing model. Confirm that this new calculation is consistent with the intended gas pricing strategy and that it has been tested across different scenarios to ensure it behaves as expected.go/common/types.go (2)
47-51: The addition of
L2PricedTransactionandL2PricedTransactionstypes introduces a new transaction model that includes pricing information. Ensure that these types are correctly used throughout the codebase and that existing transaction processing logic is updated to handle these new types where necessary.81-87: The
ToTransactionsmethod correctly convertsL2PricedTransactionstotypes.Transactions. Verify that this method is used in all places where a conversion from priced transactions to regular transactions is required, and ensure that the conversion logic is consistent with the system's transaction processing requirements.go/host/rpc/clientapi/client_api_eth.go (1)
- 187-209: The modifications to the
FeeHistorymethod, including the change in the first parameter type from[]bytetostringand the new logic to retrieve and populate theFeeHistoryResult, enhance the method's functionality. Confirm that these changes are tested and that the method behaves as expected when invoked via RPC, especially in terms of error handling and data consistency.go/enclave/evm/evm_facade.go (2)
35-35: The change in the
txsparameter type in theExecuteTransactionsfunction tocommon.L2PricedTransactionsreflects the new transaction model. Ensure that all calls to this function are updated to pass the correct type and that the function's logic correctly handles the new transaction structure.82-83: The addition of the
applyTransactionfunction is a refactoring of the transaction application logic. Verify that this function is correctly implemented and that it is used appropriately within theexecuteTransactionfunction to maintain the modularity and maintainability of the code.go/enclave/components/batch_executor.go (3)
69-71: The
payL1Feesmethod now returnscommon.L2PricedTransactionsinstead ofcommon.L2Transactions. Confirm that this change is reflected throughout the codebase and that all logic that previously depended oncommon.L2Transactionsis updated to handle the newcommon.L2PricedTransactionstype.190-210: The
ComputeBatchmethod has been adjusted to handle the newcommon.L2PricedTransactionstype. Verify that the method correctly processes both priced and free transactions and that the changes do not introduce any regressions or unexpected behavior.419-419: The
processTransactionsmethod now acceptscommon.L2PricedTransactionsinstead of a slice of*common.L2Tx. Confirm that the method's logic is updated to handle the new transaction structure and that it integrates correctly with the rest of the batch execution logic.go/enclave/enclave.go (5)
80-83: The addition of the
gasOraclefield to theenclaveImplstruct is consistent with the PR's objective to improve gas estimation and fee history. Ensure that thegasOracleis properly initialized and used throughout the codebase.275-278: The
gasOracleis correctly initialized and assigned within theNewEnclavefunction. This change aligns with the PR's objective and should enhance the enclave's capabilities regarding gas estimation.689-701: The logic to retrieve the sequence number (
seqNo) in theGetTransactionCountmethod has been updated to handle an optional third parameter. This change seems to allow fetching a batch at a specific height, which could be related to the fee history feature. Ensure that the error handling and the sequence number retrieval logic are robust and correctly implemented.1047-1063: The
EstimateGasmethod now includes additional logic to fetch the latest block and estimate thel1Costusing thegasOracle. It also calculatespublishingGasbased on thel1Costand the base fee of the head batch. This is a significant change that should be carefully tested to ensure it provides accurate gas estimates.1082-1084: The total gas estimate calculation in the
EstimateGasmethod combinespublishingGaswithexecutionGasEstimate. This change seems to be part of the new gas estimation logic and should be verified for correctness and performance implications.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- integration/common/constants.go (1 hunks)
- integration/common/utils.go (1 hunks)
- integration/simulation/simulation.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration/common/constants.go
integration/simulation/simulation.go
Outdated
| deployContractTxData := types.DynamicFeeTx{ | ||
| Nonce: NextNonce(s.ctx, s.RPCHandles, owner), | ||
| Gas: 1025_000_000, | ||
| GasFeeCap: gethcommon.Big1, // This field is used to derive the gas price for dynamic fee transactions. | ||
| Data: contractBytes, | ||
| GasTipCap: gethcommon.Big1, | ||
| } | ||
|
|
||
| signedTx, err := owner.SignTransaction(&deployContractTx) | ||
| deployContractTx := s.RPCHandles.ObscuroWalletRndClient(owner).EstimateGasAndGasPrice(&deployContractTxData) | ||
| signedTx, err := owner.SignTransaction(deployContractTx) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
The change to estimate gas and gas price before deploying the ERC20 contract is a good practice. However, ensure that EstimateGasAndGasPrice is robust against errors and that the error from SignTransaction is handled appropriately.
| panic(err) | ||
| } |
There was a problem hiding this comment.
Replace panic with proper error handling to prevent the simulation from crashing due to recoverable errors.
- panic(err)
+ return fmt.Errorf("error signing transaction for ERC20 deployment: %w", err)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| panic(err) | |
| } | |
| return fmt.Errorf("error signing transaction for ERC20 deployment: %w", err) | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- integration/simulation/transaction_injector.go (5 hunks)
- integration/simulation/transaction_injector_tracker.go (1 hunks)
- tools/hardhatdeployer/obscuro_deployer.go (1 hunks)
Files skipped from review due to trivial changes (1)
- integration/simulation/transaction_injector_tracker.go
Additional comments: 6
tools/hardhatdeployer/obscuro_deployer.go (1)
- 63-73: The variable
txhas been renamed totxData, which is fine. However, the usage offaucetClient.EstimateGasAndGasPrice(txData)is unconventional as it typically returns gas information, not a transaction. Ensure that this function is intended to return a*types.Transactionand is used correctly in this context.integration/simulation/transaction_injector.go (5)
222-225: The
newObscuroTransferTxfunction call withinissueRandomTransfersnow includes an additional argumenttestcommon.HOC. Ensure that the function signature and the call sites have been updated to handle this new argument correctly.291-305: The logic for selecting
fromWalletTokeninissueRandomDepositshas been modified to alternate betweentestcommon.HOCandtestcommon.POC. Confirm that this change aligns with the intended behavior and that thenewObscuroTransferTxfunction call is correctly updated to include thefromWalletTokenargument.338-343: The
newCustomObscuroWithdrawalTxfunction call inissueInvalidL2Txsnow includes an additional argument. Verify that this change is correct and that the function signature has been updated to accommodate the new argument.370-372: The
newObscuroTransferTxmethod has been updated to accept an additional argumentercType. Ensure that this argument is used correctly within the method and that all call sites have been updated accordingly.380-387: The
newTxmethod has been modified to include theercTypeargument, which is used to determine the contract address. Confirm that theercTypeis correctly mapped to the appropriate contract address and that the method's usage is consistent across the codebase.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration/simulation/transaction_injector.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration/simulation/transaction_injector.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- integration/simulation/output_stats.go (2 hunks)
- integration/simulation/stats/stats.go (2 hunks)
- integration/simulation/transaction_injector.go (6 hunks)
- integration/simulation/transaction_injector_tracker.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- integration/simulation/transaction_injector.go
- integration/simulation/transaction_injector_tracker.go
Additional comments: 5
integration/simulation/stats/stats.go (3)
27-31: The addition of
NrNativeTransferTransactionsfield and its mutex protection in theNativeTransfermethod is consistent with the existing pattern of handling stats updates.80-83: The
NativeTransfermethod correctly implements thread safety by using the mutex lock, consistent with the other methods in theStatsstruct.86-88: The
Withdrawalmethod correctly updates theTotalWithdrawalRequestedAmountfield within a mutex lock, ensuring thread safety.integration/simulation/output_stats.go (2)
136-136: The addition of
nrNativeTransferTransactionsto theOutputStatsstruct and its inclusion in theStringmethod output is consistent with the existing structure.155-155: The
Stringmethod has been correctly updated to include the newnrNativeTransferTransactionsfield in its output.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- go/enclave/nodetype/sequencer.go (2 hunks)
- integration/simulation/transaction_injector.go (7 hunks)
- integration/simulation/validate_chain.go (1 hunks)
Files skipped from review due to trivial changes (1)
- integration/simulation/validate_chain.go
Additional comments: 8
integration/simulation/transaction_injector.go (6)
222-222: Ensure that the addition of the
testcommon.HOCargument to thenewObscuroTransferTxfunction call is consistent with the intended use of the method and that all calls to this function have been updated accordingly.291-293: Verify that the alternating logic between
testcommon.HOCandtestcommon.POCtokens in theissueRandomDepositsmethod is correct and aligns with the intended behavior.338-338: Ensure that the addition of the
testcommon.HOCargument to thenewCustomObscuroWithdrawalTxfunction call is consistent with the intended use of the method and that all calls to this function have been updated accordingly.370-372: Verify that the new
ercTypeargument in thenewObscuroTransferTxmethod is being used correctly and that all calls to this method have been updated to include this new argument.377-377: Verify that the new
ercTypeargument in thenewCustomObscuroWithdrawalTxmethod is being used correctly and that all calls to this method have been updated to include this new argument.380-387: Verify that the
ercTypeargument in thenewTxmethod is being used correctly to determine the contract address and that theti.wallets.Tokens[ercType].L2ContractAddressis a valid and expected address for the transaction.go/enclave/nodetype/sequencer.go (2)
197-203: The addition of the
SortableTransactionstype and its associated methods (Len,Swap,Less) for sorting transactions is a good practice for encapsulating sorting logic. Ensure that this new type is used consistently wherever transaction sorting is required.245-245: The use of
sort.Sortwith theSortableTransactionstype in thecreateNewHeadBatchfunction is correct and follows Go's standard sorting pattern.
| /*wg.Go(func() error { | ||
| ti.issueRandomTransfers() | ||
| return nil | ||
| }) | ||
| })*/ //todo - fix |
There was a problem hiding this comment.
The commented-out code block should be removed or accompanied by a detailed comment explaining why it is retained. If this code is meant to be revisited or fixed later, consider adding a more descriptive TODO comment.
| receipt.BlockNumber = big.NewInt(int64(batchHeight)) | ||
| for _, l := range receipt.Logs { | ||
| l.BlockHash = batchHash | ||
| applyTx := func( |
There was a problem hiding this comment.
I don't quite understand the need for this lambda function.
There was a problem hiding this comment.
It's a copy paste of gethcore.ApplyTransaction - can be moved externally.
go/enclave/evm/evm_facade.go
Outdated
| hasL1Cost := l1cost.Cmp(big.NewInt(0)) != 0 | ||
|
|
||
| if hasL1Cost { | ||
| l1Gas.Div(l1cost, header.BaseFee) |
There was a problem hiding this comment.
this needs a comment
| // todo (#1621) - return a non-dummy fee history | ||
| header, err := api.host.DB().GetHeadBatchHeader() | ||
| if err != nil { | ||
| api.logger.Error(fmt.Sprintf("Unable to retrieve header for fee history. Cause: %w", err)) |
There was a problem hiding this comment.
no need for the sprintf
| } | ||
|
|
||
| for _, header := range batches { | ||
| feeHist.GasUsedRatio = append(feeHist.GasUsedRatio, 0.9) //todo @siliev - fix when baseFee is dynamic |
There was a problem hiding this comment.
what's this 0.9?
Should it be a constant with a comment?
| feeHist.GasUsedRatio = append(feeHist.GasUsedRatio, 0.9) //todo @siliev - fix when baseFee is dynamic | ||
| feeHist.BaseFee = append(feeHist.BaseFee, (*hexutil.Big)(header.BaseFee)) | ||
| } | ||
| return feeHist, nil |
There was a problem hiding this comment.
basically, you're returning the struct with the latest batch?
go/enclave/nodetype/sequencer.go
Outdated
| @@ -234,6 +242,8 @@ func (s *sequencer) createNewHeadBatch(l1HeadBlock *common.L1Block, skipBatchIfE | |||
| } | |||
| } | |||
|
|
|||
| sort.Sort(SortableTransactions(transactions)) | |||
There was a problem hiding this comment.
what's up with this?
There was a problem hiding this comment.
Running the simulation it seemed like transactions werent ordered by nonces, but I'm not sure its necessary. The geth mempool is just a bit weird.
There was a problem hiding this comment.
Umm. I'd say this deserves a bit of investigation. Maybe instead of sorting, check that what the mempool produces is sorted by nonce. And if not, we should investigate why not. It's probably some misconfiguration
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration/simulation/transaction_injector.go (7 hunks)
Additional comments: 2
integration/simulation/transaction_injector.go (2)
144-147: The block of code for issuing random transfers is commented out. Based on the previous review comments, this seems intentional for now, but it's important to ensure that this is not forgotten and is uncommented or removed as appropriate in the future.
370-372: The
newObscuroTransferTxmethod has been updated to accept an ERC20 token type parameter. Ensure that all calls to this method have been updated to pass the correct token type.Verification successful
The search results indicate that the
newObscuroTransferTxmethod is indeed being called with the correct number of parameters, which now includes the ERC20 token type parameter. This confirms that the method's usage has been updated in accordance with the changes made to its signature.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to `newObscuroTransferTx` without the new parameter. ast-grep --lang go --pattern $'newObscuroTransferTx($_, $_, $_)'Length of output: 65
Script:
#!/bin/bash # Search for calls to `newObscuroTransferTx` with the correct number of parameters, which should now be four. ast-grep --lang go --pattern $'newObscuroTransferTx($_, $_, $_, $_)'Length of output: 69
Script:
#!/bin/bash # Perform a broader search for the method name `newObscuroTransferTx` to find any usage within the codebase. rg 'newObscuroTransferTx' --goLength of output: 459
Script:
#!/bin/bash # Correct the ripgrep command to search for the method name `newObscuroTransferTx` in `.go` files. rg 'newObscuroTransferTx' -tgoLength of output: 519
| Gas: uint64(1_000_000_000), | ||
| GasPrice: gethcommon.Big1, |
There was a problem hiding this comment.
The gas limit and gas price are hardcoded in the issueRandomValueTransfers method. It's generally a good practice to avoid hardcoding such values, as they may need to change based on network conditions. Consider fetching these values dynamically or configuring them through parameters.
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Using panic for error handling is not recommended in Go, especially for production code. It would be better to handle the error gracefully and log it, or return it to the caller if appropriate.
| ti.logger.Info("Native value transfer transaction injected into L2.", log.TxKey, signedTx.Hash(), "fromAddress", fromWallet.Address(), "toAddress", toWallet.Address()) | ||
|
|
||
| ti.stats.Transfer() | ||
| ti.stats.NativeTransfer() |
There was a problem hiding this comment.
Logging and updating statistics for a transaction should ideally happen after the transaction has been successfully sent. If SendTransaction fails, the logs and statistics would incorrectly reflect a successful transaction.
| Gas: uint64(1_000_000_000), | ||
| GasPrice: gethcommon.Big1, |
There was a problem hiding this comment.
The gas limit and gas price are hardcoded in the newTx method. As mentioned earlier, consider fetching these values dynamically or configuring them through parameters to accommodate network changes.
| GasPrice: gethcommon.Big1, | ||
| Data: data, | ||
| To: ti.wallets.Tokens[testcommon.HOC].L2ContractAddress, | ||
| To: ti.wallets.Tokens[ercType].L2ContractAddress, |
There was a problem hiding this comment.
The To field in the newTx method is set using a map lookup without checking if the key exists. This could lead to a panic if the key is not present. It's safer to check if the key exists in the map before accessing its value.
| @@ -368,24 +367,24 @@ func (ti *TransactionInjector) rndObsWallet() wallet.Wallet { | |||
| return ti.wallets.SimObsWallets[rand.Intn(len(ti.wallets.SimObsWallets))] //nolint:gosec | |||
There was a problem hiding this comment.
The rand.Intn function is used without seeding the random number generator, which could lead to predictable results. It's a good practice to seed the generator using rand.Seed with a variable source like the current time.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (13)
- go/enclave/components/batch_executor.go (7 hunks)
- go/enclave/enclave.go (5 hunks)
- go/enclave/evm/evm_facade.go (4 hunks)
- go/enclave/l2chain/l2_chain.go (1 hunks)
- go/enclave/nodetype/sequencer.go (2 hunks)
- go/host/rpc/clientapi/client_api_eth.go (3 hunks)
- go/obsclient/authclient.go (4 hunks)
- go/responses/types.go (1 hunks)
- go/rpc/client.go (1 hunks)
- integration/common/constants.go (2 hunks)
- integration/common/utils.go (2 hunks)
- integration/simulation/simulation.go (5 hunks)
- integration/simulation/transaction_injector.go (6 hunks)
Files skipped from review due to trivial changes (2)
- go/responses/types.go
- go/rpc/client.go
Additional comments: 39
integration/common/constants.go (2)
- 7-7: The import of
github.com/ethereum/go-ethereum/paramsis added. Ensure that this package is used elsewhere in the file or in the broader codebase to justify its addition.Verification successful
The
paramspackage fromgithub.com/ethereum/go-ethereumis indeed used in various parts of the codebase, as evidenced by the output listing multiple files where it is referenced. This justifies the addition of the import inintegration/common/constants.go.* 86-88: The `BaseFee`, `GasLocalExecutionCapFlag`, and `GasBatchExecutionLimit` are initialized with new values. Confirm that these values are consistent with the intended gas strategy and that they align with the rest of the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the params package is used in other parts of the codebase. rg --type go 'params\.' --files-with-matchesLength of output: 2368
integration/common/utils.go (2)
99-105: The creation of
txDatais a good practice for separating transaction data from the transaction object itself. This allows for more flexible handling of transactions, such as estimating gas before signing.107-107: The
EstimateGasAndGasPricemethod is used to estimate gas and gas price for the transaction. Ensure that this method handles errors gracefully and does not introduce side effects that could affect the transaction signing and sending process.Verification successful
The review of the
EstimateGasAndGasPricemethod inauthclient.goindicates that:
- The method handles errors by providing default values for both gas limit and gas price if the estimation fails.
- It does not throw exceptions or panic, which suggests that it should not introduce side effects that could affect the transaction signing and sending process.
- The method returns a new
LegacyTxwith either the estimated values or the default ones.Based on this information, the method seems to handle errors gracefully and is likely safe for use in the transaction creation process.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of EstimateGasAndGasPrice to ensure it handles errors properly. rg --type go 'EstimateGasAndGasPrice' --files-with-matchesLength of output: 318
Script:
#!/bin/bash # View the implementation of EstimateGasAndGasPrice in authclient.go to check for error handling. rg --type go -C 10 'func \(.*EstimateGasAndGasPrice' go/obsclient/authclient.goLength of output: 743
Script:
#!/bin/bash # View the full implementation of EstimateGasAndGasPrice in authclient.go to check for error handling and side effects. rg --type go -A 30 'func \(ac \*AuthObsClient\) EstimateGasAndGasPrice' go/obsclient/authclient.goLength of output: 1081
go/obsclient/authclient.go (2)
63-65: The
DialWithAuthfunction now assigns the result to a variable before returning. This change is minor and does not affect the logic of the function.79-87: The
GasPricemethod is added to theAuthObsClienttype. Ensure that this method is implemented correctly and that it is consistent with how gas prices are handled elsewhere in the codebase.Verification successful
The code snippets provided from the execution of the script show the usage of the
.GasPrice()method in various contexts within the codebase. Here are the findings:
- In
integration/smartcontract/debug_wallet.go, theGasPricemethod is used as part of aCallMsgstructure, which likely represents a transaction call.- In
integration/ethereummock/node.go, theGasPricemethod is used to set theGasPricefield in aLegacyTxstructure, which is part of transaction preparation.- In
go/obsclient/authclient.go, theGasPricemethod is called to obtain a gas price, and if an error occurs, a default value is set.- In
go/enclave/rpc_transaction.go, theGasPricemethod is used to populate arpcTransactionstructure, which seems to be related to transaction encoding for RPC.- In
go/enclave/components/batch_executor.go, theGasPricemethod is used to determine if a transaction is a "free transaction" by comparing the gas price to zero.From these usages, it appears that the
GasPricemethod is consistently used to retrieve or manipulate the gas price associated with transactions. The implementation inauthclient.goseems to be in line with these patterns, as it provides a way to retrieve the gas price from the RPC client, which is a common operation across the codebase.Based on the information gathered, the
GasPricemethod inAuthObsClientis implemented correctly and is consistent with how gas prices are handled elsewhere in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of the GasPrice method and its consistency with the codebase. rg --type go 'GasPrice' --files-with-matchesLength of output: 1248
Script:
#!/bin/bash # Search for the definition and usage of the GasPrice method to compare with the implementation in authclient.go # Find the definition of GasPrice method in the codebase ast-grep --lang go --pattern $'func ($_ *$_) GasPrice($_) ($_ *big.Int, $_ error) { $$$ }' # Find usages of GasPrice method to see how it's being called rg --type go '\.GasPrice\(' --files-with-matchesLength of output: 316
Script:
#!/bin/bash # Extract the actual code snippets where the GasPrice method is called to compare the context and usage. # List the files where GasPrice method is called files=( "integration/smartcontract/debug_wallet.go" "integration/ethereummock/node.go" "go/obsclient/authclient.go" "go/enclave/rpc_transaction.go" "go/enclave/components/batch_executor.go" ) # Extract the relevant code snippets from each file for file in "${files[@]}"; do echo "File: $file" rg --context 5 '\.GasPrice\(' "$file" echo doneLength of output: 3000
go/enclave/l2chain/l2_chain.go (1)
- 136-136: The
ObsCallAtBlockfunction now subtracts 1 frombatch.Header.GasLimitbefore passing it to theToMessagemethod. Confirm that this change is intentional and correctly implements the desired logic.go/host/rpc/clientapi/client_api_eth.go (2)
10-10: The import of the
paramspackage is necessary for the changes in theGasPricefunction to useparams.InitialBaseFee.76-80: The change to use
params.InitialBaseFeein theGasPricefunction should be verified to ensure it aligns with the intended gas pricing strategy.Verification successful
The usage of
params.InitialBaseFeein theGasPricefunction is consistent with another instance in the codebase, suggesting that the change aligns with the intended gas pricing strategy.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the InitialBaseFee is being used correctly across the codebase. rg --type go 'params.InitialBaseFee'Length of output: 248
go/enclave/evm/evm_facade.go (3)
7-7: The addition of an unnamed import statement (
_ "unsafe") should be verified for its necessity and potential security implications.34-37: The change in the
ExecuteTransactionsfunction signature to acceptcommon.L2PricedTransactionsshould be verified to ensure that all calls to this function have been updated accordingly.Verification successful
The call to
ExecuteTransactionsinbatch_executor.gocorrectly uses thecommon.L2PricedTransactionstype as its first argument, which is consistent with the updated function signature inevm_facade.go.* 83-84: The `applyTransaction` function has been linked using `go:linkname` to an internal function in the `go-ethereum` package. Confirm that this linkage is necessary and that the function is used correctly within the `executeTransaction` function.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the ExecuteTransactions function is called with the correct type. rg --type go 'ExecuteTransactions'Length of output: 220
Script:
#!/bin/bash # Check the call to ExecuteTransactions in batch_executor.go to ensure it uses the correct type. rg --type go -C 10 'evm\.ExecuteTransactions' go/enclave/components/batch_executor.goLength of output: 568
Verification successful
The search results confirm that the
applyTransactionfunction is defined inevm_facade.goand is used within the same file. The linkage to thego-ethereumpackage is established with thego:linknamedirective, and the function is utilized in theexecuteTransactionfunction as intended.Since the
applyTransactionfunction is only used withinevm_facade.goand the linkage is explicitly mentioned, it appears that the function is being used correctly in the context of theexecuteTransactionfunction.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other usages of applyTransaction to ensure it's being used correctly. rg --type go 'applyTransaction'Length of output: 562
integration/simulation/simulation.go (4)
15-15: The addition of the
gethparamsimport is noted. Ensure that this new import is used appropriately throughout the file.213-213: Using
gethparams.Etherfor ether allocation inprefundObscuroAccountsimproves code clarity and ensures that the ether value is correctly represented.239-239: The change to use
EstimateGasAndGasPricefor thedeployContractTxvariable indeployObscuroERC20sis a good practice as it allows for dynamic gas estimation rather than relying on hardcoded values.242-243:
integration/simulation/transaction_injector.go (9)
184-185: The value and gas parameters for transactions in
issueRandomValueTransfershave been modified. Ensure that the new values are within acceptable limits for the intended transaction types.192-193:
195-197:
222-222: The addition of the
ercTypeparameter inissueRandomTransfersmethod allows for more flexible transaction creation based on the ERC20 token type.291-293: The logic to alternate between HOC and POC tokens in
issueRandomDepositsprovides a more balanced simulation of deposits for different token types.338-338: The modification in
issueInvalidL2Txsto create custom obscuro withdrawal transactions is noted. Ensure that these transactions are indeed invalid and handled correctly by the system.367-367:
384-385:
387-387:
go/enclave/nodetype/sequencer.go (2)
196-202: The implementation of
SortableTransactionsand its methodsLen(),Swap(), andLess()appears correct and follows the standard pattern for implementing custom sorting in Go. TheLess()method correctly uses the transaction nonce for comparison, which is standard for ordering transactions in blockchain systems.244-244: The addition of
sort.Sort(SortableTransactions(transactions))ensures that transactions are ordered by nonce before being processed in a batch. This is a critical step for correct transaction processing in blockchain systems, as it ensures that transactions are executed in the correct order. The previous comment by StefanIliev545 indicates that this was added due to observed behavior in simulations. Given the nature of blockchain transaction processing, this sorting step is indeed necessary and should be retained.go/enclave/components/batch_executor.go (7)
- 97-121: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [73-120]
The
payL1Feesfunction has been refactored to usecommon.L2PricedTransactionsinstead ofcommon.L2Transactions. The logic for processing transactions has been updated to categorize transactions as free or not based on their gas price and gas fee cap. However, the actual subtraction of balance for paid transactions is commented out (lines 111-112), which may be intentional as per the previous review comments, but it's important to ensure that this logic is indeed handled elsewhere as indicated by the developer's response.Verification successful
The search results show that the
SubBalancemethod is used inevm_facade.goto subtract balance from the sender and add it to the coinbase. There is also a corresponding refund logic in case of an error during transaction application, where the coinbase's balance is subtracted and the sender's balance is refunded.In
batch_executor.go, the commented-out lines forSubBalanceandAddBalanceare present, and there is an additional occurrence whereAddBalanceandSubBalanceare used, but this seems to be for a different logic related to refunds.Based on the search results, it appears that the balance subtraction logic for transaction fees is indeed handled in
evm_facade.go. The commented-out lines inbatch_executor.goare likely left as a placeholder for future logic (as indicated by the "todo - add refund logic" comment).Given this information, it seems that the balance subtraction is handled elsewhere, as indicated by the developer's response.
- : The balance subtraction logic related to transaction fees is handled in
evm_facade.go, and the commented-out lines inbatch_executor.goare placeholders for future refund logic.* 210-210: The `executor.refundL1Fees` call is commented out on line 210. This might be intentional, as indicated by the previous review comments, but it's important to ensure that the refund logic is implemented elsewhere or if this is a temporary change that needs to be revisited.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the balance subtraction logic is indeed handled elsewhere as indicated by the developer's response. # Search for balance subtraction logic related to transaction fees. rg --type go "SubBalance" --context 5Length of output: 3176
195-201: The creation of
common.L2PricedTransactionsfrom cross-chain transactions (lines 195-201) seems to be a new addition. It's important to ensure that thePublishingCostis correctly set to zero for these transactions and that this aligns with the intended logic for cross-chain transactions.224-224: The
copyBatch.Transactionsis being appended withfreeTransactions.ToTransactions()on line 224. This is a critical change as it affects the transactions included in the batch. It's important to ensure that this change is consistent with the intended transaction processing logic and that no transactions are unintentionally excluded or duplicated.Verification successful
The verification process has been completed, and based on the provided context and the analysis of the
batch_executor.gofile, the following has been determined:
- The
freeTransactionsare identified as transactions with bothGasFeeCapandGasPriceset to zero.- These transactions are processed separately from those with associated costs.
- The
processTransactionsfunction is called without includingfreeTransactions, ensuring they are not processed for fees.- The
freeTransactionsare appended to the batch only after all other transaction processing is complete, avoiding any duplication.Therefore, the change on line 224 in
batch_executor.go, wherecopyBatch.Transactionsis appended withfreeTransactions.ToTransactions(), is consistent with the intended transaction processing logic, and there is no evidence of transactions being unintentionally excluded or duplicated.* 247-247: The logging of new receipts on line 247 is a new addition. While logging is helpful for debugging, it's important to ensure that this does not lead to log flooding, especially in production environments where the number of transactions can be high.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the inclusion of free transactions in the batch is consistent with the intended transaction processing logic. # Search for logic related to transaction inclusion in batches. rg --type go "ToTransactions" --context 5Length of output: 1391
Script:
#!/bin/bash # Search for references to `freeTransactions` within `batch_executor.go` to understand how they are determined and handled. rg --type go "freeTransactions" "go/enclave/components/batch_executor.go" --context 10Length of output: 4851
Script:
#!/bin/bash # Search for the `processTransactions` function and its usage to verify that `freeTransactions` are not included in its processing. rg --type go "processTransactions" "go/enclave/components/batch_executor.go" --context 10Length of output: 1705
- 420-423: The
processTransactionsfunction now acceptscommon.L2PricedTransactionsinstead of a slice of*common.L2Tx. This change aligns with the new transaction types introduced in the PR. It's important to ensure that all calls to this function have been updated accordingly.Verification successful
The output from the script confirms that the
payL1Feesfunction returns two values, both of which are of typecommon.L2PricedTransactions. This matches the expected type for theprocessTransactionsfunction's argument. Therefore, bothtransactionsToProcessandxchainTxsare of the correct typecommon.L2PricedTransactions.Based on the information provided:
- The
xchainTxsvariable is explicitly declared ascommon.L2PricedTransactions.- The
transactionsToProcessvariable is the result of thepayL1Feesfunction, which returnscommon.L2PricedTransactions.Hence, all calls to
processTransactionshave been updated to use the correct typecommon.L2PricedTransactions.* 440-451: The error handling in the `processTransactions` function (lines 440-451) includes appending to `executedTransactions` and `excludedTransactions` based on the presence of a receipt. It's important to ensure that the error handling logic is comprehensive and that all possible error scenarios are accounted for.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to `processTransactions` have been updated to use `common.L2PricedTransactions`. # Search for calls to `processTransactions` and check the argument types. rg --type go "processTransactions" --context 5Length of output: 2158
Script:
#!/bin/bash # Check the declaration and initialization of `transactionsToProcess` and `xchainTxs` to confirm their types. rg --type go -A 2 "transactionsToProcess :=" go/enclave/components/batch_executor.go rg --type go -A 2 "xchainTxs :=" go/enclave/components/batch_executor.goLength of output: 318
Script:
#!/bin/bash # Search for the declaration of `transactionsToProcess` to confirm its type. rg --type go "transactionsToProcess" go/enclave/components/batch_executor.goLength of output: 358
Script:
#!/bin/bash # Check the return type of the `payL1Fees` function to ensure it matches `common.L2PricedTransactions`. rg --type go -A 10 "func \(executor \*batchExecutor\) payL1Fees" go/enclave/components/batch_executor.goLength of output: 732
go/enclave/enclave.go (5)
80-83: The addition of
gasOracle gas.Oracleto theenclaveImplstruct and its initialization in theNewEnclavefunction is consistent with the summary provided. This change is critical for the new gas estimation logic.683-695: The updated
GetTransactionCountmethod now includes logic to handle a third parameter for block tags, which allows fetching the transaction count at a specific block height. Ensure that all calls to this method are updated to handle the new logic correctly.1041-1065: The
EstimateGasmethod now includes additional calculations for gas estimation, which involve interaction with thegasOracleand fetching the head batch for base fee information. Ensure that the new logic is thoroughly tested, especially the division operation for calculatingpublishingGasto avoid division by zero errors.1063-1065: The addition of
1topublishingGasin theEstimateGasmethod is intended to cover edge cases. Verify that this is necessary and consider documenting the specific edge cases it addresses.Verification successful
The context provided from the script output includes a comment explaining the reason for adding
1topublishingGas:// The one additional gas captures the modulo leftover in some edge cases // where BaseFee is bigger than the l1cost.This comment is located directly above the line of code in question, indicating that the edge case handling is indeed documented. The specific edge case mentioned is when the
BaseFeeis larger than thel1cost.Based on this information, the addition of
1topublishingGasis documented, and the comment explains the edge case it addresses.* 1084-1086: The `EstimateGas` method combines L1 cost estimation with L2 gas estimation. Verify that the integration of L1 and L2 estimations is correct and that the method returns accurate results.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the edge case handling in EstimateGas is necessary and documented. rg --type go --multiline 'publishingGas = big.NewInt\(0\).Add\(publishingGas, gethcommon.Big1\)'Length of output: 187
Script:
#!/bin/bash # Look for comments or documentation near the code change that explain the additional gas for edge cases. rg --type go --multiline -C 10 'publishingGas = big.NewInt\(0\).Add\(publishingGas, gethcommon.Big1\)'Length of output: 1439
| func (api *EthereumAPI) FeeHistory(context.Context, string, rpc.BlockNumber, []float64) (*FeeHistoryResult, error) { | ||
| // todo (#1621) - return a non-dummy fee history | ||
| header, err := api.host.DB().GetHeadBatchHeader() | ||
| if err != nil { | ||
| api.logger.Error(fmt.Sprintf("Unable to retrieve header for fee history. Cause: %w", err)) | ||
| return nil, fmt.Errorf("unable to retrieve fee history") | ||
| } | ||
|
|
||
| batches := make([]*common.BatchHeader, 0) | ||
| batches = append(batches, header) | ||
|
|
||
| return &FeeHistoryResult{ | ||
| OldestBlock: (*hexutil.Big)(big.NewInt(0)), | ||
| feeHist := &FeeHistoryResult{ | ||
| OldestBlock: (*hexutil.Big)(header.Number), | ||
| Reward: [][]*hexutil.Big{}, | ||
| BaseFee: []*hexutil.Big{}, | ||
| GasUsedRatio: []float64{}, | ||
| }, nil | ||
| } | ||
|
|
||
| for _, header := range batches { | ||
| feeHist.GasUsedRatio = append(feeHist.GasUsedRatio, 0.9) //todo @siliev - fix when baseFee is dynamic | ||
| feeHist.BaseFee = append(feeHist.BaseFee, (*hexutil.Big)(header.BaseFee)) | ||
| } | ||
| return feeHist, nil |
There was a problem hiding this comment.
The FeeHistory function has been modified to retrieve batch headers for fee history. Ensure that the logic for processing fee history data is correctly implemented and that the hardcoded 0.9 gas used ratio is replaced with dynamic values when available.
- feeHist.GasUsedRatio = append(feeHist.GasUsedRatio, 0.9) //todo @siliev - fix when baseFee is dynamic
+ // TODO: Replace the hardcoded value with dynamic calculation of gas used ratio
+ feeHist.GasUsedRatio = append(feeHist.GasUsedRatio, calculateDynamicGasUsedRatio(header))Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (api *EthereumAPI) FeeHistory(context.Context, string, rpc.BlockNumber, []float64) (*FeeHistoryResult, error) { | |
| // todo (#1621) - return a non-dummy fee history | |
| header, err := api.host.DB().GetHeadBatchHeader() | |
| if err != nil { | |
| api.logger.Error(fmt.Sprintf("Unable to retrieve header for fee history. Cause: %w", err)) | |
| return nil, fmt.Errorf("unable to retrieve fee history") | |
| } | |
| batches := make([]*common.BatchHeader, 0) | |
| batches = append(batches, header) | |
| return &FeeHistoryResult{ | |
| OldestBlock: (*hexutil.Big)(big.NewInt(0)), | |
| feeHist := &FeeHistoryResult{ | |
| OldestBlock: (*hexutil.Big)(header.Number), | |
| Reward: [][]*hexutil.Big{}, | |
| BaseFee: []*hexutil.Big{}, | |
| GasUsedRatio: []float64{}, | |
| }, nil | |
| } | |
| for _, header := range batches { | |
| feeHist.GasUsedRatio = append(feeHist.GasUsedRatio, 0.9) //todo @siliev - fix when baseFee is dynamic | |
| feeHist.BaseFee = append(feeHist.BaseFee, (*hexutil.Big)(header.BaseFee)) | |
| } | |
| return feeHist, nil | |
| func (api *EthereumAPI) FeeHistory(context.Context, string, rpc.BlockNumber, []float64) (*FeeHistoryResult, error) { | |
| // todo (#1621) - return a non-dummy fee history | |
| header, err := api.host.DB().GetHeadBatchHeader() | |
| if err != nil { | |
| api.logger.Error(fmt.Sprintf("Unable to retrieve header for fee history. Cause: %w", err)) | |
| return nil, fmt.Errorf("unable to retrieve fee history") | |
| } | |
| batches := make([]*common.BatchHeader, 0) | |
| batches = append(batches, header) | |
| feeHist := &FeeHistoryResult{ | |
| OldestBlock: (*hexutil.Big)(header.Number), | |
| Reward: [][]*hexutil.Big{}, | |
| BaseFee: []*hexutil.Big{}, | |
| GasUsedRatio: []float64{}, | |
| } | |
| for _, header := range batches { | |
| // TODO: Replace the hardcoded value with dynamic calculation of gas used ratio | |
| feeHist.GasUsedRatio = append(feeHist.GasUsedRatio, calculateDynamicGasUsedRatio(header)) | |
| feeHist.BaseFee = append(feeHist.BaseFee, (*hexutil.Big)(header.BaseFee)) | |
| } | |
| return feeHist, nil | |
| } |
| } | ||
|
|
||
| for _, header := range batches { | ||
| feeHist.GasUsedRatio = append(feeHist.GasUsedRatio, 0.9) //todo @siliev - fix when baseFee is dynamic |
There was a problem hiding this comment.
The hardcoded value 0.9 for GasUsedRatio should be replaced with a dynamic calculation based on actual gas usage. This is in line with the previous comment by tudor-malene and should be addressed.
go/enclave/evm/evm_facade.go
Outdated
| hasL1Cost := l1cost.Cmp(big.NewInt(0)) != 0 | ||
|
|
||
| if hasL1Cost { | ||
| l1Gas.Div(l1cost, header.BaseFee) |
There was a problem hiding this comment.
The calculation of l1Gas within the executeTransaction function should be documented to explain its purpose and how it relates to the transaction's gas limit and l1cost.
| Nonce: startingNonce + uint64(idx), | ||
| Value: alloc, | ||
| Gas: uint64(1_000_000), | ||
| GasPrice: gethcommon.Big1, | ||
| To: &destAddr, | ||
| } | ||
|
|
||
| tx := faucetClient.EstimateGasAndGasPrice(txData) | ||
| signedTx, err := faucetWallet.SignTransaction(tx) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
The use of panic for error handling is still present. It is recommended to handle errors more gracefully by returning them instead of panicking, which can crash the entire program.
- panic(err)
+ return fmt.Errorf("error signing transaction: %w", err)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| panic(err) | |
| return fmt.Errorf("error signing transaction: %w", err) |
integration/common/utils.go
Outdated
| panic(fmt.Sprintf("could not transfer from faucet. Cause: %s", err)) | ||
| var txJson []byte | ||
| txJson, _ = signedTx.MarshalJSON() | ||
| panic(fmt.Sprintf("could not transfer from faucet for tx %s. Cause: %s", string(txJson[:]), err)) |
There was a problem hiding this comment.
The updated error message now includes transaction details, which can be helpful for debugging. However, the use of panic should be replaced with proper error handling.
- panic(fmt.Sprintf("could not transfer from faucet for tx %s. Cause: %s", string(txJson[:]), err))
+ return fmt.Errorf("could not transfer from faucet for tx %s. Cause: %w", string(txJson[:]), err)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| panic(fmt.Sprintf("could not transfer from faucet for tx %s. Cause: %s", string(txJson[:]), err)) | |
| return fmt.Errorf("could not transfer from faucet for tx %s. Cause: %w", string(txJson[:]), err) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (7)
- go/enclave/components/batch_executor.go (7 hunks)
- go/enclave/enclave.go (5 hunks)
- go/enclave/nodetype/sequencer.go (2 hunks)
- go/rpc/client.go (1 hunks)
- integration/common/utils.go (2 hunks)
- integration/simulation/simulation.go (5 hunks)
- integration/simulation/validate_chain.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- go/enclave/components/batch_executor.go
- go/enclave/nodetype/sequencer.go
- go/rpc/client.go
- integration/common/utils.go
- integration/simulation/simulation.go
Additional comments: 7
integration/simulation/validate_chain.go (4)
474-480: The introduction of
notFoundTransfersvariable and the loop to iterate overtransfersis a logical change for tracking transactions not found in the L2 blockchain. Ensure that the logic for determining if a transaction was not found is correct and that the error handling is adequate.480-480: Ensure that the
getSenderfunction correctly retrieves the sender's address from the transaction and that the error handling for the transaction retrieval by hash is robust.Verification successful
The
getSenderfunction invalidate_chain.goretrieves the sender's address from a transaction and panics if it encounters an error, which is a strong form of error handling that prevents further execution. This behavior is appropriate for critical operations where failure to retrieve the sender's address would invalidate the transaction validation process. The correctness of thegetSenderfunction depends on thetypes.Senderfunction, which is assumed to be reliable.
- The use of panic in
getSenderis a valid error handling strategy for critical validation checks.- The function is used consistently in the context of transaction retrieval.
* 480-480: The same review comments for the `transfers` loop apply to the loops for `withdrawals` and `nativeTransfers`. Ensure that the logic for checking the presence of these transactions is correct and that error handling is in place.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correctness of the getSender function. ast-grep --lang go --pattern $'func getSender($_) $_ { $$$ }' # Check for usage of the getSender function in the context of transaction retrieval. rg --type go 'getSender'Length of output: 1010
- 477-477: Initialization of the
notFoundTransfersvariable is correct, and the function's return values seem to be used to report the number of transactions not found. Ensure that these counts are used appropriately in the calling context.go/enclave/enclave.go (3)
80-83: The addition of
gasOracleto theenclaveImplstruct and its initialization in theNewEnclavefunction is consistent with the PR's objective to improve gas estimation and handling.272-275: The assignment of
chain,registry,service, andgasOraclein theNewEnclavefunction's return statement is correct and follows Go's idiomatic way of initializing struct fields.1083-1085: The calculation of
totalGasEstimatein theEstimateGasfunction is a new addition and seems to correctly combine the L1 cost with the execution gas estimate. This change is in line with the PR's objectives to improve gas estimation.
go/enclave/enclave.go
Outdated
| l2Head, err := e.storage.FetchBatchBySeqNo(seqNo) | ||
| if err == nil { | ||
| // todo - we should return an error when head state is not available, but for current test situations with race | ||
| // conditions we allow it to return zero while head state is uninitialized | ||
| s, err := e.storage.CreateStateDB(l2Head.Hash()) | ||
| if err != nil { | ||
| return nil, responses.ToInternalError(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in GetTransactionCount to handle the retrieval of the nonce seems to be incomplete. The variable nonce is declared but not assigned any value before being encoded and returned. This could lead to returning an incorrect nonce value (always zero).
- var nonce uint64
+ nonce, err := e.chain.GetNonceAt(address, l2Head.Number())
+ if err != nil {
+ return responses.AsPlaintextError(fmt.Errorf("unable to get nonce for address %s: %v", address.Hex(), err)), nil
+ }Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| l2Head, err := e.storage.FetchBatchBySeqNo(seqNo) | |
| if err == nil { | |
| // todo - we should return an error when head state is not available, but for current test situations with race | |
| // conditions we allow it to return zero while head state is uninitialized | |
| s, err := e.storage.CreateStateDB(l2Head.Hash()) | |
| if err != nil { | |
| return nil, responses.ToInternalError(err) | |
| } | |
| } | |
| l2Head, err := e.storage.FetchBatchBySeqNo(seqNo) | |
| if err == nil { | |
| // todo - we should return an error when head state is not available, but for current test situations with race | |
| // conditions we allow it to return zero while head state is uninitialized | |
| s, err := e.storage.CreateStateDB(l2Head.Hash()) | |
| if err != nil { | |
| return nil, responses.ToInternalError(err) | |
| } | |
| nonce, err := e.chain.GetNonceAt(address, l2Head.Number()) | |
| if err != nil { | |
| return responses.AsPlaintextError(fmt.Errorf("unable to get nonce for address %s: %v", address.Hex(), err)), nil | |
| } | |
| } |
| block, err := e.blockResolver.FetchHeadBlock() | ||
| if err != nil { | ||
| return responses.AsPlaintextError(fmt.Errorf("internal server error")), err | ||
| } | ||
|
|
||
| // The message is ran through the l1 publishing cost estimation for the current | ||
| // known head block. | ||
| l1Cost, err := e.gasOracle.EstimateL1CostForMsg(callMsg, block) | ||
| if err != nil { | ||
| return responses.AsPlaintextError(fmt.Errorf("internal server error")), err | ||
| } |
There was a problem hiding this comment.
The EstimateGas function now includes additional calculations for gas estimation, which aligns with the PR's objectives. However, the error messages on lines 1042 and 1049 are generic and do not provide enough context about the failure. It would be beneficial to include more details in the error messages to aid in debugging.
- return responses.AsPlaintextError(fmt.Errorf("internal server error")), err
+ return responses.AsPlaintextError(fmt.Errorf("failed to fetch head block for gas estimation: %w", err)), nilCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| block, err := e.blockResolver.FetchHeadBlock() | |
| if err != nil { | |
| return responses.AsPlaintextError(fmt.Errorf("internal server error")), err | |
| } | |
| // The message is ran through the l1 publishing cost estimation for the current | |
| // known head block. | |
| l1Cost, err := e.gasOracle.EstimateL1CostForMsg(callMsg, block) | |
| if err != nil { | |
| return responses.AsPlaintextError(fmt.Errorf("internal server error")), err | |
| } | |
| block, err := e.blockResolver.FetchHeadBlock() | |
| if err != nil { | |
| return responses.AsPlaintextError(fmt.Errorf("failed to fetch head block for gas estimation: %w", err)), nil | |
| } | |
| // The message is ran through the l1 publishing cost estimation for the current | |
| // known head block. | |
| l1Cost, err := e.gasOracle.EstimateL1CostForMsg(callMsg, block) | |
| if err != nil { | |
| return responses.AsPlaintextError(fmt.Errorf("failed to fetch head block for gas estimation: %w", err)), nil | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/enclave/enclave.go (5 hunks)
- go/host/rpc/clientapi/client_api_eth.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- go/enclave/enclave.go
- go/host/rpc/clientapi/client_api_eth.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (5)
- go/enclave/components/batch_executor.go (7 hunks)
- go/enclave/evm/evm_facade.go (4 hunks)
- go/host/rpc/clientapi/client_api_eth.go (3 hunks)
- go/obsclient/authclient.go (5 hunks)
- tools/hardhatdeployer/contract_deployer.go (2 hunks)
Additional comments: 13
tools/hardhatdeployer/contract_deployer.go (2)
13-13: The import of the
paramspackage is noted and seems to be used appropriately for referencing network parameters such asInitialBaseFee.95-95: The change to
GasPriceto useparams.InitialBaseFeealigns with dynamic network fee structures. Please ensure thatparams.InitialBaseFeeis correctly set and reflects the intended base fee for the network.go/obsclient/authclient.go (2)
11-11: The import of the
paramspackage is noted and seems to be used appropriately for referencing network parameters such asInitialBaseFee.80-88: The addition of the
GasPricemethod to theAuthObsClientstruct is a good enhancement for gas price estimation. Please ensure that this method is being used correctly wherever gas price estimation is required.go/enclave/evm/evm_facade.go (2)
34-34: The change in the
ExecuteTransactionsfunction signature to acceptcommon.L2PricedTransactionsis noted. Please verify that all calls to this function have been updated to pass the correct type.172-179: Adjustments to the receipt in
executeTransactionto point to the correct batch hash are noted. Please ensure that these changes are consistent with the rest of the system and that the batch hash is being set correctly in all relevant places.go/host/rpc/clientapi/client_api_eth.go (4)
10-10: The addition of the
paramspackage fromgithub.com/ethereum/go-ethereumis noted and seems to be used later in theGasPricefunction to referenceparams.InitialBaseFee.77-77: Replacing hardcoded values with constants from the
paramspackage is a good practice for maintainability and clarity.188-210: The
FeeHistoryfunction signature has been updated to accept a string parameter, which seems to align with the intended use of the function. The implementation now retrieves the latest batch header to construct the fee history. Ensure that all callers of this function are updated to pass a string argument.#!/bin/bash # Search for calls to FeeHistory to ensure they pass a string argument. ast-grep --lang go --pattern $'FeeHistory($_, $$$)'
- 190-210: The implementation of
FeeHistorynow constructs a fee history result using the latest batch header. This change seems to be in line with the PR objectives to improve fee history retrieval.go/enclave/components/batch_executor.go (3)
- 97-125: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [70-118]
The
payL1Feesfunction has been renamed toestimateL1Feesand now returns two slices ofcommon.L2PricedTransactions. The logic within the function has been updated to handle pricing of transactions and the handling of free transactions. This change seems to align with the PR objectives to improve gas estimation and nonce management.
- 169-192: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [172-225]
The
ComputeBatchfunction has been updated with new logic for handling transactions, including cross-chain transactions and the adjustment of logs and receipts. This is a significant change and should be thoroughly tested to ensure it behaves as expected.#!/bin/bash # Search for calls to ComputeBatch to ensure they handle the new return types correctly. ast-grep --lang go --pattern $'ComputeBatch($_, $_)'
- 415-432: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [398-429]
The
processTransactionsfunction now processescommon.L2PricedTransactionsand includes logic for handling executed and excluded transactions, as well as sorting receipts. Ensure that the changes in transaction processing are in line with the intended behavior and that all dependent code has been updated accordingly.#!/bin/bash # Search for calls to processTransactions to ensure they handle the new parameter type correctly. ast-grep --lang go --pattern $'processTransactions($_, $_, $_, $_, $_, $_)'
| GasPrice: big.NewInt(1), | ||
| Gas: uint64(5_000_000), | ||
| GasPrice: big.NewInt(params.InitialBaseFee), | ||
| Gas: uint64(500_000_000), |
There was a problem hiding this comment.
The Gas value has been increased significantly. Could you provide context on the decision for this specific value and confirm that it has been tested across various scenarios, especially considering the potential for transactions to be rejected due to excessively high gas limits?
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/core/types" | ||
| "github.com/ethereum/go-ethereum/crypto" | ||
| "github.com/ethereum/go-ethereum/params" | ||
| "github.com/ten-protocol/go-ten/go/common/constants" | ||
| "github.com/ten-protocol/go-ten/go/common/retry" | ||
| "github.com/ten-protocol/go-ten/go/wallet" |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1-1]
There is a TODO comment regarding the potential merger with the network manager package. Please ensure this is tracked in your project management tool for future action.
go/obsclient/authclient.go
Outdated
| gasPrice, err := ac.GasPrice(context.Background()) | ||
| if err != nil { | ||
| gasPrice = big.NewInt(params.InitialBaseFee) | ||
| } |
There was a problem hiding this comment.
The modification to EstimateGasAndGasPrice to use the new GasPrice method is noted. Please clarify if falling back to params.InitialBaseFee when gas price estimation fails is the intended behavior and confirm that this default value is appropriate in all cases.
| @@ -4,6 +4,7 @@ | |||
| "errors" | |||
| "fmt" | |||
| "math/big" | |||
| _ "unsafe" | |||
There was a problem hiding this comment.
The import of the _ "unsafe" package is unusual. Could you provide clarification on why this is necessary and ensure that its usage complies with best practices for safety and maintainability?
go/enclave/evm/evm_facade.go
Outdated
| result[t.Tx.Hash()] = err | ||
| logger.Info("Failed to execute tx:", log.TxKey, t.Tx.Hash(), log.CtrErrKey, err) | ||
| continue | ||
| } | ||
| result[t.Hash()] = r | ||
| result[t.Tx.Hash()] = r | ||
| logReceipt(r, logger) | ||
| } | ||
| s.Finalise(true) | ||
| return result | ||
| } | ||
|
|
||
| //go:linkname applyTransaction github.com/ethereum/go-ethereum/core.applyTransaction | ||
| func applyTransaction(msg *gethcore.Message, config *params.ChainConfig, gp *gethcore.GasPool, statedb *state.StateDB, blockNumber *big.Int, blockHash gethcommon.Hash, tx *types.Transaction, usedGas *uint64, evm *vm.EVM) (*types.Receipt, error) | ||
|
|
||
| func executeTransaction( | ||
| s *state.StateDB, | ||
| cc *params.ChainConfig, | ||
| chain *ObscuroChainContext, | ||
| gp *gethcore.GasPool, | ||
| header *types.Header, | ||
| t *common.L2Tx, | ||
| t common.L2PricedTransaction, | ||
| usedGas *uint64, | ||
| vmCfg vm.Config, | ||
| tCount int, | ||
| batchHash common.L2BatchHash, | ||
| batchHeight uint64, | ||
| ) (*types.Receipt, error) { | ||
| rules := cc.Rules(big.NewInt(0), true, 0) | ||
| from, err := types.Sender(types.LatestSigner(cc), t) | ||
| from, err := types.Sender(types.LatestSigner(cc), t.Tx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| s.Prepare(rules, from, gethcommon.Address{}, t.To(), nil, nil) | ||
| s.Prepare(rules, from, gethcommon.Address{}, t.Tx.To(), nil, nil) | ||
| snap := s.Snapshot() | ||
| s.SetTxContext(t.Hash(), tCount) | ||
| s.SetTxContext(t.Tx.Hash(), tCount) | ||
|
|
||
| before := header.MixDigest | ||
| // calculate a random value per transaction | ||
| header.MixDigest = crypto.CalculateTxRnd(before.Bytes(), tCount) | ||
| receipt, err := gethcore.ApplyTransaction(cc, chain, nil, gp, s, header, t, usedGas, vmCfg) | ||
|
|
||
| // adjust the receipt to point to the right batch hash | ||
| if receipt != nil { | ||
| receipt.Logs = s.GetLogs(t.Hash(), batchHeight, batchHash) | ||
| receipt.BlockHash = batchHash | ||
| receipt.BlockNumber = big.NewInt(int64(batchHeight)) | ||
| for _, l := range receipt.Logs { | ||
| l.BlockHash = batchHash | ||
| applyTx := func( | ||
| config *params.ChainConfig, | ||
| bc gethcore.ChainContext, | ||
| author *gethcommon.Address, | ||
| gp *gethcore.GasPool, | ||
| statedb *state.StateDB, | ||
| header *types.Header, | ||
| tx common.L2PricedTransaction, | ||
| usedGas *uint64, | ||
| cfg vm.Config, | ||
| ) (*types.Receipt, error) { | ||
| msg, err := gethcore.TransactionToMessage(tx.Tx, types.MakeSigner(config, header.Number, header.Time), header.BaseFee) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| l1cost := tx.PublishingCost | ||
| l1Gas := big.NewInt(0) | ||
| hasL1Cost := l1cost.Cmp(big.NewInt(0)) != 0 | ||
|
|
||
| if hasL1Cost { | ||
| l1Gas.Div(l1cost, header.BaseFee) | ||
| l1Gas.Add(l1Gas, big.NewInt(1)) // account for leftover | ||
|
|
||
| if msg.GasLimit < l1Gas.Uint64() { | ||
| return nil, fmt.Errorf("gas limit for tx too low. Want at least: %d have: %d", l1Gas, msg.GasLimit) | ||
| } | ||
| msg.GasLimit -= l1Gas.Uint64() | ||
|
|
||
| statedb.SubBalance(msg.From, l1cost) | ||
| statedb.AddBalance(header.Coinbase, l1cost) | ||
|
|
||
| } | ||
|
|
||
| // Create a new context to be used in the EVM environment | ||
| blockContext := gethcore.NewEVMBlockContext(header, bc, author) | ||
| vmenv := vm.NewEVM(blockContext, vm.TxContext{BlobHashes: tx.Tx.BlobHashes()}, statedb, config, cfg) | ||
| receipt, err := applyTransaction(msg, config, gp, statedb, header.Number, header.Hash(), tx.Tx, usedGas, vmenv) | ||
| if err != nil { | ||
| if hasL1Cost { | ||
| statedb.SubBalance(header.Coinbase, l1cost) | ||
| statedb.AddBalance(msg.From, l1cost) | ||
| } | ||
|
|
||
| return receipt, err | ||
| } | ||
|
|
||
| // Do not increase the balance of zero address as it is the contract deployment address. |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [112-170]
The addition of the applyTransaction function is a significant change. Please provide more context on its implementation, particularly how it handles L1 costs and gas adjustments, and confirm that it has been tested thoroughly, including edge cases and error handling.
go/enclave/evm/evm_facade.go
Outdated
| l1Gas.Div(l1cost, header.BaseFee) | ||
| l1Gas.Add(l1Gas, big.NewInt(1)) // account for leftover | ||
|
|
||
| if msg.GasLimit < l1Gas.Uint64() { | ||
| return nil, fmt.Errorf("gas limit for tx too low. Want at least: %d have: %d", l1Gas, msg.GasLimit) | ||
| } |
There was a problem hiding this comment.
It appears there was a previous comment regarding the need for documentation on the calculation of l1Gas. Please ensure that this calculation is well-documented for maintainability.
go/enclave/evm/evm_facade.go
Outdated
| l1Gas.Add(l1Gas, big.NewInt(1)) // account for leftover | ||
|
|
||
| if msg.GasLimit < l1Gas.Uint64() { | ||
| return nil, fmt.Errorf("gas limit for tx too low. Want at least: %d have: %d", l1Gas, msg.GasLimit) |
There was a problem hiding this comment.
The error message for a low gas limit should clearly indicate it is a user error and provide guidance on how to resolve it. This aligns with the previous comment by tudor-malene and should be addressed.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/enclave/components/batch_executor.go (7 hunks)
- go/enclave/crosschain/message_bus_manager.go (1 hunks)
Additional comments: 5
go/enclave/crosschain/message_bus_manager.go (1)
- 89-89: The gas limit for the
GenerateMessageBusDeployTxmethod has been increased to 500,000,000 to accommodate the new cost estimation strategy.go/enclave/components/batch_executor.go (4)
- 97-125: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [70-118]
The
estimateL1Feesfunction has been updated to handle the newcommon.L2PricedTransactionstype and to calculate and store the publishing cost for each transaction.
- 172-201: The
ComputeBatchfunction has been updated to integrate the new fee estimation logic and handlecommon.L2PricedTransactions.- 381-382: The
verifyInboundCrossChainTransactionsfunction now correctly takescommon.L2PricedTransactionsas the first argument.- 415-432: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [398-429]
The
processTransactionsfunction has been updated to takecommon.L2PricedTransactionsand includes logic for handling the new transaction types.
| /*stateDB.SubBalance(*sender, cost) | ||
| stateDB.AddBalance(context.Creator, cost)*/ |
There was a problem hiding this comment.
The commented-out code for fee handling should be removed as it can lead to confusion and is no longer needed.
- /*stateDB.SubBalance(*sender, cost)
- stateDB.AddBalance(context.Creator, cost)*/
- // todo - add refund logic.Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| /*stateDB.SubBalance(*sender, cost) | |
| stateDB.AddBalance(context.Creator, cost)*/ |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- go/config/enclave_cli_flags.go (2 hunks)
- go/enclave/components/batch_executor.go (7 hunks)
- go/enclave/evm/evm_facade.go (4 hunks)
- go/obsclient/authclient.go (5 hunks)
- integration/common/constants.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- go/enclave/components/batch_executor.go
Additional comments: 13
integration/common/constants.go (4)
- 7-7: The import of
github.com/ethereum/go-ethereum/paramsis added to useparams.InitialBaseFee.- 79-79: Replacing hard-coded constants with
params.InitialBaseFeeforMinGasPriceis a good practice to keep the codebase up to date with the network parameters.- 86-86: Using
params.InitialBaseFeeforBaseFeeensures consistency with the network's base fee.- 88-92: The significant increase in
GasLocalExecutionCapFlagandGasBatchExecutionLimitvalues aligns with the PR's objective to handle high gas limits and accommodate hidden L1 costs within L2 transactions.go/config/enclave_cli_flags.go (2)
- 4-4: The import of
github.com/ethereum/go-ethereum/paramsis added to useparams.InitialBaseFee.- 57-57: Setting the default value of
L2BaseFeeFlagtoparams.InitialBaseFeeensures that the base fee is aligned with the network's parameters.go/obsclient/authclient.go (3)
- 11-11: The import of
github.com/ethereum/go-ethereum/paramsis added to useparams.InitialBaseFee.- 80-88: The new
GasPricemethod is added to theAuthObsClientstruct, which is a good encapsulation of the gas price retrieval logic.- 192-198: The
EstimateGasAndGasPricemethod now uses the newGasPricemethod, which is a cleaner approach than directly accessing the gas price.go/enclave/evm/evm_facade.go (4)
- 8-10: The import of the
_ "unsafe"package has been previously flagged and discussed.- 37-37: The
txsparameter type in theExecuteTransactionsfunction has been updated tocommon.L2PricedTransactions, which is consistent with the PR's objectives to handle gas costs more accurately.- 135-136: The calculation of
l1Gaswithin theexecuteTransactionfunction has been previously flagged for needing documentation.- 175-182: The adjustments to the receipt in
executeTransactionto point to the correct batch hash and block number are consistent with the PR's objectives and improve the accuracy of transaction tracking.
go/enclave/evm/evm_facade.go
Outdated
| applyTx := func( | ||
| config *params.ChainConfig, | ||
| bc gethcore.ChainContext, | ||
| author *gethcommon.Address, | ||
| gp *gethcore.GasPool, | ||
| statedb *state.StateDB, | ||
| header *types.Header, | ||
| tx common.L2PricedTransaction, | ||
| usedGas *uint64, | ||
| cfg vm.Config, | ||
| ) (*types.Receipt, error) { | ||
| msg, err := gethcore.TransactionToMessage(tx.Tx, types.MakeSigner(config, header.Number, header.Time), header.BaseFee) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| l1cost := tx.PublishingCost | ||
| l1Gas := big.NewInt(0) | ||
| hasL1Cost := l1cost.Cmp(big.NewInt(0)) != 0 | ||
|
|
||
| if hasL1Cost { | ||
| l1Gas.Div(l1cost, header.BaseFee) | ||
| l1Gas.Add(l1Gas, big.NewInt(1)) | ||
|
|
||
| if msg.GasLimit < l1Gas.Uint64() { | ||
| return nil, fmt.Errorf("gas limit set by user is too low to pay for execution and l1 fees. Want at least: %d have: %d", l1Gas, msg.GasLimit) | ||
| } | ||
| msg.GasLimit -= l1Gas.Uint64() | ||
|
|
||
| statedb.SubBalance(msg.From, l1cost) | ||
| statedb.AddBalance(header.Coinbase, l1cost) | ||
|
|
||
| } |
There was a problem hiding this comment.
The lambda function applyTx within executeTransaction should be documented to explain its purpose, especially the logic involving l1Gas and how it relates to the transaction's gas limit and l1cost. This will improve maintainability and understandability of the code.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts (1 hunks)
Additional comments: 1
contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts (1)
- 100-100: The
maxFeePerGasvalue has been increased from 2 to 1000000001. This is a significant change that will affect the gas fee calculation and the verification of a message's finalization. Please ensure that this value is appropriate and has been tested thoroughly to avoid unexpected costs.Also applies to: 104-104
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts (1 hunks)
- go/config/enclave_cli_flags.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts
- go/config/enclave_cli_flags.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/faucet/faucet/faucet.go (1 hunks)
Additional comments: 1
tools/faucet/faucet/faucet.go (1)
- 127-129: The hardcoded gas value has been removed, and the
EstimateGasAndGasPricemethod is now used to dynamically determine the gas for the transaction. This change should improve the accuracy of gas estimation and is in line with the PR's objectives to enhance MetaMask compatibility.
StefanIliev545
changed the title
| @@ -97,43 +97,25 @@ func (executor *batchExecutor) payL1Fees(stateDB *state.StateDB, context *BatchE | |||
| isFreeTransaction = isFreeTransaction && tx.GasPrice().Cmp(gethcommon.Big0) == 0 | |||
|
|
|||
| if isFreeTransaction { | |||
| freeTransactions = append(freeTransactions, tx) | |||
| freeTransactions = append(freeTransactions, common.L2PricedTransaction{ | |||
There was a problem hiding this comment.
I'm not quite clear what prevents a user from setting the "GasFeeCap=0", and become a "free transaction"?
There was a problem hiding this comment.
The mempool would reject such a transaction. The only way to get it in is bypass the mempool.
| @@ -133,7 +133,7 @@ func (oc *obscuroChain) ObsCallAtBlock(apiArgs *gethapi.TransactionArgs, blockNu | |||
| return nil, fmt.Errorf("unable to fetch head state batch. Cause: %w", err) | |||
| } | |||
|
|
|||
| callMsg, err := apiArgs.ToMessage(batch.Header.GasLimit, batch.Header.BaseFee) | |||
| callMsg, err := apiArgs.ToMessage(batch.Header.GasLimit-1, batch.Header.BaseFee) | |||
There was a problem hiding this comment.
crickets ... I must have had some genius thought that is now forever lost 🪕
go/enclave/nodetype/sequencer.go
Outdated
| @@ -234,6 +242,8 @@ func (s *sequencer) createNewHeadBatch(l1HeadBlock *common.L1Block, skipBatchIfE | |||
| } | |||
| } | |||
|
|
|||
| sort.Sort(SortableTransactions(transactions)) | |||
There was a problem hiding this comment.
Umm. I'd say this deserves a bit of investigation. Maybe instead of sorting, check that what the mempool produces is sorted by nonce. And if not, we should investigate why not. It's probably some misconfiguration
| } | ||
|
|
||
| for _, header := range batches { | ||
| feeHist.GasUsedRatio = append(feeHist.GasUsedRatio, 0.9) // todo @siliev - fix when baseFee is dynamic |
There was a problem hiding this comment.
what is the significance of the 0.9 magic number?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- go/enclave/components/batch_executor.go (6 hunks)
- go/enclave/evm/evm_facade.go (4 hunks)
- go/host/rpc/clientapi/client_api_eth.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- go/enclave/evm/evm_facade.go
Additional comments: 6
go/host/rpc/clientapi/client_api_eth.go (2)
- 77-77: The change to use
params.InitialBaseFeein theGasPricemethod aligns with the Arbitrum gas model migration.- 188-212: The modifications to the
FeeHistorymethod, including the change in parameter type and the logic to construct theFeeHistoryResult, are consistent with the PR objectives.go/enclave/components/batch_executor.go (4)
- 97-121: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [70-114]
The renaming of
payL1FeestofilterTransactionsWithSufficientFundsand the changes to returncommon.L2PricedTransactionswith the publishing cost calculation are in line with the PR's objectives to adjust gas estimation to include L1 costs.
- 168-197: The integration of
refundL1Feeslogic intoComputeBatchand the adjustments toverifyInboundCrossChainTransactionsandprocessTransactionsfunctions to handlecommon.L2PricedTransactionsare appropriate for the new transaction processing flow.- 376-377: The update to
verifyInboundCrossChainTransactionsto acceptcommon.L2PricedTransactionsis consistent with the changes made to the transaction processing logic.- 410-427: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [393-424]
The changes to the
processTransactionsfunction to acceptcommon.L2PricedTransactionsalign with the updated transaction processing requirements.
| @@ -175,19 +175,19 @@ func (executor *batchExecutor) ComputeBatch(context *BatchExecutionContext, fail | |||
| }) | |||
| } | |||
|
|
|||
| xchainTxs = append(xchainTxs, freeTransactions...) | |||
| syntheticTransactions := append(xchainTxs, freeTransactions...) | |||
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (5)
- go/enclave/components/batch_executor.go (7 hunks)
- go/enclave/evm/evm_facade.go (4 hunks)
- go/enclave/gas/oracle.go (2 hunks)
- integration/common/utils.go (4 hunks)
- integration/obscurogateway/tengateway_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- go/enclave/components/batch_executor.go
- go/enclave/evm/evm_facade.go
Additional comments: 9
go/enclave/gas/oracle.go (3)
- 17-17: The method
EstimateL1CostForMsghas been added to theOracleinterface. Ensure that all implementations of this interface are updated to include this new method.- 56-68: The implementation of
EstimateL1CostForMsgin theoraclestruct seems to correctly calculate the L1 cost for a message. However, ensure that the calculation ofoverheadandl1Gasaligns with the expected gas model.- 65-65: The usage of
params.TxDataNonZeroGasEIP2028suggests that the code is considering EIP-2028 standards for calldata gas cost. Confirm that this is the intended behavior for the gas model being implemented.integration/common/utils.go (1)
- 100-100: The
PrefundWalletsfunction has been refactored to usetxDataandfaucetClient.EstimateGasAndGasPrice. Ensure that the refactoring maintains the original functionality and that the gas estimation is accurate.integration/obscurogateway/tengateway_test.go (5)
- 659-681: The
getFeeAndGasfunction has been added to calculate gas and fee values for transactions. Ensure that the gas estimation logic is consistent with the expected behavior and that the base fee is correctly retrieved from the fee history.- 163-165: The
getFeeAndGasfunction is used in thetestMultipleAccountsSubscriptionfunction. Verify that the gas and fee values are correctly applied to the transaction.- 425-427: The
getFeeAndGasfunction is used in thetestErrorsRevertedArePassedfunction. Confirm that the gas estimation and fee application are correct and that the error handling is consistent with the expected behavior.- 493-494: The
getFeeAndGasfunction is used in thetestUnsubscribefunction. Ensure that the gas estimation is accurate and that the function behaves as expected after unsubscribing from events.- 545-546: The
getFeeAndGasfunction is used in thetestClosingConnectionWhileSubscribedfunction. Verify that the gas estimation is correct and that the behavior after closing the websocket connection is as expected.
integration/common/utils.go
Outdated
| Nonce: startingNonce + uint64(idx), | ||
| Value: alloc, | ||
| Gas: uint64(100_000), | ||
| GasPrice: gethcommon.Big1, | ||
| To: &destAddr, | ||
| } | ||
|
|
||
| tx := faucetClient.EstimateGasAndGasPrice(txData) // nolint: contextcheck |
There was a problem hiding this comment.
The use of EstimateGasAndGasPrice without checking the context for cancellation may lead to potential issues if the context is canceled. Consider passing the context to this function.
- tx := faucetClient.EstimateGasAndGasPrice(txData) // nolint: contextcheck
+ tx, err := faucetClient.EstimateGasAndGasPrice(ctx, txData)
+ if err != nil {
+ return fmt.Errorf("error estimating gas and gas price: %w", err)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| tx := faucetClient.EstimateGasAndGasPrice(txData) // nolint: contextcheck | |
| tx, err := faucetClient.EstimateGasAndGasPrice(ctx, txData) | |
| if err != nil { | |
| return fmt.Errorf("error estimating gas and gas price: %w", err) | |
| } |
integration/common/utils.go
Outdated
| tx := faucetClient.EstimateGasAndGasPrice(txData) // nolint: contextcheck | ||
| signedTx, err := faucetWallet.SignTransaction(tx) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| err = faucetClient.SendTransaction(ctx, signedTx) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("could not transfer from faucet. Cause: %s", err)) | ||
| var txJSON []byte | ||
| txJSON, _ = signedTx.MarshalJSON() | ||
| panic(fmt.Sprintf("could not transfer from faucet for tx %s. Cause: %s", string(txJSON[:]), err)) |
There was a problem hiding this comment.
The panic usage for error handling in PrefundWallets should be replaced with proper error handling to prevent crashing the program.
- panic(err)
+ return fmt.Errorf("error signing transaction: %w", err)
- panic(fmt.Sprintf("could not transfer from faucet for tx %s. Cause: %s", string(txJSON[:]), err))
+ return fmt.Errorf("could not transfer from faucet for tx %s. Cause: %w", string(txJSON[:]), err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| tx := faucetClient.EstimateGasAndGasPrice(txData) // nolint: contextcheck | |
| signedTx, err := faucetWallet.SignTransaction(tx) | |
| if err != nil { | |
| panic(err) | |
| } | |
| err = faucetClient.SendTransaction(ctx, signedTx) | |
| if err != nil { | |
| panic(fmt.Sprintf("could not transfer from faucet. Cause: %s", err)) | |
| var txJSON []byte | |
| txJSON, _ = signedTx.MarshalJSON() | |
| panic(fmt.Sprintf("could not transfer from faucet for tx %s. Cause: %s", string(txJSON[:]), err)) | |
| tx := faucetClient.EstimateGasAndGasPrice(txData) // nolint: contextcheck | |
| signedTx, err := faucetWallet.SignTransaction(tx) | |
| if err != nil { | |
| return fmt.Errorf("error signing transaction: %w", err) | |
| } | |
| err = faucetClient.SendTransaction(ctx, signedTx) | |
| if err != nil { | |
| var txJSON []byte | |
| txJSON, _ = signedTx.MarshalJSON() | |
| return fmt.Errorf("could not transfer from faucet for tx %s. Cause: %w", string(txJSON[:]), err) |
| @@ -124,7 +129,7 @@ | |||
| defer wg.Done() | |||
| err := AwaitReceipt(ctx, faucetClient, txHash, timeout) | |||
| if err != nil { | |||
| panic(fmt.Sprintf("faucet transfer transaction unsuccessful. Cause: %s", err)) | |||
| panic(fmt.Sprintf("faucet transfer transaction %s unsuccessful. Cause: %s", txHash, err)) | |||
There was a problem hiding this comment.
The panic usage in AwaitReceipt should be replaced with proper error handling to prevent crashing the program.
- panic(fmt.Sprintf("faucet transfer transaction %s unsuccessful. Cause: %s", txHash, err))
+ return fmt.Errorf("faucet transfer transaction %s unsuccessful. Cause: %w", txHash, err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| panic(fmt.Sprintf("faucet transfer transaction %s unsuccessful. Cause: %s", txHash, err)) | |
| return fmt.Errorf("faucet transfer transaction %s unsuccessful. Cause: %w", txHash, err) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/common/utils.go (4 hunks)
- integration/obscurogateway/tengateway_test.go (8 hunks)
Files skipped from review as they are similar to previous changes (2)
- integration/common/utils.go
- integration/obscurogateway/tengateway_test.go
Why this change is needed
This PR fixes the compatibility issues with metamask:
Sim tests were changed:
There was a mix up in transfers tracking.
Gas pricing enabled.
Gas limit is now set by estimating the gas as the estimation now includes l1 fees.
Gas limit per batch raised significantly - The geth mempool rejects transactions with too high of a gas limit, but as the l1 cost is now hidden within the gas limit of a transaction, they can overrun the low ethereum cloned limits. Arbitrum has a very high gas limit and should (or at least was) use the same strategy we are.
What changes were made as part of this PR
The L1 payments and refunds have been migrated on a per tx basis inside the EVM facade.
Estimate gas takes a predicted l1 cost and divides it by the current l2 gas price in order to derive a gas quantity.
Gas Quantity = Total Cost / Gas Price Per UnitSimulation tests have been adjusted.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks