feat: unify version modifier for v2 #21508
Conversation
randygrok
requested review from
a team,
testinginprod,
raynaudoe,
kocubinski,
julienrbrt and
hieuvubk
as code owners
WalkthroughWalkthroughThe changes involve updates across multiple files, primarily enhancing version management, refining gas limit handling, and improving testing capabilities. Key modifications include the introduction of a new Changes
Assessment against linked issues
Possibly related issues
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:x/consensus
C:server/v2
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
server/v2/stf/go.sumis excluded by!**/*.sum
Files selected for processing (6)
- core/server/app.go (1 hunks)
- runtime/v2/module.go (3 hunks)
- server/v2/stf/core_branch_service.go (2 hunks)
- server/v2/stf/core_branch_service_test.go (3 hunks)
- server/v2/stf/go.mod (1 hunks)
- x/consensus/depinject.go (3 hunks)
Files skipped from review due to trivial changes (2)
- core/server/app.go
- runtime/v2/module.go
Additional context used
Path-based instructions (3)
server/v2/stf/core_branch_service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/consensus/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/core_branch_service_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (12)
server/v2/stf/go.mod (4)
10-10: LGTM!The addition of the
testifydependency is approved. It's a widely used testing library in Go and will enhance the module's testing capabilities.
15-15: LGTM!The addition of the indirect dependency on
go-spewis approved. It's a useful package for debugging Go data structures and is likely required by one of the direct dependencies.
17-17: LGTM!The addition of the indirect dependency on
go-difflibis approved. It's a useful package for computing differences between Go structures and is likely required by one of the direct dependencies, such astestify.
19-19: LGTM!The addition of the indirect dependency on
yaml.v3is approved. It's a useful package for handling YAML data in Go and is likely required by one of the direct dependencies.server/v2/stf/core_branch_service.go (2)
44-44: LGTM!The code change simplifies the logic for setting the gas limit by directly using the remaining gas value from the original gas meter. It does not introduce any correctness issues.
65-66: LGTM!The addition of the
msgRouterandqueryRouterfields to thebranchedCtxcontext enhances its capabilities for handling messages and queries. This change suggests an expansion of the service's ability to process tasks and does not introduce any correctness issues.x/consensus/depinject.go (3)
4-8: LGTM!The imports are correctly added and follow the Uber Golang style guide.
27-27: LGTM!The registration of the
ProvideAppVersionModifierfunction is correctly added and follows the Uber Golang style guide.
77-110: LGTM!The
versionModifierstruct, its methods, and theProvideAppVersionModifierfunction are correctly implemented and follow the Uber Golang style guide. The error handling in theSetAppVersionandAppVersionmethods is also correctly implemented.server/v2/stf/core_branch_service_test.go (3)
9-9: LGTM!Adding the
requirepackage from thetestifylibrary is a good practice for writing more expressive and readable tests.
75-75: LGTM!Capturing the remaining gas before executing the function is necessary for asserting the gas consumption later. The variable name
originalGasclearly conveys the purpose of the variable.
86-86: LGTM!The assertion ensures that the gas used during the execution matches the difference between the original gas and the remaining gas. This is important for validating the gas accounting when the state reverts due to an error.
julienrbrt
added
the
backport/v0.52.x
|
Adding the backport label as there are changes in a module. |
julienrbrt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
server/v2/stf/core_branch_service.go (1)
65-66: Provide more information about the new context fields.The
msgRouterandqueryRouterfields have been added to the context structure in theexecutemethod. These fields suggest an enhancement in the context's capabilities, potentially enabling more complex interactions within the service.To improve code clarity and maintainability, consider adding comments or documentation explaining the purpose and usage of these new fields. This will help future developers understand their role and how they contribute to the overall functionality of the
BranchService.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
server/v2/stf/go.sumis excluded by!**/*.sum
Files selected for processing (6)
- core/server/app.go (1 hunks)
- runtime/v2/module.go (3 hunks)
- server/v2/stf/core_branch_service.go (2 hunks)
- server/v2/stf/core_branch_service_test.go (3 hunks)
- server/v2/stf/go.mod (1 hunks)
- x/consensus/depinject.go (3 hunks)
Files skipped from review due to trivial changes (2)
- core/server/app.go
- runtime/v2/module.go
Additional context used
Path-based instructions (3)
server/v2/stf/core_branch_service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/consensus/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/core_branch_service_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (12)
server/v2/stf/go.mod (4)
10-10: LGTM!Adding the
testifytesting framework is a good practice to improve the quality of the codebase. The version specified is the latest stable version at the time of this review.
15-15: LGTM!The
go-spewlibrary is likely a transitive dependency oftestify. The version specified is the latest stable version at the time of this review. Marking the dependency as indirect is correct since it is not directly used by the module.
17-17: LGTM!The
go-diffliblibrary is likely a transitive dependency oftestify. The version specified is the latest stable version at the time of this review. Marking the dependency as indirect is correct since it is not directly used by the module.
19-19: LGTM!The
yaml.v3library is likely a transitive dependency oftestify. The version specified is the latest stable version at the time of this review. Marking the dependency as indirect is correct since it is not directly used by the module.server/v2/stf/core_branch_service.go (1)
44-44: Verify the impact of the gas limit calculation change.The gas limit calculation has been simplified to directly use the remaining gas value. This change may affect how gas limits are handled during execution.
Ensure that this change does not introduce any unintended side effects or break existing functionality. Thoroughly test the gas limit handling in various scenarios to validate the correctness of the new approach.
To verify the impact, run the following script:
Verification successful
Verify the impact of the gas limit calculation change through existing tests.
The
ExecuteWithGasLimitmethod is used in various parts of the codebase, and there are multiple test cases that cover different scenarios related to gas limits. These tests should help ensure that the change in gas limit calculation does not introduce any unintended side effects. Please verify the test results to confirm the correctness of the new approach.
- Test files:
server/v2/stf/core_branch_service_test.go,runtime/branch_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the gas limit calculation change. # Test 1: Search for the `ExecuteWithGasLimit` method usage. # Expect: Identify the code paths that invoke this method. rg --type go -A 5 $'ExecuteWithGasLimit' # Test 2: Search for test cases related to gas limit handling. # Expect: Identify existing test cases that cover gas limit scenarios. rg --type go -A 5 $'ExecuteWithGasLimit' $(fd _test.go$)Length of output: 34917
x/consensus/depinject.go (4)
77-110: LGTM!The code segment follows the Uber Go Style Guide and is well-structured. The
versionModifiertype and its methods provide a clean way to manage the application version. TheProvideAppVersionModifierfunction acts as a dependency injection provider, which is a good practice.
82-97: LGTM!The
SetAppVersionmethod is implemented correctly and handles errors appropriately. It follows the Uber Go Style Guide.
99-106: LGTM!The
AppVersionmethod is implemented correctly and handles errors appropriately. It follows the Uber Go Style Guide.
27-27: LGTM!The registration of the
ProvideAppVersionModifierfunction as a dependency injection provider for the consensus module is done correctly and follows the established pattern in the file.server/v2/stf/core_branch_service_test.go (3)
9-9: LGTM!The import statement for the
requirepackage is approved. It is a common assertion library used in Go testing and facilitates more expressive assertions within the test cases.
75-75: LGTM!The introduction of the
originalGasvariable is approved. It is crucial for validating the gas consumption during the execution of theExecuteWithGasLimitmethod.
86-86: LGTM!The assertion is approved. It enhances the test's robustness by ensuring that the gas accounting is accurate, thereby improving the reliability of the test outcomes.
The previous comment by tac0turtle is no longer applicable as the assertion is now using the
requirepackage.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- server/v2/stf/core_branch_service_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/stf/core_branch_service_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- baseapp/baseapp.go (3 hunks)
- baseapp/options.go (3 hunks)
- runtime/module.go (3 hunks)
- simapp/app.go (1 hunks)
Additional context used
Path-based instructions (4)
runtime/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/options.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/baseapp.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (8)
runtime/module.go (2)
Line range hint
4-4: Approved import removal.The removal of the
serverpackage import is consistent with the removal of theProvideAppVersionModifierfunction that returned aserver.VersionModifier. This change suggests that the module no longer depends on server-related functionalities.
Line range hint
81-83: Verify the impact of theProvideAppVersionModifierfunction removal on the application lifecycle.The removal of the
ProvideAppVersionModifierfunction indicates a possible refactoring or a decision to manage application versioning differently. This change may affect how versioning is integrated into the application lifecycle.Please provide more context on how the application versioning will be handled after this change. Also, ensure that the removal of this function does not break any existing functionality.
baseapp/options.go (2)
329-335: LGTM!The addition of the
SetVersionModifiermethod is a good design choice. It allows for greater flexibility in managing versioning within the application. The check to prevent calling the function on a sealedBaseAppis also a good defensive programming practice.
163-167: Verify the usage ofSetAppVersionin the codebase.The implementation logic of
SetAppVersionhas been significantly modified. It now uses aversionModifierinstead of aparamStoreto set the app version.Please run the following script to verify the usage of
SetAppVersionacross the codebase:If the verification passes and no issues are found, the changes can be approved.
Verification successful
The usage of
SetAppVersionis consistent and correct.The
SetAppVersionfunction is used appropriately across the codebase with theversionModifier, and its functionality is verified through tests. The change fromparamStoretoversionModifieris well-integrated. No issues were found with the current implementation.
- Files where
SetAppVersionis used:
x/upgrade/keeper/keeper.gox/upgrade/keeper/migrations.gox/consensus/depinject.gocore/server/app.gobaseapp/abci_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `SetAppVersion` provide a `versionModifier`. # Test: Search for usage of `SetAppVersion`. Expect: All calls provide a `versionModifier`. rg --type go -A 5 $'SetAppVersion'Length of output: 2303
simapp/app.go (1)
299-300: Verify the usage of the version modifier in the codebase.Ensure that the introduction of the version modifier does not lead to any inconsistencies or unexpected behavior in the application.
To verify the correct usage of the version modifier, run the following script:
baseapp/baseapp.go (3)
5-5: LGTM!The code change is approved.
259-263: LGTM!The code changes are approved. The changes to the
AppVersionmethod are consistent with the changes to theBaseAppstruct and improve the error handling.
93-93: Verify the usage ofBaseAppin the codebase.Ensure that all usages of
BaseAppare updated to handle the change in the type ofversionModifierfield.Run the following script to verify the usage of
BaseApp:Verification successful
Verification Successful:
BaseAppUsage UpdatedThe
versionModifierfield in theBaseAppstruct has been updated to useserver.VersionModifier, and this change is reflected correctly across the codebase. No instances of the oldsdk.VersionModifiertype were found in the usage ofBaseApp.
- The change is confirmed in
x/upgrade/keeper/keeper.gowhereserver.VersionModifieris used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `BaseApp` are updated to handle the change in the type of `versionModifier` field. # Test: Search for the usage of `BaseApp`. Expect: No usages of `BaseApp.versionModifier` with the old type `sdk.VersionModifier`. rg --type go -A 5 $'BaseApp'Length of output: 199605
| @@ -72,3 +74,37 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { | |||
| BaseAppOption: baseappOpt, | |||
There was a problem hiding this comment.
Line 68, we should return a baseapp option that sets the version modifier too:
baseappOpt := func(app *baseapp.BaseApp) {
app.SetParamStore(k.ParamsStore)
+ app.SetVersionModifier(versionModifier{Keeper: k})
}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- simapp/app.go (1 hunks)
Additional context used
Path-based instructions (1)
simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
simapp/app.go (1)
299-300: Setting the version modifier for the application.The changes introduce a new functionality to set a version modifier for the application based on the consensus parameters. This enhances the application's ability to manage versioning in relation to its consensus parameters.
Please verify that the version modifier is correctly set and used throughout the application. Ensure that the application behaves as expected in different versions or states.
Skipping the comment to add a note in
simapp/CHANGELOG.mdas it's still valid from the previous review.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- baseapp/options.go (3 hunks)
- simapp/CHANGELOG.md (1 hunks)
- x/consensus/depinject.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/consensus/depinject.go
Additional context used
Path-based instructions (2)
simapp/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"baseapp/options.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (4)
simapp/CHANGELOG.md (1)
49-49: LGTM!The changelog entry for pull request #21508 is well-written and informative. It clearly describes the introduction of the
VersionModifierinterface for updating the application state version inapp.go, highlighting the benefits of improved flexibility and maintainability.baseapp/options.go (3)
163-167: LGTM!The changes to the
SetAppVersionmethod look good. The use of aversionModifiersimplifies the version setting logic and the error handling ensures that theversionModifieris set before setting the version.
330-330: The existing comment byjulienrbrtis still valid:let's add a go doc
Please add a godoc comment describing the purpose of the
SetVersionModifiermethod.
331-336: LGTM!The
SetVersionModifiermethod looks good. The check to prevent modifying theversionModifierafter theBaseAppis sealed is a good practice and the implementation follows the pattern of other setter methods in the file.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- baseapp/baseapp_test.go (1 hunks)
- baseapp/utils_test.go (2 hunks)
- runtime/module.go (3 hunks)
- runtime/v2/module.go (3 hunks)
Files skipped from review due to trivial changes (1)
- runtime/v2/module.go
Files skipped from review as they are similar to previous changes (1)
- runtime/module.go
Additional context used
Path-based instructions (2)
baseapp/utils_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"baseapp/baseapp_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (2)
baseapp/utils_test.go (1)
450-465: Review of newMockedVersionModifier and mockedVersionModifierThe new function
newMockedVersionModifierand the structmockedVersionModifierare well-implemented for the purpose of version management in tests. Here are specific observations and suggestions:
Functionality: The
newMockedVersionModifierfunction correctly initializes an instance ofmockedVersionModifierwith a given starting version. This is crucial for testing scenarios where version state needs to be manipulated.Method Implementation:
SetAppVersion: This method correctly sets the version to a new value. It's straightforward and effective for changing the version during tests.AppVersion: This method retrieves the current version. It's simple and serves its purpose well.Error Handling: Both methods
SetAppVersionandAppVersiondo not currently handle any specific errors related to the version setting or getting. Since this is a mock implementation used for testing, this is acceptable. However, if this pattern is used in production code, error handling should be considered.Testing: The implementation is part of the testing suite, which is good as it isolates version management testing from production code. Ensure that tests using these methods are robust enough to cover various scenarios of version changes.
Documentation: Consider adding inline comments or documentation to these methods to explain their usage, especially since they are part of a testing framework. This can help other developers understand their purpose and usage context quickly.
Overall, the additions are well-suited for their intended testing purposes and follow good coding practices.
baseapp/baseapp_test.go (1)
84-84: Review of the addition ofSetVersionModifierinNewBaseAppSuite.The addition of
app.SetVersionModifier(newMockedVersionModifier(0))at line 84 is a crucial change that aligns with the PR's objective to unify version handling across the SDK. This change ensures that the version modifier is set during the initialization of theBaseAppSuite, which is likely used to simulate different versioning scenarios during testing.
- Correctness: The method call is syntactically correct and matches the expected usage pattern in Go.
- Contextual Fit: This change fits well within the context of the PR, which aims to manage version consistency.
- Testing: Given that this is a test suite file, the change directly impacts how tests might behave with version modifications. It's essential to ensure that other tests within this suite or dependent tests are aware of this versioning behavior to avoid unexpected outcomes.
Recommendation: Ensure that all tests using
BaseAppSuiteare reviewed to accommodate this new versioning behavior, especially those that might rely on version-specific logic. It might also be beneficial to add specific tests that verify the behavior of thenewMockedVersionModifierfunction to ensure it behaves as expected across different scenarios.
(cherry picked from commit dce0365) # Conflicts: # baseapp/utils_test.go # core/server/app.go # runtime/v2/module.go # server/v2/stf/core_branch_service.go # server/v2/stf/core_branch_service_test.go
Co-authored-by: Randy Grok <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #21418
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores