bump eth from v1.10.26 to v1.11.0 #1864
Conversation
trajan0x
requested review from
dwasse,
aureliusbtc and
ChiTimesChi
as code owners
WalkthroughThe recent updates include the introduction of a generic pointer copy function in Go, extended testing for both nil and non-nil pointers, enhancements to the Ethereum simulated backend reflecting protocol updates, and adjustments to gas configuration in line with new block time settings. Additionally, there's a shift from Ropsten to Sepolia chain configurations, and trie databases are now part of the gas price testing setup. 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 path filters (19)
agents/go.modis excluded by:!**/*.modagents/go.sumis excluded by:!**/*.sumcontrib/promexporter/go.modis excluded by:!**/*.modcontrib/promexporter/go.sumis excluded by:!**/*.sumethergo/go.modis excluded by:!**/*.modethergo/go.sumis excluded by:!**/*.sumgo.work.sumis excluded by:!**/*.sumservices/cctp-relayer/go.modis excluded by:!**/*.modservices/cctp-relayer/go.sumis excluded by:!**/*.sumservices/explorer/go.modis excluded by:!**/*.modservices/explorer/go.sumis excluded by:!**/*.sumservices/omnirpc/go.modis excluded by:!**/*.modservices/omnirpc/go.sumis excluded by:!**/*.sumservices/rfq/go.modis excluded by:!**/*.modservices/rfq/go.sumis excluded by:!**/*.sumservices/scribe/go.modis excluded by:!**/*.modservices/scribe/go.sumis excluded by:!**/*.sumservices/sinner/go.modis excluded by:!**/*.modservices/sinner/go.sumis excluded by:!**/*.sum
Files selected for processing (8)
- core/ptr.go (1 hunks)
- core/ptr_test.go (1 hunks)
- ethergo/backends/simulated/multibackend/chainid.go (2 hunks)
- ethergo/backends/simulated/multibackend/simulated_gen.go (5 hunks)
- ethergo/backends/simulated/multibackend/simulated_gen_test.go (6 hunks)
- ethergo/chain/client/config.go (1 hunks)
- ethergo/chain/gas/gas_setter_test.go (1 hunks)
- ethergo/chain/gas/londinium/gasprice_test.go (3 hunks)
Files skipped from review due to trivial changes (1)
- ethergo/backends/simulated/multibackend/simulated_gen_test.go
Additional comments: 14
core/ptr.go (1)
- 51-58: The implementation of
CopyPointerlooks correct and follows best practices for generic pointer copying in Go. It properly handlesnilpointers and returns a new pointer to a new value of the same type.core/ptr_test.go (2)
52-69: The test
TestCopyPointercorrectly checks for non-nil pointers, ensuring that a new pointer is returned and that the value is correctly copied.71-79: The test
TestCopyPointerNilcorrectly checks fornilpointer handling, ensuring thatCopyPointerreturnsnilwhen provided with anilpointer.ethergo/backends/simulated/multibackend/chainid.go (2)
37-43: The updates to the
NewConfigWithChainIDfunction include new protocol parameters and use theCopyPointerandCopyBigIntutility functions to ensure that the configuration is correctly initialized. This is a good use of the new utility functions to avoid direct pointer copying, which can lead to shared state bugs.53-53: The
NewSimulatedBackendWithConfigfunction correctly initializes a new simulated backend with the provided configuration. The use ofMustCommitto commit the genesis state to the database and the subsequent creation of the blockchain and backend are standard practices for setting up a simulated backend in Go Ethereum.ethergo/chain/gas/londinium/gasprice_test.go (2)
21-21: The addition of the
github.com/ethereum/go-ethereum/trieimport is necessary for the changes made to thenewTestBackendfunction, which now includes trie databases for testing.72-79: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [75-90]
The
newTestBackendfunction has been updated to include trie database initialization, which is a necessary change for testing purposes. The use oftrie.NewDatabaseand the subsequent calls togspec.Commitandcore.NewBlockChainare consistent with the initialization of a test blockchain that includes state trie information.ethergo/chain/gas/gas_setter_test.go (1)
- 45-46: The changes to the
TestGasPriceSetterFrom0PreLondonfunction reflect the updated configuration fieldsShanghaiTimeandCancunTime. This change is likely due to an update in the Ethereum protocol where block numbers are replaced by timestamps for certain upgrades. It's important to ensure that these changes are reflected throughout the codebase wherever the old block number fields were used.Verification successful
The absence of output from the
rgcommand suggests that the termsShanghaiBlockandCancunBlockare not present in any.gofiles in the codebase. This would imply that the old block number fields have been successfully removed or replaced, in line with the changes observed in thegas_setter_test.gofile.Based on this information, the review comment is consistent with the codebase, as it approves the changes and suggests verification throughout the codebase, which has been done.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the old block number fields are no longer used in the codebase. rg --type go 'ShanghaiBlock|CancunBlock'Length of output: 42
ethergo/chain/client/config.go (1)
- 339-339: The change from
params.RopstenChainConfigtoparams.SepoliaChainConfigin thechainConfigslist is a significant update that reflects a change in the default chain configuration. This change should be cross-verified to ensure that it does not affect any existing configurations or deployments that rely on the Ropsten testnet.ethergo/backends/simulated/multibackend/simulated_gen.go (5)
- 83-88: The initialization of
core.Genesishas been updated to useparams.AllEthashProtocolChanges. Verify that this change aligns with the intended protocol configurations for the simulated backend.Verification successful
The ast-grep output confirms that the
core.Genesisstruct is indeed initialized withparams.AllEthashProtocolChangesin thesimulated_gen.gofile. This matches the code snippet provided in the review comment, verifying that the change to useparams.AllEthashProtocolChangesis present in the codebase.* 532-532: The error handling in the `EstimateGas` method has been updated to return `core.ErrInsufficientFundsForTransfer` if the balance is insufficient. Ensure that this change is consistent with the error handling strategy across the entire codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that AllEthashProtocolChanges is the intended protocol configuration ast-grep --lang go --pattern $'core.Genesis{ Config: params.AllEthashProtocolChanges, $$$ }'Length of output: 522
798-802: The
AdjustTimemethod has been modified to retrieve the parent block before generating a new chain. Confirm that this change does not introduce any issues with block retrieval and time adjustment logic.849-869: The
HeaderByNumbermethod in thefilterBackendtype has been modified to handle different block numbers and return errors accordingly. Verify that the error messages and handling are consistent with the rest of the codebase.877-882: New methods
GetBody,ChainConfig, andCurrentHeaderhave been added to thefilterBackendtype. Ensure that these methods are implemented correctly and are necessary for the functionality of thefilterBackend.Verification successful
The verification scripts have confirmed the implementation of the
GetBody,ChainConfig, andCurrentHeadermethods in thefilterBackendtype within theethergo/backends/simulated/multibackend/simulated_gen.gofile.
- The
GetBodymethod is fully implemented with logic to retrieve the block body by hash and return an error if not found.- The
ChainConfigandCurrentHeadermethods are present but both contain apanic("not supported")statement, indicating they are placeholders and not yet supported.Based on this information, the new methods
GetBody,ChainConfig, andCurrentHeaderare indeed added to thefilterBackendtype, withGetBodybeing properly implemented and the other two methods currently not supported.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of new methods in filterBackend ast-grep --lang go --pattern $'func (fb *filterBackend) GetBody($_, $_, $_) ($_, error) { $$$ }' ast-grep --lang go --pattern $'func (fb *filterBackend) ChainConfig() *params.ChainConfig { $$$ }' ast-grep --lang go --pattern $'func (fb *filterBackend) CurrentHeader() *types.Header { $$$ }'Length of output: 1463
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1864 +/- ##
===================================================
+ Coverage 51.67130% 51.70116% +0.02985%
===================================================
Files 409 409
Lines 28062 28069 +7
Branches 307 307
===================================================
+ Hits 14500 14512 +12
+ Misses 12145 12143 -2
+ Partials 1417 1414 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -26,6 +26,7 @@ var ( | |||
| _ = common.Big1 | |||
| _ = types.BloomLookup | |||
| _ = event.NewSubscription | |||
| _ = abi.ConvertType | |||
There was a problem hiding this comment.
The line is commented out and seems to be unused. If abi.ConvertType is not used elsewhere in the code, consider removing this line to keep the code clean.
| @@ -26,6 +26,7 @@ var ( | |||
| _ = common.Big1 | |||
| _ = types.BloomLookup | |||
| _ = event.NewSubscription | |||
| _ = abi.ConvertType | |||
There was a problem hiding this comment.
The reference import _ = abi.ConvertType is unused in the provided code changes. If this import is no longer needed due to the changes, consider removing it to keep the code clean.
| @@ -26,6 +26,7 @@ var ( | |||
| _ = common.Big1 | |||
| _ = types.BloomLookup | |||
| _ = event.NewSubscription | |||
| _ = abi.ConvertType | |||
There was a problem hiding this comment.
The addition of unused variables _ = common.Big1, _ = types.BloomLookup, and _ = event.NewSubscription seems unnecessary. If these are not used elsewhere in the file, consider removing them to keep the code clean.
| parsed, err := AddressUpgradeableMetaData.GetAbi() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return bind.NewBoundContract(address, parsed, caller, transactor, filterer), nil | ||
| return bind.NewBoundContract(address, *parsed, caller, transactor, filterer), nil |
There was a problem hiding this comment.
The shell script output indicates that there is still a reference to abi.JSON in the file agents/contracts/summit/topics.go. This suggests that not all instances of abi.JSON have been replaced with MetaData.GetAbi().
Based on this information, the review comment is correct in asking to ensure that all instances where abi.JSON was previously used are now correctly using MetaData.GetAbi(). However, since there is still at least one occurrence of abi.JSON, the task is not fully completed.
Analysis chain
The change to use MetaData.GetAbi() in bindAddressUpgradeable is noted. Ensure that all instances where abi.JSON was previously used are now correctly using MetaData.GetAbi().
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to abi.JSON that should have been replaced.
rg --type go 'abi\.JSON' agents/contracts/summit/
Length of output: 155
| @@ -26,6 +26,7 @@ var ( | |||
| _ = common.Big1 | |||
| _ = types.BloomLookup | |||
| _ = event.NewSubscription | |||
| _ = abi.ConvertType | |||
There was a problem hiding this comment.
The search results confirm that abi.ConvertType is indeed used in various other parts of the codebase. This suggests that the function is still relevant and has not been completely deprecated or removed. The review comment about the removal of abi.ConvertType being intentional and the functionality no longer required may not be accurate, given the widespread usage of the function in other files.
Based on the provided information, it seems that the removal of abi.ConvertType in the specific instance of testbridgev1.abigen.go might be an isolated change, possibly due to a refactor or update in that particular part of the code. However, the function itself is still in use elsewhere in the codebase.
Analysis chain
The removal of abi.ConvertType is noted. Ensure that this change is intentional and that the functionality is no longer required due to the updates in the Ethereum library.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that abi.ConvertType is not used elsewhere in the codebase.
rg --type go 'abi\.ConvertType'
Length of output: 387416
| @@ -26,6 +26,7 @@ var ( | |||
| _ = common.Big1 | |||
| _ = types.BloomLookup | |||
| _ = event.NewSubscription | |||
| _ = abi.ConvertType | |||
There was a problem hiding this comment.
The search results indicate that abi.ConvertType is indeed used in various other parts of the codebase. This suggests that the function is still required and its removal in the specific instance noted in the review comment may need to be reconsidered or further justified.
Based on the provided context and the search results, it seems that the removal of abi.ConvertType in the specific instance of dfkhero.abigen.go might not be aligned with the usage pattern in the rest of the codebase, where the function is still in use.
Analysis chain
The removal of abi.ConvertType is noted. Ensure that this change is intentional and that the function is no longer required due to the updates in the ABI handling.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that abi.ConvertType is not used elsewhere in the codebase after its removal.
rg --type go 'abi\.ConvertType'
Length of output: 387416
| @@ -26,6 +26,7 @@ var ( | |||
| _ = common.Big1 | |||
| _ = types.BloomLookup | |||
| _ = event.NewSubscription | |||
| _ = abi.ConvertType | |||
There was a problem hiding this comment.
Unused variables _ = common.Big1, _ = types.BloomLookup, and _ = event.NewSubscription should be removed if they are not required, as they add unnecessary clutter to the code.
| @@ -26,6 +26,7 @@ var ( | |||
| _ = common.Big1 | |||
| _ = types.BloomLookup | |||
| _ = event.NewSubscription | |||
| _ = abi.ConvertType | |||
There was a problem hiding this comment.
Unused variables _ = common.Big1, _ = types.BloomLookup, and _ = event.NewSubscription should be removed if they are not required for the functionality.
| parsed, err := ContextUpgradeableMetaData.GetAbi() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return bind.NewBoundContract(address, parsed, caller, transactor, filterer), nil | ||
| return bind.NewBoundContract(address, *parsed, caller, transactor, filterer), nil |
There was a problem hiding this comment.
The verification process has revealed that the bindContextUpgradeable function's new implementation using ContextUpgradeableMetaData.GetAbi() is consistently applied across the codebase with one exception. In agents/contracts/notarymanager/notarymanager.abigen.go, the function still uses abi.JSON(strings.NewReader(ContextUpgradeableABI)) instead of the new method. This inconsistency should be reviewed to ensure it aligns with the intended changes of the pull request.
Analysis chain
Ensure that the bindContextUpgradeable function's new implementation is correctly handled wherever it's used.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of bindContextUpgradeable to ensure they handle the new implementation.
rg 'bindContextUpgradeable' --type go
Length of output: 23461
Script:
#!/bin/bash
# Examine the implementation of bindContextUpgradeable to understand the changes.
rg 'func bindContextUpgradeable' --type go -A 10
Length of output: 32358
| parsed, err := ContextUpgradeableMetaData.GetAbi() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return bind.NewBoundContract(address, parsed, caller, transactor, filterer), nil | ||
| return bind.NewBoundContract(address, *parsed, caller, transactor, filterer), nil |
There was a problem hiding this comment.
The broader search for occurrences of ContextUpgradeableMetaData and GetAbi in the codebase returned multiple results, confirming that the GetAbi method is indeed implemented and used in various files. This includes the specific file services/explorer/contracts/metaswap/metaswap.abigen.go mentioned in the review comment, where the GetAbi method is called on the ContextUpgradeableMetaData struct.
Given this information, the review comment stating that the changes to bindContextUpgradeable are consistent with the established pattern and the instruction to verify that the GetAbi method is correctly implemented in the ContextUpgradeableMetaData struct can be considered correct. The method is present and utilized as expected in the codebase.
Analysis chain
The changes to bindContextUpgradeable are consistent with the established pattern. Verify that the GetAbi method is correctly implemented in the ContextUpgradeableMetaData struct.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Confirm the correct implementation of GetAbi in ContextUpgradeableMetaData
ast-grep --lang go --pattern $'func ($_ *ContextUpgradeableMetaData) GetAbi() ($_, $_) { $$$ }'
Length of output: 96
Script:
#!/bin/bash
# Search for the ContextUpgradeableMetaData struct and any associated methods
rg "type ContextUpgradeableMetaData struct" -A 10
rg "func \(\*ContextUpgradeableMetaData\) GetAbi"
Length of output: 103
Script:
#!/bin/bash
# Perform a broader search for any occurrence of ContextUpgradeableMetaData and GetAbi
rg "ContextUpgradeableMetaData"
rg "GetAbi"
Length of output: 148584
Description
Upgrade Eth #1767
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Tests