refactor(core)!: clean-up core and simplify preblock #19672
Conversation
|
Warning Rate Limit Exceeded@julienrbrt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 14 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThese updates streamline error handling and module interactions across the codebase, notably shifting several functions to return only errors instead of a combination of response objects and errors. This simplification affects pre-block processing and module migration mechanisms, enhancing clarity and efficiency. Additionally, the modifications include interface refactoring within the app module system, emphasizing clearer function signatures and responsibilities, alongside adjustments in the upgrade module to align with these core changes. 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 (
|
julienrbrt
changed the title
julienrbrt
changed the title
|
we should see if we can drop gogoproto in core as well. protoiface.Messagev1 should suffice? |
| // write the consensus parameters in store to context | ||
| if rsp.ConsensusParamsChanged { |
There was a problem hiding this comment.
I like this being removed 👍
core/appmodule/module.go
Outdated
| AppModule | ||
| // HasPreBlocker is the extension interface that modules should implement to run | ||
| // custom logic before BeginBlock. | ||
| // It can modify consensus parameters in storage and signal the caller through the return value. |
There was a problem hiding this comment.
// It can modify consensus parameters in storage and signal the caller through the return value.
This is not like this anymore, should be removed right?
![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: 4
Configuration used: .coderabbit.yml
Files selected for processing (19)
- baseapp/abci_test.go (4 hunks)
- baseapp/baseapp.go (1 hunks)
- core/CHANGELOG.md (1 hunks)
- core/appmodule/migrations.go (1 hunks)
- core/appmodule/module.go (3 hunks)
- core/appmodule/v2/migrations.go (2 hunks)
- core/appmodule/v2/module.go (1 hunks)
- runtime/app.go (1 hunks)
- runtime/services/autocli.go (1 hunks)
- simapp/app.go (1 hunks)
- testutil/mock/types_mock_appmodule.go (1 hunks)
- types/abci.go (2 hunks)
- types/module/configurator.go (4 hunks)
- types/module/module.go (1 hunks)
- types/module/module_test.go (1 hunks)
- x/upgrade/CHANGELOG.md (1 hunks)
- x/upgrade/keeper/abci.go (7 hunks)
- x/upgrade/keeper/abci_test.go (11 hunks)
- x/upgrade/module.go (1 hunks)
Additional comments: 58
core/appmodule/migrations.go (1)
- 3-17: The introduction of type aliases from
cosmossdk.io/core/appmodule/v2enhances clarity and maintainability by providing a direct reference to the types related to module migrations and consensus versions. This change aligns with the PR's objective of simplifying and cleaning up the core components.core/appmodule/v2/migrations.go (1)
- 11-17: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [14-23]
The refinement of the
HasMigrationsinterface to include theMigrationRegistrarfor registering in-place store migrations enhances the module's upgrade capabilities. This change supports the PR's goal of improving the structure and organization of module migrations and consensus version handling.types/abci.go (1)
- 39-39: Modifying the
PreBlockerfunction signature to return only an error simplifies error handling across different parts of the SDK. This change aligns with the PR's objective of standardizing thePreBlockfunction's behavior and simplifying error handling.x/upgrade/CHANGELOG.md (2)
- 28-31: The changelog entry correctly highlights the simplification of the
PreBlockfunctionality as per the latest changes in thecosmossdk.io/corepackage. This entry provides clear information to users about the improvements made.- 38-38: The changelog entry for the modification of the
NewKeeperfunction to take anappmodule.Environmentinstead of individual services accurately reflects the changes made to improve the module's flexibility and maintainability.core/appmodule/v2/module.go (1)
- 22-30: The addition of the
HasPreBlockerinterface, which allows modules to execute custom logic beforeBeginBlock, enhances the flexibility and modularity of the module's functionality. This change aligns with the PR's objective of simplifying and cleaning up the core components.x/upgrade/keeper/abci.go (1)
- 84-106: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [24-115]
Refactoring the
PreBlockermethod to return only an error simplifies the upgrade process by focusing on error propagation and handling, removing unnecessary complexity. This change aligns with the PR's objective of simplifying the Preblock functionality and improving the structure and organization of module migrations and consensus version handling.x/upgrade/module.go (1)
- 154-154: Modifying the
PreBlockfunction to return only an error simplifies the control flow and error handling in the pre-block phase. This change aligns with the PR's objective of simplifying the Preblock functionality and enhancing the maintainability and clarity of the Cosmos SDK's core components.types/module/configurator.go (3)
- 12-12: The addition of
"cosmossdk.io/core/appmodule"import is appropriate given the change in the method signature ofRegisterto useappmodule.MigrationHandler. This aligns with the PR's objective of simplifying and cleaning up core components by using more specific types.- 50-50: The update to the
Registermethod's signature in theConfiguratorinterface to useappmodule.MigrationHandlerinstead offunc(context.Context) errorfor thehandlerparameter is a positive change. It enhances clarity and maintainability by leveraging a more specific type that better represents the intended functionality. This change is consistent with the PR's objectives of simplifying and improving the SDK's core components.- 127-127: The implementation of the updated
Registermethod in theconfiguratorstruct correctly adapts theappmodule.MigrationHandlerto the existingRegisterMigrationmethod by wrapping the handler in a function that matches the expected signature. This maintains compatibility and ensures that the new method signature can be integrated seamlessly with the existing migration logic. It's a good example of adapting interfaces to enhance code modularity and maintainability.core/CHANGELOG.md (1)
- 59-59: The entry for PR refactor(core)!: clean-up core and simplify preblock #19672 correctly summarizes the change to
PreBlockto return only an error. This aligns with the PR's objective to simplify the PreBlock functionality.runtime/app.go (1)
- 156-156: The modification of the
PreBlockermethod signature to return only anerroris consistent with the PR's objective to simplify the PreBlock functionality. This change should streamline error handling across the SDK.x/upgrade/keeper/abci_test.go (17)
- 48-48: The change from ignoring the error to explicitly handling it by assigning it to
erris a good practice as it ensures that errors are not silently ignored and are properly checked.- 56-56: Correctly handling the error by assigning it to
errand usingrequire.NoErrorto assert that no error occurred is a good practice. This ensures that the test fails if an unexpected error is encountered.- 66-66: Assigning the error to
errand checking it withrequire.ErrorContainsis a good practice. It ensures that the specific expected error is returned, which is crucial for validating the behavior of thePreBlockfunction under test conditions.- 74-74: Properly handling the error by assigning it to
errand asserting no error withrequire.NoErroris correct. This change ensures that the test accurately reflects the expected behavior of thePreBlockfunction after the upgrade handler is set.- 178-178: Assigning the error to
errand asserting no error withrequire.NoErroris appropriate. It verifies that thePreBlockfunction behaves as expected when there is no upgrade plan registered in the database, which is a valid test scenario.- 185-185: The explicit error handling by assigning the result to
errand usingrequire.EqualErrorto check for a specific error message is a good practice. It ensures that the test accurately captures the expected behavior when a binary update occurs before the trigger.- 192-192: Properly handling the error by assigning it to
errand asserting no error withrequire.NoErroris correct. This ensures that the test accurately reflects the expected behavior of thePreBlockfunction when the upgrade plan is on time.- 226-226: Correctly handling the error by assigning it to
errand usingrequire.NoErrorto assert that no error occurred is a good practice. This ensures that the test fails if an unexpected error is encountered, which is crucial for validating the behavior of thePreBlockfunction in scenarios where no upgrade has been scheduled.- 263-263: Assigning the error to
errand asserting no error withrequire.NoErroris appropriate. It verifies that thePreBlockfunction behaves as expected when the upgrade plan is cleared due to the skip upgrade flag, which is a valid test scenario.- 271-271: Properly handling the error by assigning it to
errand asserting no error withrequire.NoErroris correct. This change ensures that the test accurately reflects the expected behavior of thePreBlockfunction after clearing a second upgrade plan due to the skip upgrade flag.- 298-298: Assigning the error to
errand asserting no error withrequire.NoErroris appropriate. It verifies that thePreBlockfunction behaves as expected when the upgrade plan is cleared in one case and proceeds with the upgrade in another, which is a valid test scenario.- 331-331: Properly handling the error by assigning it to
errand asserting no error withrequire.NoErroris correct. This ensures that the test accurately reflects the expected behavior of thePreBlockfunction when clearing upgrade plans due to the skip upgrade flag.- 339-339: Assigning the error to
errand asserting no error withrequire.NoErroris appropriate. It verifies that thePreBlockfunction behaves as expected when clearing a second upgrade plan due to the skip upgrade flag, which is a valid test scenario.- 360-360: Correctly handling the error by assigning it to
errand usingrequire.ErrorContainsto assert that a specific error occurred is a good practice. This ensures that the test accurately captures the expected behavior when an upgrade is needed at a specific height.- 450-450: Assigning the error to
errand conditionally checking it based on theexpectErrorflag is a good practice. It ensures that the test accurately reflects the expected behavior of thePreBlockfunction under various scenarios, including both error and no-error conditions.- 489-489: Properly handling the error by assigning it to
errand asserting no error withrequire.NoErroris correct. This ensures that the test accurately reflects the expected behavior of thePreBlockfunction after a successful upgrade.- 539-539: Correctly handling the error by assigning it to
errand conditionally checking it based on theexpectErrorflag is a good practice. It ensures that the test accurately captures the expected behavior under various downgrade verification scenarios.types/module/module_test.go (2)
- 421-422: The modification to the
PreBlockfunction to directly handle errors and the corresponding adjustment in the test to assert no error withrequire.NoErroris a good practice. It simplifies the function's signature and focuses on error handling, which is crucial for the correctness of the test.- 427-427: Properly handling the error by assigning it to
errand usingrequire.EqualErrorto check for a specific error message is a good practice. It ensures that the test accurately captures the expected behavior when an error is returned by thePreBlockfunction.testutil/mock/types_mock_appmodule.go (1)
- 662-666: The change in the
PreBlockmethod's return type from(appmodule.ResponsePreBlock, error)to justerroraligns with the PR's objective of simplifying the PreBlock functionality. This should help in streamlining error handling and reducing complexity. However, it's important to verify that this change does not adversely affect error handling patterns or the usage of this method across the SDK.types/module/module.go (1)
- 709-718: The changes to the
PreBlockfunction, including the updated return type and the adjusted error handling logic, align with the PR's objectives to simplify the PreBlock functionality. This simplification should make the code more maintainable and easier to understand. Ensure that all modules implementingappmodule.HasPreBlockercorrectly handle errors in theirPreBlockimplementations.simapp/app.go (1)
- 618-618: The
PreBlockerfunction's signature has been simplified to return only anerror, aligning with the PR's objective to simplify the PreBlock functionality. This change standardizes error handling across different parts of the SDK and reduces complexity. Ensure that all calls toPreBlockeracross the SDK have been updated to handle the new signature correctly.baseapp/baseapp.go (1)
- 709-718: The modifications to the
preBlockfunction enhance clarity and ensure that the context is updated with the latest consensus parameters and gas metering. This is crucial for maintaining the integrity of the blockchain state and ensuring that transactions are processed correctly with respect to the current consensus rules. The error handling and context updates are implemented correctly and follow best practices for state management within the Cosmos SDK'sBaseApp.One minor suggestion is to ensure that any potential side effects of
app.preBlocker(ctx, req)are well-documented, especially since the context is being updated immediately after. It's important for future maintainers to understand why these updates are necessary and how they affect the application's state.Overall, the changes are well-implemented and contribute to the robustness of the
preBlockfunctionality.baseapp/abci_test.go (22)
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [11-59]
Looks good! The test
TestABCI_Infois well-structured and covers the expected behavior of theInfoABCI call comprehensively.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [61-76]
The test
TestABCI_First_block_Heightcorrectly verifies the initial block height setting during theInitChaincall.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [78-169]
The test
TestABCI_InitChainis comprehensive and effectively tests various scenarios related to theInitChaincall, including error handling and state initialization.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [171-187]
The test
TestABCI_InitChain_WithInitialHeightcorrectly verifies the behavior of setting an initial block height during theInitChaincall.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [189-210]
The test
TestABCI_FinalizeBlock_WithInitialHeighteffectively verifies the behavior of finalizing a block with a specific initial height, including error handling.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [212-261]
The test
TestABCI_FinalizeBlock_WithBeginAndEndBlockercorrectly verifies the behavior of customBeginBlockerandEndBlockerfunctions during theFinalizeBlockcall, including event generation.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [263-366]
The test
TestABCI_ExtendVoteis comprehensive and effectively tests various scenarios related to vote extension and verification, including error handling.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [368-413]
The test
TestABCI_OnlyVerifyVoteExtensioncorrectly verifies the behavior of verifying vote extensions independently of theExtendVotecall.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [415-454]
The test
TestABCI_GRPCQueryeffectively verifies the behavior of executing a gRPC query through the ABCI interface, including error handling and response validation.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [456-485]
The test
TestABCI_P2PQuerycorrectly verifies the behavior of executing P2P queries through the ABCI interface, including custom filter handling and response validation.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [487-507]
The test
TestBaseApp_PrepareCheckStateeffectively verifies that thePrepareCheckStatehook is called with the correct context.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [509-529]
The test
TestBaseApp_Precommiteffectively verifies that thePrecommithook is called with the correct context.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [531-597]
The test
TestABCI_CheckTxis comprehensive and effectively tests various scenarios related to theCheckTxcall, including persistence of changes and error handling.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [599-660]
The test
TestABCI_FinalizeBlock_DeliverTxis comprehensive and effectively tests various scenarios related to theFinalizeBlockcall, including transaction delivery and event generation.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [662-724]
The test
TestABCI_FinalizeBlock_MultiMsgeffectively verifies the behavior of delivering a transaction with multiple messages during theFinalizeBlockcall, including handling of different message types.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [726-789]
The test
TestABCI_Query_SimulateTxis comprehensive and effectively tests the behavior of simulating a transaction through the ABCIQuerycall, including gas consumption reporting and response validation.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [791-870]
The test
TestABCI_InvalidTransactionis comprehensive and effectively tests various scenarios related to handling invalid transactions during theFinalizeBlockcall, including error handling and response validation.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [872-944]
The test
TestABCI_TxGasLimitsis comprehensive and effectively tests various scenarios related to enforcing gas limits on transactions during theFinalizeBlockcall, including error handling and response validation.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [946-1008]
The test
TestABCI_MaxBlockGasLimitsis comprehensive and effectively tests various scenarios related to enforcing maximum block gas limits during theFinalizeBlockcall, including error handling and response validation.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1010-1037]
The test
TestABCI_GasConsumptionBadTxcorrectly verifies the behavior of gas consumption for transactions that fail during theFinalizeBlockcall, including handling of different failure scenarios.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1039-1071]
The test
TestABCI_Queryeffectively verifies the behavior of executing a query through the ABCI interface, including setting and retrieving values from the store.
- 2041-2049: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1073-1122]
The test
TestABCI_GetBlockRetentionHeightis comprehensive and effectively tests various scenarios for calculating block retention height based on different parameters.
| ### API Breaking Changes | ||
|
|
||
| * [#19672](https://github.com/cosmos/cosmos-sdk/pull/19672) `PreBlock` now returns only an error for consistency with server/v2. The SDK has upgraded x/upgrade accordingly. | ||
| * [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` to `x/tx`. |
There was a problem hiding this comment.
The URL for PR #18861 contains a typo: httpes://github.com/cosmos/cosmos-sdk/pull/18861. It should be https://.
- * [#18861](httpes://github.com/cosmos/cosmos-sdk/pull/18861) Moved `coin.ParseCoin` to `client/v2/internal`.
+ * [#18861](https://github.com/cosmos/cosmos-sdk/pull/18861) Moved `coin.ParseCoin` to `client/v2/internal`.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.
| * [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` to `x/tx`. | |
| * [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` to `x/tx`. | |
| * [#18861](https://github.com/cosmos/cosmos-sdk/pull/18861) Moved `coin.ParseCoin` to `client/v2/internal`. |
| * Add `MsgHandler` as an alternative to grpc handlers | ||
| * Provide separate `MigrationRegistrar` instead of grouping with `RegisterServices` | ||
|
|
||
| ### Improvements | ||
|
|
||
| ### API Breaking Changes | ||
|
|
||
| * [#19672](https://github.com/cosmos/cosmos-sdk/pull/19672) `PreBlock` now returns only an error for consistency with server/v2. The SDK has upgraded x/upgrade accordingly. | ||
| * [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` to `x/tx`. | ||
| * [#18861](httpes://github.com/cosmos/cosmos-sdk/pull/18861) Moved `coin.ParseCoin` to `client/v2/internal`. | ||
| * [#18866](https://github.com/cosmos/cosmos-sdk/pull/18866) All items related to depinject have been moved to `cosmossdk.io/depinject` (`Provide`, `Invoke`, `Register`) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [42-42]
There's a missing space after the period in the entry for PR #18457. This is a minor formatting issue but important for readability.
- * [#18457](https://github.com/cosmos/cosmos-sdk/pull/18457) Add branch.ExecuteWithGasLimit.
+ * [#18457](https://github.com/cosmos/cosmos-sdk/pull/18457) Add branch.ExecuteWithGasLimit.
core/appmodule/module.go
Outdated
| // HasPreBlocker is the extension interface that modules should implement to run | ||
| // custom logic before BeginBlock. | ||
| // It can modify consensus parameters in storage and signal the caller through the return value. | ||
| // The new context (ctx) must be passed to all the other lifecycle methods. | ||
| type HasPreBlocker = appmodule.HasPreBlocker | ||
|
|
||
| // RegisterMigrations registers the module's migrations with the app's migrator. | ||
| RegisterMigrations(MigrationRegistrar) error | ||
| } | ||
| // HasBeginBlocker is the extension interface that modules should implement to run | ||
| // custom logic before transaction processing in a block. | ||
| type HasBeginBlocker = appmodule.HasBeginBlocker | ||
|
|
||
| // HasEndBlocker is the extension interface that modules should implement to run | ||
| // custom logic after transaction processing in a block. | ||
| type HasEndBlocker = appmodule.HasEndBlocker |
There was a problem hiding this comment.
The removal of the HasMigrations interface and the addition of HasBeginBlocker and HasEndBlocker interfaces are significant changes that enhance the modularity and clarity of the module's functionality. These changes align with the PR's objective of simplifying and cleaning up the core components. However, ensure that the removal of HasMigrations does not impact modules that might rely on this interface for migration logic.
| func (a *autocliConfigurator) Register(string, uint64, appmodule.MigrationHandler) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Updating the Register method to take a MigrationHandler type for the third parameter simplifies the method signature and potentially aligns with the broader refactoring efforts to streamline error handling and module migrations. Ensure this change is consistently applied across all relevant parts of the SDK where the Register method is used.
Looks like we still need to import it for: cosmos-sdk/core/appmodule/v2/message.go Line 12 in f2ef59a I would guess it is fine? I cannot access revProtoTypes from gogoproto without importing it.
|
Description
Further clean-up of core after #19652 and #19627 gets merged.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
PreBlockfunction across various components to return only an error, enhancing consistency and error handling.HasBeginBlockerandHasEndBlocker) in the core app module.PreBlockfunctionality and other significant updates.