feat(abci): add a custom process proposal #1286
Conversation
WalkthroughThe changes primarily revolve around the introduction of a new configuration option Changes
TipsChat with CodeRabbit Bot (
|
![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: 2
Configuration used: CodeRabbit UI
Files selected for processing (8)
- cosmos/config/config.go (1 hunks)
- cosmos/config/flags/flags.go (1 hunks)
- cosmos/config/template.go (1 hunks)
- cosmos/x/evm/genesis_test.go (1 hunks)
- eth/core/chain.go (3 hunks)
- eth/core/chain_writer.go (2 hunks)
- eth/polar/backend.go (1 hunks)
- eth/polar/config.go (1 hunks)
Files skipped from review due to trivial changes (3)
- cosmos/config/flags/flags.go
- cosmos/config/template.go
- eth/polar/config.go
Additional comments: 6
cosmos/config/config.go (1)
- 83-93: The new configuration option
IsMainnetis being read from the parser and assigned to thePolarstruct in theConfigstruct. If the flag is not found or there is an error retrieving its value, an error is returned. This is a good practice as it allows the function to fail early and propagate the error up the call stack.eth/polar/backend.go (1)
- 114-117: The
core.NewChainfunction now accepts an additionalisMainnetparameter. Ensure that the function definition and all calls to this function throughout the codebase have been updated to match the new signature.- blockchain: core.NewChain(host, &config.Chain, engine), + blockchain: core.NewChain(host, &config.Chain, engine, config.IsMainnet),cosmos/x/evm/genesis_test.go (1)
- 83-87: Ensure that all calls to
core.NewChainthroughout the codebase have been updated to match the new signature. Also, consider the implications of hardcodingtruefor theisMainnetparameter. If this test is meant to represent a mainnet scenario, this is fine. However, if you want to test both mainnet and testnet scenarios, you might want to parameterize this value.eth/core/chain.go (3)
102-105: The
isMainnetfield has been added to theblockchainstruct. This field will be used to determine whether the blockchain is running on the mainnet or not.114-115: The
NewChainfunction now accepts an additionalisMainnetparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.130-131: The
isMainnetfield is being set in theNewChainfunction. This is the correct place to set this field as it is part of the initialization of theblockchainstruct.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1286 +/- ##
==========================================
- Coverage 48.73% 48.61% -0.13%
==========================================
Files 84 84
Lines 4838 4850 +12
==========================================
Hits 2358 2358
- Misses 2306 2318 +12
Partials 174 174
|
itsdevbear
changed the title
IsMainnet
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- cosmos/go.mod
- cosmos/go.sum
Files selected for processing (17)
- cosmos/config/config.go (1 hunks)
- cosmos/runtime/ante/ante.go (1 hunks)
- cosmos/runtime/chain/abci.go (1 hunks)
- cosmos/runtime/chain/chain.go (1 hunks)
- cosmos/runtime/chain/interfaces.go (1 hunks)
- cosmos/runtime/miner/abci.go (1 hunks)
- cosmos/runtime/miner/interfaces.go (1 hunks)
- cosmos/runtime/miner/miner.go (2 hunks)
- cosmos/runtime/runtime.go (4 hunks)
- cosmos/runtime/txpool/handler_test.go (2 hunks)
- cosmos/runtime/txpool/mempool_test.go (1 hunks)
- cosmos/x/evm/genesis_test.go (2 hunks)
- cosmos/x/evm/keeper/abci.go (1 hunks)
- cosmos/x/evm/keeper/genesis.go (2 hunks)
- cosmos/x/evm/keeper/keeper.go (3 hunks)
- cosmos/x/evm/keeper/processor.go (1 hunks)
- eth/core/chain_writer.go (2 hunks)
Files skipped from review due to trivial changes (7)
- cosmos/config/config.go
- cosmos/runtime/ante/ante.go
- cosmos/runtime/chain/abci.go
- cosmos/runtime/miner/abci.go
- cosmos/runtime/txpool/handler_test.go
- cosmos/runtime/txpool/mempool_test.go
- eth/core/chain_writer.go
Additional comments: 30
cosmos/x/evm/keeper/processor.go (1)
- 65-71: The
InsertBlockAndSetHeadfunction is now being used instead ofPreparePlugins. Ensure that theInsertBlockAndSetHeadfunction performs all the necessary operations that were previously done byPreparePlugins. Also, ensure that the error handling is adequate and that the function behaves as expected in all scenarios.cosmos/x/evm/keeper/abci.go (1)
- 32-38: The
GetBlockByNumbermethod is now called onk.wrappedChaininstead ofk.chain. Ensure thatk.wrappedChainis correctly initialized and that it has the same behavior ask.chainin this context.- block := k.chain.GetBlockByNumber(blockNum) + block := k.wrappedChain.GetBlockByNumber(blockNum)cosmos/runtime/chain/interfaces.go (3)
1-19: The license header is included correctly. Ensure that the license is compatible with the project's license.
21-27: The package and import statements are correct.
29-33: The
Appinterface is defined correctly with four methods. Ensure that all implementations of this interface throughout the codebase have been updated to include these methods.cosmos/x/evm/keeper/genesis.go (2)
31-37: The
genState.Configassignment has been updated to usek.wrappedChain.Config(). Ensure thatk.wrappedChain.Config()is correctly initialized and has the expected values before this assignment.47-47: The
k.chain.WriteGenesisBlockfunction has been replaced withk.wrappedChain.WriteGenesisState. Ensure thatk.wrappedChain.WriteGenesisStateis correctly implemented and that it correctly writes the genesis state.cosmos/runtime/miner/interfaces.go (2)
1-19: Ensure that the license is appropriate for your project and that it is correctly referenced. If the license is not appropriate, you may need to update it.
35-53: The interfaces and types defined here look good. They are well-documented and seem to cover the necessary functionality for the miner application. However, ensure that all methods defined in the interfaces are implemented wherever these interfaces are used.
cosmos/x/evm/genesis_test.go (2)
34-38: The new import statement for the package "pkg.berachain.dev/polaris/cosmos/runtime/chain" is introduced correctly. Ensure that the package is available and accessible.
84-88: The Setup function call is modified correctly to pass a new argument "chain.New(core.NewChain(k.Host, params.DefaultChainConfig, beacon.NewFaker()), nil)" instead of "core.NewChain(k.Host, params.DefaultChainConfig, beacon.NewFaker())". Ensure that the function signatures for
chain.Newandcore.NewChainare updated accordingly throughout the codebase.cosmos/runtime/runtime.go (4)
30-30: Ensure that the newly added import is used in the code. Unused imports can lead to compilation errors.
62-68: The
CosmosAppinterface has been extended with new methods. Make sure all implementations of this interface are updated to include these new methods.77-85: The
Polarisstruct has been extended with a new fieldWrappedBlockchain. Ensure that all instances ofPolarisare updated to include this new field.113-126: The
Buildmethod of thePolarisstruct has been updated to include the initialization ofWrappedBlockchain. Ensure that all calls to this method are updated to handle the new field.cosmos/x/evm/keeper/keeper.go (4)
31-38: The import statements look fine. Ensure that all the imported packages are being used in the code.
40-46: The
WrappedBlockchaininterface is well defined. It encapsulates the methods related to blockchain operations.50-54: The
Keeperstruct has been updated to use theWrappedBlockchaininterface. This change should be reflected wherever theKeeperstruct is used.73-79: The
Setupfunction now accepts aWrappedBlockchainparameter and assigns it to thewrappedChainfield of theKeeperstruct. Ensure that all calls to this function have been updated to match the new signature.cosmos/runtime/miner/miner.go (4)
24-35: The import statements are well organized and separated by functionality. Good practice.
37-45: The
Minerstruct has been updated to include achainfield of typecore.Blockchain. This is in line with the pull request summary.48-55: The
Newfunction now accepts an additional parameterchain core.Blockchainand assigns it to thechainfield of theMinerstruct. This is in line with the pull request summary.59-64: The
buildBlockfunction seems to be unchanged. It's not clear why this hunk is included in the diff. If there are no changes, it can be ignored.cosmos/runtime/chain/chain.go (7)
1-19: The license header is well formatted and clear. It's good to see that the copyright and license information is included at the top of the file.
23-28: The import statements are well organized and correctly formatted.
30-36: The
WrappedBlockchainstruct is well defined and includes the necessary fields. The comments provide a clear explanation of the struct and its fields.38-42: The
Newfunction is correctly implemented and returns a new instance ofWrappedBlockchain. The function signature is clear and the function body is concise.44-51: The
WriteGenesisStatemethod is correctly implemented. It prepares the plugins and writes the genesis block to the blockchain. The function signature is clear and the function body is concise.53-60: The
InsertBlockAndSetHeadmethod is correctly implemented. It prepares the plugins and inserts a block into the blockchain, setting it as the head. The function signature is clear and the function body is concise.62-69: The
InsertBlockWithoutSetHeadmethod is correctly implemented. It prepares the plugins and inserts a block into the blockchain without setting it as the head. The function signature is clear and the function body is concise.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (3)
- cosmos/runtime/chain/abci.go (1 hunks)
- cosmos/runtime/miner/abci.go (1 hunks)
- cosmos/x/evm/plugins/state/plugin.go (1 hunks)
Additional comments: 9
cosmos/runtime/miner/abci.go (7)
1-19: The license header is correct and up-to-date.
38-82: The function
PrepareProposalhas been updated to include new logic for preparing a proposal. It now runs thePreBlockerandBeginBlockerfunctions, refunds gas, sets gas configurations, and triggers thebuildBlockfunction. Ensure that these changes are compatible with the rest of the codebase and that all necessary tests have been updated or added to reflect these changes.46-59: The error handling for the
PreBlockerandBeginBlockerfunctions is correct. If either function returns an error, thePrepareProposalfunction will immediately return the error.61-66: The gas refunding and configuration setting logic seems correct. However, it's important to ensure that the gas configurations are being set correctly and that the refunding of gas is not causing any unexpected behavior.
68-73: The context is being set on the state plugin. If the
SetLatestQueryContextfunction returns an error, thePrepareProposalfunction will immediately return the error. This is correct error handling.75-78: The
buildBlockfunction is being triggered and its error is being handled correctly. If thebuildBlockfunction returns an error, thePrepareProposalfunction will immediately return the error.80-81: The function returns the payload as a transaction in the proposal. This seems correct, but ensure that this is the expected behavior and that the payload is being correctly included in the proposal.
cosmos/runtime/chain/abci.go (2)
38-115: The
ProcessProposalfunction seems to be handling errors correctly and returning appropriate responses. However, it's important to ensure that all the functions it calls (PreBlocker,BeginBlocker,TxDecode,ExecutableDataToBlock,InsertBlockWithoutSetHead) are also handling their errors correctly. If any of these functions can panic, it would be a good idea to add adeferstatement with arecoverfunction at the beginning ofProcessProposalto handle any panics and prevent the entire program from crashing.88-93: The function assumes that the proposal must contain an Ethereum envelope and rejects the proposal if it does not. This may not be the desired behavior if the system is expected to handle proposals that do not contain Ethereum envelopes. If this is the case, the function should be updated to handle such proposals appropriately.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- cosmos/runtime/chain/abci.go (1 hunks)
Additional comments: 3
cosmos/runtime/chain/abci.go (3)
45-62: The previous comment about checking for nil before calling
PreBlockerandBeginBlockeris still valid. This will prevent potential runtime panics.95-102: The previous comment about checking the success of the conversion from
ExecutionPayloadEnvelopeto a block is still valid. This will prevent potential errors when inserting the block into the chain.104-110: The previous comment about checking the success of the block insertion into the chain is still valid. This will ensure the response accurately reflects the outcome of the proposal processing.
Summary by CodeRabbit