feat: support v3 app #3870
feat: support v3 app #3870
Conversation
cmwaters
requested review from
rootulp and
rach-id
and removed request for
a team
WalkthroughWalkthroughThe changes in this pull request focus on integrating a new application version ( Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
pkg/appconsts/v3/app_consts.go (1)
1-7: LGTM! Consider adding documentation and ensuring consistent usage.The constants defined in this file provide a centralized way to manage configuration values for version 3 of the application. This approach enhances maintainability and clarity.
Suggestions:
- Add comments to document the purpose and usage of each constant. This will help future maintainers understand their significance and how they should be used.
- Ensure that these constants are used consistently throughout the codebase for version 3 of the application. This will help maintain coherence and avoid potential bugs or inconsistencies.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (14)
- app/app.go (3 hunks)
- app/check_tx.go (1 hunks)
- app/module/versioned_ibc_module_test.go (1 hunks)
- app/modules.go (4 hunks)
- app/prepare_proposal.go (4 hunks)
- app/process_proposal.go (3 hunks)
- app/test/std_sdk_test.go (1 hunks)
- app/test/upgrade_test.go (4 hunks)
- go.mod (1 hunks)
- pkg/appconsts/v3/app_consts.go (1 hunks)
- pkg/appconsts/versioned_consts.go (1 hunks)
- x/blob/types/blob_tx.go (3 hunks)
- x/blob/types/blob_tx_test.go (1 hunks)
- x/signal/integration_test.go (5 hunks)
Additional comments not posted (38)
pkg/appconsts/versioned_consts.go (2)
5-5: LGTM!The import statement update from
v2tov3is consistent with the PR objective of introducingv3as a supported application version. This change is necessary to use the constants and functionality provided by thev3package.
9-9: Verify the required constants and functions in thev3package.The update to the
LatestVersionconstant to referencev3.Versionis consistent with the PR objective. However, please ensure that thev3package defines the following:
- The
Versionconstant.- The
SubtreeRootThresholdfunction.- The
SquareSizeUpperBoundfunction.This is important to avoid potential runtime errors when using the
DefaultSubtreeRootThresholdandDefaultSquareSizeUpperBoundvariables.Run the following script to verify the existence of the required constants and functions:
Verification successful
Verification complete: Constants found in v3 package
The required elements for the v3 package have been verified to exist in the file
pkg/appconsts/v3/app_consts.go. However, their implementation differs slightly from what was initially expected:
Versionis correctly defined as a constant with a value of 3.SquareSizeUpperBoundandSubtreeRootThresholdare defined as constants rather than functions, with values 128 and 64 respectively.This implementation still provides the necessary constants for the v3 package, although in a different form than originally anticipated. The update to
LatestVersionin the reviewed file is consistent with this implementation.No further action is required regarding the existence of these constants in the v3 package.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the required constants and functions in the `v3` package. # Test 1: Check if the `Version` constant is defined in the `v3` package. # Expect: The constant should be defined. ast-grep --lang go --pattern $'package v3 const Version = $_' # Test 2: Check if the `SubtreeRootThreshold` function is defined in the `v3` package. # Expect: The function should be defined. ast-grep --lang go --pattern $'package v3 func SubtreeRootThreshold($$$) { $$$ }' # Test 3: Check if the `SquareSizeUpperBound` function is defined in the `v3` package. # Expect: The function should be defined. ast-grep --lang go --pattern $'package v3 func SquareSizeUpperBound($$$) { $$$ }'Length of output: 1638
Script:
#!/bin/bash # Description: Verify the required constants and functions in the `v3` package. # Find the v3 package directory v3_dir=$(fd -t d -d 2 "v3$") if [ -z "$v3_dir" ]; then echo "v3 package directory not found" exit 1 fi # Display the contents of the v3 package file(s) cat "$v3_dir"/*.go # Search for the required constants and functions echo "Searching for Version constant:" grep -n "const Version =" "$v3_dir"/*.go echo "Searching for SubtreeRootThreshold function:" grep -n "func SubtreeRootThreshold(" "$v3_dir"/*.go echo "Searching for SquareSizeUpperBound function:" grep -n "func SquareSizeUpperBound(" "$v3_dir"/*.goLength of output: 127
Script:
#!/bin/bash # Description: Locate and verify the required constants and functions for v3. # Step 1: Try to find any directory containing "v3" in its name v3_dir=$(fd -t d -H "v3") if [ -n "$v3_dir" ]; then echo "Potential v3 directory found: $v3_dir" echo "Contents of the directory:" ls -la "$v3_dir" echo "Searching for Go files in the directory:" find "$v3_dir" -name "*.go" -type f echo "Searching for Version constant and required functions:" grep -n -R "const Version =" "$v3_dir" grep -n -R "func SubtreeRootThreshold(" "$v3_dir" grep -n -R "func SquareSizeUpperBound(" "$v3_dir" else echo "No directory containing 'v3' found. Searching for relevant Go files and contents." # Step 2: Search for Go files containing the Version constant and required functions echo "Searching for Version constant:" rg -n "const Version =" -g "*.go" echo "Searching for SubtreeRootThreshold function:" rg -n "func SubtreeRootThreshold\(" -g "*.go" echo "Searching for SquareSizeUpperBound function:" rg -n "func SquareSizeUpperBound\(" -g "*.go" fi # Step 3: If we still can't find the required elements, this will be evident from the outputLength of output: 896
Script:
#!/bin/bash # Description: Examine v3 package contents and search for required elements echo "Contents of pkg/appconsts/v3/app_consts.go:" cat pkg/appconsts/v3/app_consts.go echo -e "\nSearching for Version-like constants:" grep -n "const.*Version" pkg/appconsts/v3/app_consts.go echo -e "\nSearching for SubtreeRootThreshold-like functions:" grep -n "func.*SubtreeRootThreshold" pkg/appconsts/v3/app_consts.go echo -e "\nSearching for SquareSizeUpperBound-like functions:" grep -n "func.*SquareSizeUpperBound" pkg/appconsts/v3/app_consts.go echo -e "\nAll constants in the file:" grep -n "const" pkg/appconsts/v3/app_consts.go echo -e "\nAll functions in the file:" grep -n "func" pkg/appconsts/v3/app_consts.goLength of output: 1021
app/check_tx.go (1)
44-45: LGTM! The dynamic threshold approach improves compatibility and correctness.The changes introduce a dynamic approach to determine the subtree root threshold based on the application version. This ensures that the appropriate validation logic is applied for the current application context.
Benefits of the dynamic threshold approach:
- Enhances the flexibility and correctness of the transaction validation process.
- Improves the compatibility and functionality of the application across different versions.
Great work on implementing this change!
x/signal/integration_test.go (8)
7-7: LGTM!The import path update for the
v2package is consistent with the PR objective of introducingv3as a supported application version.
23-25: LGTM!Setting the
AppVersiontov2.Versionin the consensus parameters and updating the test application setup accordingly simulates the upgrade scenario fromv2tov3. These changes align with the PR objective and the AI-generated summary.
28-28: LGTM!Updating the
Appversion in the context header tov2.Versionis consistent with the simulated upgrade scenario and aligns with the PR objective and the AI-generated summary.
35-35: LGTM!Updating the version number to
3in theQueryVersionTallyRequestreflects the new versioning scheme and is consistent with the PR objective and the AI-generated summary.
46-46: LGTM!Updating the version number to
3in theMsgSignalVersionreflects the new versioning scheme for signaling votes and is consistent with the PR objective and the AI-generated summary.
51-51: LGTM!Updating the version number to
3in theQueryVersionTallyRequestreflects the new versioning scheme for tallying votes and is consistent with the PR objective and the AI-generated summary.
71-71: LGTM!Updating the version number to
4in theMsgSignalVersionis part of the test scenario to verify the behavior when an upgrade is pending. The test expects an error when attempting to signal a version4while an upgrade to version3is pending. This change is consistent with the PR objective and the AI-generated summary.
84-84: LGTM!Updating the expected version number to
3in the assertion reflects the new versioning scheme and the simulated upgrade scenario. This change is consistent with the PR objective and the AI-generated summary.app/prepare_proposal.go (5)
33-33: LGTM!Setting the
Appfield to the current application version in the context header is correct and necessary.
53-77: Excellent version-specific handling!The switch statement effectively distinguishes between different application versions and uses the appropriate data structures and methods for each version. This enhances compatibility and functionality.
The error handling for unsupported versions is also clear and appropriate.
85-85: LGTM!The erasure encoding of the data square using
da.ExtendSharesis correct and necessary.
111-111: LGTM!Setting the
SquareSizefield to the calculated size of the data square is correct and necessary.
Line range hint
1-116: Great work on enhancing version compatibility!The changes in this file significantly improve the handling of different application versions when preparing a proposal. The version-specific logic is well-structured and modular, ensuring the correct data structures and methods are used for each version.
The error handling is also appropriate and informative, providing clear messages for unsupported versions.
Overall, these modifications enhance the flexibility and robustness of the proposal preparation process and align well with the PR objective of introducing v3 as a supported application version.
x/blob/types/blob_tx.go (3)
6-6: LGTM!The import statement for the
v3package is correctly added with an appropriate alias.
83-85: LGTM!The new conditional block correctly validates the compatibility between the blob's share version and the application version. The error message is clear and informative.
39-39: Verify the function signature change in the codebase.The new parameter
appVersionis correctly added to theValidateBlobTxfunction signature. However, ensure that all function calls toValidateBlobTxhave been updated to pass theappVersionargument.Run the following script to verify the function usage:
Verification successful
All function calls to ValidateBlobTx have been correctly updated
The verification process confirms that all occurrences of the ValidateBlobTx function in the codebase have been properly updated to include the new appVersion argument. The function is called in three different files:
- x/blob/types/blob_tx_test.go (test file)
- app/process_proposal.go
- app/check_tx.go
In each case, the appVersion argument is correctly provided, using appropriate values or variables (appconsts.LatestVersion, app.AppVersion(), and appVersion respectively). No further changes are required.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ValidateBlobTx` pass the `appVersion` argument. # Test: Search for the function usage. Expect: Only occurrences with the new signature. rg --type go -A 5 $'ValidateBlobTx'Length of output: 2209
x/blob/types/blob_tx_test.go (1)
254-254: LGTM!The inclusion of the
appconsts.LatestVersionargument in theValidateBlobTxfunction call aligns with the updated function signature and enables version-aware transaction validation. This change enhances the flexibility and adaptability of the validation logic to handle version-specific rules or constants. The test case ensures that the function correctly accepts and utilizes the new version parameter.app/process_proposal.go (5)
12-16: LGTM!The new imports for
squarev2andsharev2packages are necessary to support the version 3 specific changes in theProcessProposalmethod.
114-114: LGTM!The updated
blobtypes.ValidateBlobTxfunction call with theapp.AppVersion()parameter allows for version-specific blob transaction validation, which is consistent with the overall goal of supporting multiple application versions.
128-130: LGTM!The new variables
dataSquareBytesanderrare declared appropriately to store the result and error from constructing the data square, which is then used in the version-specific logic.
133-153: LGTM!The switch statement effectively handles the version-specific construction of data squares using the appropriate packages for each version. The assertion of the square size and the default case for unsupported versions ensure the correctness and robustness of the proposal processing logic.
156-157: LGTM!The improved error handling and logging for failures in computing the data square provide better visibility and consistency in handling invalid proposal blocks.
app/module/versioned_ibc_module_test.go (1)
26-26: LGTM!The change to instantiate the
VersionedIBCModulewith version 3 aligns with the PR objective of introducing v3 as a supported app version. The existing test cases provide good coverage for the behavior of the module with different app versions.app/test/upgrade_test.go (3)
37-144: LGTM!The
TestAppUpgradeV3function provides comprehensive test coverage for the upgrade process from version 2 to version 3. It ensures that the application behaves as expected during the upgrade and that transactions can be processed correctly after the upgrade.
Line range hint
148-239: LGTM!The
TestAppUpgradeV2function provides test coverage for the parameter overrides during the upgrade from version 1 to version 2. It ensures that the module parameters are correctly updated during the upgrade process.The renaming of the function improves clarity and aligns with the naming convention used for the new
TestAppUpgradeV3function.
Line range hint
241-281: LGTM!The
SetupTestAppWithUpgradeHeightfunction provides a convenient way to set up a test application with a specified upgrade height.The modification to return a
genesis.Genesisobject instead of a keyring reflects a change in how the genesis state is managed during the test setup.The update to the supported versions list ensures that the application correctly recognizes all available versions, including version 3.
go.mod (1)
9-9: New dependency added:github.com/celestiaorg/go-square.The addition of the
go-squarepackage at versionv1.0.0suggests that the project will now leverage functionality provided by this external module. This could introduce changes in the codebase to integrate and utilize the capabilities offered bygo-square.It's important to ensure that:
- The new dependency is compatible with the existing codebase and doesn't introduce any breaking changes.
- The version
v1.0.0is stable and has been thoroughly tested.- The usage of
go-squarealigns with the project's requirements and goals.Please verify that the integration of this new dependency is properly implemented and tested throughout the affected parts of the codebase.
app/modules.go (4)
99-99: LGTM!The update to the
genutilmodule's supported version range aligns with the PR objective of introducing version 3 support.
103-103: Looks good!The updates to the
ToVersionfor various modules tov3align with the PR objective of introducing version 3 support. This change ensures that these modules are compatible with the new application version.Also applies to: 107-107, 111-111, 115-115, 119-119, 123-123, 127-127, 131-131, 135-135, 139-139, 143-143, 147-147, 151-151, 155-155, 159-159, 163-163, 167-167
175-175: Approved!Updating the
FromVersiontov3for thesignal,minfee,packetforward, andicamodules ensures they are compatible with the new application version. This change aligns with the PR objective.Also applies to: 179-179, 183-183, 187-187
Line range hint
306-363: Excellent!Adding the
v3mapping to theversionedStoreKeysfunction is a crucial step in supporting the new application version. The comprehensive list of store keys ensures that the application can handle the data structures and functionalities introduced in version 3. This change is consistent with the PR objective and facilitates a smooth transition.app/app.go (4)
15-15: LGTM!The import statement correctly adds the new
v3version constant from theappv3package, aligning with the PR objective.
112-112: LGTM!The constant declaration correctly introduces
v3as a new supported app version, aligning with the PR objective.
346-346: Verify thepacketForwardMiddlewareimplementation.The modification to support both
v2andv3versions in thetransferStackfor thepacketForwardMiddlewarelooks good. However, please ensure that thepacketForwardMiddlewareimplementation correctly handles both versions to maintain compatibility.
349-349: Verify thetokenFilterMiddelwareimplementation.The modification to support both
v1andv3versions in thetransferStackfor thetokenFilterMiddelwarelooks good. However, please ensure that thetokenFilterMiddelwareimplementation correctly handles both versions to maintain compatibility.
app/test/std_sdk_test.go
Outdated
| @@ -307,7 +307,7 @@ func (s *StandardSDKIntegrationTestSuite) TestStandardSDK() { | |||
| name: "signal a version change", | |||
| msgFunc: func() (msgs []sdk.Msg, signer string) { | |||
| valAccount := s.getValidatorAccount() | |||
| msg := signal.NewMsgSignalVersion(valAccount, 2) | |||
| msg := signal.NewMsgSignalVersion(valAccount, 4) | |||
There was a problem hiding this comment.
Align the test case with the PR changes.
The version number used in the test case (4) is higher than the new application version being introduced in this PR (v3). To accurately validate the changes, consider updating the version number to match the new version.
Apply this diff to align the test case:
-msg := signal.NewMsgSignalVersion(valAccount, 4)
+msg := signal.NewMsgSignalVersion(valAccount, 3)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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| msg := signal.NewMsgSignalVersion(valAccount, 4) | |
| msg := signal.NewMsgSignalVersion(valAccount, 3) |
app/test/std_sdk_test.go
Outdated
| @@ -307,7 +307,7 @@ func (s *StandardSDKIntegrationTestSuite) TestStandardSDK() { | |||
| name: "signal a version change", | |||
| msgFunc: func() (msgs []sdk.Msg, signer string) { | |||
| valAccount := s.getValidatorAccount() | |||
| msg := signal.NewMsgSignalVersion(valAccount, 2) | |||
| msg := signal.NewMsgSignalVersion(valAccount, 4) | |||
There was a problem hiding this comment.
| msg := signal.NewMsgSignalVersion(valAccount, 4) | |
| msg := signal.NewMsgSignalVersion(valAccount, 3) |
There was a problem hiding this comment.
[blocking] 4 seems incorrect b/c that version isn't supported by the state machine? Why not use 3 here like Nina suggests.
There was a problem hiding this comment.
Because 3 is the current version. It would be weird to signal for the current version. What is more normal is to signal for the next version
There was a problem hiding this comment.
I could do something like LatestVersion + 1 so we don't have to continually increment it
There was a problem hiding this comment.
Oh I see, thanks! No longer blocking.
4 or LatestVersion + 1 both work for me.
| @@ -27,9 +34,118 @@ import ( | |||
| dbm "github.com/tendermint/tm-db" | |||
| ) | |||
|
|
|||
| func TestAppUpgradeV3(t *testing.T) { | |||
There was a problem hiding this comment.
more comments would be useful for the reader
| @@ -341,10 +343,10 @@ func New( | |||
| packetforwardkeeper.DefaultRefundTransferPacketTimeoutTimestamp, // refund timeout | |||
| ) | |||
| // PacketForwardMiddleware is used only for version 2. | |||
| transferStack = module.NewVersionedIBCModule(packetForwardMiddleware, transferStack, v2, v2) | |||
| transferStack = module.NewVersionedIBCModule(packetForwardMiddleware, transferStack, v2, v3) | |||
There was a problem hiding this comment.
should we have some kind of test to avoid forgetting to bump versioned ibc modules?
app/modules.go
Outdated
| @@ -341,6 +341,26 @@ func versionedStoreKeys() map[uint64][]string { | |||
| stakingtypes.StoreKey, | |||
| upgradetypes.StoreKey, | |||
| }, | |||
| v3: { | |||
There was a problem hiding this comment.
[no change needed] verified this is the exact same list as v2 https://www.diffchecker.com/pHyajXyz/
| ) | ||
| switch app.AppVersion() { |
There was a problem hiding this comment.
should we add a unit test to prepare proposal to verify this logic?
|
|
||
| switch app.AppVersion() { | ||
| case v3: | ||
| var dataSquare squarev2.Square |
There was a problem hiding this comment.
should we add a unit test to process proposal to verify this logic?
There was a problem hiding this comment.
What logic would this test be verifying? Do we want to build a v3 square with an authored blob and assert that we can parse out the signer or something like that?
app/test/std_sdk_test.go
Outdated
| @@ -307,7 +307,7 @@ func (s *StandardSDKIntegrationTestSuite) TestStandardSDK() { | |||
| name: "signal a version change", | |||
| msgFunc: func() (msgs []sdk.Msg, signer string) { | |||
| valAccount := s.getValidatorAccount() | |||
| msg := signal.NewMsgSignalVersion(valAccount, 2) | |||
| msg := signal.NewMsgSignalVersion(valAccount, 4) | |||
There was a problem hiding this comment.
[blocking] 4 seems incorrect b/c that version isn't supported by the state machine? Why not use 3 here like Nina suggests.
Co-authored-by: Rootul P <[email protected]> Co-authored-by: nina / ნინა <[email protected]>
celestia-bot
requested review from
a team and
ninabarbakadze
and removed request for
a team
cmwaters
dismissed stale reviews from ninabarbakadze and rootulp
via
3b6ff9d
celestia-bot
requested review from
a team and
ninabarbakadze
and removed request for
a team
Adds v3 as an app version.