chore: upgrade wasmd to v0.53.0 and wasmvm to v2.1.2 #8616
Conversation
PaddyMc
requested review from
mattverse,
nicolaslara,
p0mvn,
czarcas7ic and
AlpinYukseloglu
as code owners
WalkthroughThe recent changes center around upgrading the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant wasmvm
participant Database
User->>App: Send request
App->>wasmvm: Process transaction
wasmvm->>Database: Store transaction
Database-->>wasmvm: Confirm storage
wasmvm-->>App: Return results
App-->>User: Send response
Recent review detailsConfiguration used: CodeRabbit UI 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
C:x/epochs
C:app-wiring
|
Important Notice This PR modifies an in-repo Go module. It is one of:
The dependent Go modules, especially the root one, will have to be Please follow the instructions below:
Please let us know if you need any help. |
PaddyMc
changed the title
![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
Outside diff range, codebase verification and nitpick comments (3)
wasmbinding/message_plugin.go (3)
64-69: Incomplete Integration ofmsgResponsesincreateDenomMethodThe
createDenommethod's additional return valuemsgResponsesis not being handled in the calling code. This oversight may lead to missed processing of important response data. Please review and update the integration to ensuremsgResponsesis appropriately utilized.
- File:
wasmbinding/message_plugin.go- Location: Invocation of
createDenommethodAnalysis chain
Verify
createDenommethod integration.The
createDenommethod now includesmsgResponsesin its return values. Ensure that this change is integrated correctly in the calling code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `msgResponses` in `createDenom` method. # Test: Search for usage of `createDenom` to ensure the new return value is handled. rg --type go 'createDenom' -A 3Length of output: 4623
Script:
#!/bin/bash # Description: Check if `msgResponses` from `createDenom` is handled in the calling code. # Find all instances where `createDenom` is called and check the surrounding lines for handling of `msgResponses`. rg --type go 'createDenom' -A 10Length of output: 8127
136-141: Ensure Proper Handling ofmsgResponsesinchangeAdminIntegrationThe
msgResponsesreturn value from thechangeAdminfunction is not currently being utilized in the calling code. Please ensure thatmsgResponsesis processed or used appropriately to complete the integration. This may involve updating the logic in the calling function to handle this new return value effectively.
- File:
wasmbinding/message_plugin.go- Function:
changeAdminAnalysis chain
Verify
changeAdminmethod integration.The
changeAdminmethod now includesmsgResponsesin its return values. Ensure that this change is integrated correctly in the calling code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `msgResponses` in `changeAdmin` method. # Test: Search for usage of `changeAdmin` to ensure the new return value is handled. rg --type go 'changeAdmin' -A 3Length of output: 6045
Script:
#!/bin/bash # Description: Search for calls to `changeAdmin` and examine handling of return values, especially `msgResponses`. # Search for function calls to `changeAdmin` and include enough context to see how return values are handled. rg --type go '\bchangeAdmin\b' -A 10Length of output: 10255
98-103: Integration ofmsgResponsesinmintTokensmethod is incomplete.The
msgResponsesreturn value from themintTokensmethod is not being utilized or handled in the calling code. This suggests that the integration of this new return value is incomplete and may need further implementation.
- File:
wasmbinding/message_plugin.go- Lines: 98-103
Analysis chain
Verify
mintTokensmethod integration.The
mintTokensmethod now includesmsgResponsesin its return values. Ensure that this change is integrated correctly in the calling code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `msgResponses` in `mintTokens` method. # Test: Search for usage of `mintTokens` to ensure the new return value is handled. rg --type go 'mintTokens' -A 3Length of output: 1337
Script:
#!/bin/bash # Description: Search for the complete function or method that calls `mintTokens` to verify handling of `msgResponses`. # Test: Search for function definitions that include a call to `mintTokens` to see how the return values are handled. rg --type go 'func ' -A 20 | rg 'mintTokens'Length of output: 490
Script:
#!/bin/bash # Description: Capture more context around the `mintTokens` call to verify handling of `msgResponses`. # Test: Search for the `mintTokens` call with more surrounding lines to see how the return values are handled. rg --type go 'mintTokens' -A 10 -B 10Length of output: 3442
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
wasmbinding/message_plugin.go (3)
168-173: EnsuremsgResponsesis handled when usingburnTokens.The
burnTokensmethod now includesmsgResponsesin its return values, but its usage inwasmbinding/message_plugin.godoes not handle this new return value. Please ensure thatmsgResponsesis appropriately used or checked whereburnTokensis called.
- File:
wasmbinding/message_plugin.go- Line: Usage of
burnTokenswithout handlingmsgResponses.Analysis chain
LGTM! Verify usage of
burnTokens.The addition of
msgResponsesto the return values is a good enhancement for detailed responses.Ensure that all instances where
burnTokensis used handle the new return value correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `burnTokens` to ensure compatibility with the new return value. # Test: Search for `burnTokens` usage in the codebase. Expect: All usages handle `msgResponses`. rg --type go 'burnTokens' -A 5Length of output: 960
98-103: Ensure Handling ofmsgResponsesinmintTokensUsageThe
mintTokensmethod's new return value,msgResponses, is not being utilized in its current usage withinwasmbinding/message_plugin.go. This oversight should be addressed to ensure all return values are properly handled. Please review the following:
- File:
wasmbinding/message_plugin.go
- Line: Where
mintTokensis called, ensuremsgResponsesis appropriately processed.Analysis chain
LGTM! Verify usage of
mintTokens.The addition of
msgResponsesto the return values is a beneficial enhancement.Ensure that all instances where
mintTokensis used handle the new return value correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `mintTokens` to ensure compatibility with the new return value. # Test: Search for `mintTokens` usage in the codebase. Expect: All usages handle `msgResponses`. rg --type go 'mintTokens' -A 5Length of output: 1638
64-69: EnsuremsgResponsesis handled increateDenomusageThe
createDenommethod has been updated to includemsgResponsesin its return values, but current usages do not handle this new return value. Please review the following locations to ensuremsgResponsesis appropriately processed:
wasmbinding/message_plugin.go: Ensure that when callingcreateDenom, themsgResponsesreturn value is handled or utilized as needed.Analysis chain
LGTM! Verify usage of
createDenom.The addition of
msgResponsesto the return values enhances the method's response capabilities.Ensure that all instances where
createDenomis used handle the new return value correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `createDenom` to ensure compatibility with the new return value. # Test: Search for `createDenom` usage in the codebase. Expect: All usages handle `msgResponses`. rg --type go 'createDenom' -A 5Length of output: 6357
PaddyMc
changed the title
PaddyMc
force-pushed
the
chore/upgrade-cosmwasm-v0.52.0
branch
from
756270e to
8f3ddf8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (5)
x/smart-account/ante/circuit_breaker_test.go (1)
55-56: Use a more descriptive directory name.While using a random integer ensures uniqueness, consider adding a prefix to the directory name for clarity, such as
testdir_<random_number>.- s.HomeDir = fmt.Sprintf("%d", rand.Int()) + s.HomeDir = fmt.Sprintf("testdir_%d", rand.Int())x/smart-account/post/post_test.go (1)
59-60: Use a more descriptive directory name.Consider using a more descriptive directory name for clarity.
- s.HomeDir = fmt.Sprintf("%d", rand.Int()) + s.HomeDir = fmt.Sprintf("testdir_%d", rand.Int())x/ibc-rate-limit/ibc_middleware_test.go (1)
96-99: Enhance error handling for directory removal.Consider checking for errors when removing directories to handle potential issues gracefully.
Add error handling as follows:
for _, dir := range osmosisibctesting.TestingDirectories { - os.RemoveAll(dir) + err := os.RemoveAll(dir) + if err != nil { + suite.T().Logf("Failed to remove directory %s: %v", dir, err) + } }x/smart-account/authenticator/composition_test.go (1)
80-82: Enhance error handling for directory removal.Consider checking for errors when removing the directory to handle potential issues gracefully.
Add error handling as follows:
os.RemoveAll(s.HomeDir) + if err != nil { + s.T().Logf("Failed to remove directory %s: %v", s.HomeDir, err) + }x/smart-account/integration_test.go (1)
78-82: Enhance error handling for directory removal.Consider checking for errors when removing directories to handle potential issues gracefully.
Add error handling as follows:
for _, dir := range osmosisibctesting.TestingDirectories { - os.RemoveAll(dir) + err := os.RemoveAll(dir) + if err != nil { + suite.T().Logf("Failed to remove directory %s: %v", dir, err) + } }
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (8)
x/smart-account/ante/circuit_breaker_test.go (1)
55-56: Use ofrand.Int()for directory names is acceptable but consider usingrand.Int63()for a wider range.Using
rand.Int()is fine, butrand.Int63()can provide a larger range of values, reducing the chance of collisions.- s.HomeDir = fmt.Sprintf("%d", rand.Int()) + s.HomeDir = fmt.Sprintf("%d", rand.Int63())x/pool-incentives/keeper/genesis_test.go (2)
70-71: Use ofrand.Int()for directory names is acceptable but consider usingrand.Int63()for a wider range.Using
rand.Int()is fine, butrand.Int63()can provide a larger range of values, reducing the chance of collisions.- dirName := fmt.Sprintf("%d", rand.Int()) + dirName := fmt.Sprintf("%d", rand.Int63())
97-98: Use ofrand.Int()for directory names is acceptable but consider usingrand.Int63()for a wider range.Using
rand.Int()is fine, butrand.Int63()can provide a larger range of values, reducing the chance of collisions.- dirName := fmt.Sprintf("%d", rand.Int()) + dirName := fmt.Sprintf("%d", rand.Int63())x/smart-account/post/post_test.go (1)
59-60: Use ofrand.Int()for directory names is acceptable but consider usingrand.Int63()for a wider range.Using
rand.Int()is fine, butrand.Int63()can provide a larger range of values, reducing the chance of collisions.- s.HomeDir = fmt.Sprintf("%d", rand.Int()) + s.HomeDir = fmt.Sprintf("%d", rand.Int63())x/incentives/keeper/genesis_test.go (1)
177-178: Use ofrand.Int()for directory names is acceptable but consider usingrand.Int63()for a wider range.Using
rand.Int()is fine, butrand.Int63()can provide a larger range of values, reducing the chance of collisions.- dirName := fmt.Sprintf("%d", rand.Int()) + dirName := fmt.Sprintf("%d", rand.Int63())x/smart-account/authenticator/signature_authenticator_test.go (1)
45-47: Initialization ofHomeDirnot found insignature_authenticator_test.goThe
HomeDirvariable is used in theTearDownTestmethod, but its initialization is not present in thesignature_authenticator_test.gofile. Ensure thatHomeDiris properly initialized before it is used to avoid potential issues.
- Check the setup methods in
signature_authenticator_test.goforHomeDirinitialization.Analysis chain
LGTM! Ensure proper usage of
HomeDir.The addition of the
TearDownTestmethod is a good practice for cleaning up test resources.Verify that
HomeDiris correctly initialized and used in the test setup.Run the following script to verify the initialization and usage of
HomeDir:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify initialization and usage of `HomeDir` in the test setup. # Test: Search for `HomeDir` initialization and usage. Expect: Proper initialization and usage. rg --type go 'HomeDir' -A 5Length of output: 19155
Script:
#!/bin/bash # Description: Search for the initialization of `HomeDir` in the signature_authenticator_test.go file. # Locate the file first file_path=$(fd 'signature_authenticator_test.go') # Search for `HomeDir` initialization in the located file rg 'HomeDir' -A 5 "$file_path"Length of output: 354
x/smart-account/ante/ante_test.go (1)
65-66: Potential Directory Name Collision RiskThe use of
rand.Int()for generating directory names in test setups might lead to collisions due to the limited range of integers it produces. Consider usingrand.Int63()or appending a timestamp to ensure uniqueness.
- Instances found in:
x/smart-account/ante/ante_test.gox/smart-account/authenticator/base_test.gox/smart-account/authenticator/cosmwasm_test.gox/smart-account/post/post_test.goAnalysis chain
LGTM! Verify unique directory creation.
The
SetupTestmethod now creates a unique directory for each test run, enhancing test isolation.Verify that the directory creation logic is consistent and doesn't lead to collisions.
Run the following script to verify the directory creation logic:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify unique directory creation logic in `SetupTest`. # Test: Search for directory creation logic. Expect: Proper usage of random integers. rg --type go 'rand.Int()' -A 5Length of output: 26345
x/smart-account/authenticator/cosmwasm_test.go (1)
48-49: Potential for Directory Name CollisionsThe use of
rand.Int()for generating directory names can lead to collisions, especially if the random seed is not set uniquely for each test run. Consider using a more robust method, such as UUIDs or timestamps, to ensure unique directory names.Analysis chain
LGTM! Verify unique directory creation.
The
SetupTestmethod now creates a unique directory for each test run, enhancing test isolation.Verify that the directory creation logic is consistent and doesn't lead to collisions.
Run the following script to verify the directory creation logic:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify unique directory creation logic in `SetupTest`. # Test: Search for directory creation logic. Expect: Proper usage of random integers. rg --type go 'rand.Int()' -A 5Length of output: 26345
PaddyMc
changed the title
| "cosmwasm_1_2", | ||
| "cosmwasm_1_4", | ||
| "cosmwasm_2_0", | ||
| "cosmwasm_2_1", |
There was a problem hiding this comment.
added the latest cosmwasm features here
There was a problem hiding this comment.
For ref of what this is:
https://github.com/CosmWasm/cosmwasm/blob/main/docs/CAPABILITIES-BUILT-IN.md
| @@ -354,7 +355,8 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) { | |||
| WithHomeDir(homeDir). | |||
| WithViper("OSMOSIS") | |||
|
|
|||
| tempApp := osmosis.NewOsmosisApp(log.NewNopLogger(), cosmosdb.NewMemDB(), nil, true, map[int64]bool{}, osmosis.DefaultNodeHome, 5, sims.EmptyAppOptions{}, osmosis.EmptyWasmOpts, baseapp.SetChainID("osmosis-1")) | |||
| tempDir := fmt.Sprintf("%s%d", ".temp-osmosis", rand.Int()) | |||
There was a problem hiding this comment.
Needed this as we call NewOsmosisApp twice during every run of osmosisd it's necessary because of this new feature in wasmvm => https://github.com/CosmWasm/wasmvm/blob/main/internal/api/lib.go#L44-L63
There was a problem hiding this comment.
This is basically why the PR is so big as well 🥹
| @@ -354,7 +355,8 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) { | |||
| WithHomeDir(homeDir). | |||
| WithViper("OSMOSIS") | |||
|
|
|||
| tempApp := osmosis.NewOsmosisApp(log.NewNopLogger(), cosmosdb.NewMemDB(), nil, true, map[int64]bool{}, osmosis.DefaultNodeHome, 5, sims.EmptyAppOptions{}, osmosis.EmptyWasmOpts, baseapp.SetChainID("osmosis-1")) | |||
| tempDir := fmt.Sprintf("%s%d", ".temp-osmosis", rand.Int()) | |||
| "cosmwasm_1_2", | ||
| "cosmwasm_1_4", | ||
| "cosmwasm_2_0", | ||
| "cosmwasm_2_1", |
There was a problem hiding this comment.
For ref of what this is:
https://github.com/CosmWasm/cosmwasm/blob/main/docs/CAPABILITIES-BUILT-IN.md
| github.com/CosmWasm/wasmd v0.53.0 | ||
| github.com/CosmWasm/wasmvm/v2 v2.1.2 | ||
| github.com/cometbft/cometbft v0.38.11 | ||
| github.com/cometbft/cometbft-db v0.12.0 | ||
| github.com/cosmos/cosmos-db v1.0.2 | ||
| github.com/cosmos/cosmos-proto v1.0.0-beta.5 | ||
| github.com/cosmos/cosmos-sdk v0.50.9 | ||
| github.com/cosmos/go-bip39 v1.0.0 | ||
| github.com/cosmos/gogoproto v1.6.0 | ||
| github.com/cosmos/gogoproto v1.7.0 | ||
| github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v8 v8.0.2 | ||
| github.com/cosmos/ibc-apps/modules/async-icq/v8 v8.0.0 | ||
| github.com/cosmos/ibc-go/modules/capability v1.0.1 | ||
| github.com/cosmos/ibc-go/modules/light-clients/08-wasm v0.1.1-ibc-go-v7.3-wasmvm-v1.5 | ||
| github.com/cosmos/ibc-go/modules/light-clients/08-wasm v0.4.2-0.20240730185033-ccd4dc278e72 |
| github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v8 v8.0.2 | ||
| github.com/cosmos/ibc-apps/modules/async-icq/v8 v8.0.0 | ||
| github.com/cosmos/ibc-go/modules/capability v1.0.1 | ||
| github.com/cosmos/ibc-go/modules/light-clients/08-wasm v0.1.1-ibc-go-v7.3-wasmvm-v1.5 | ||
| github.com/cosmos/ibc-go/modules/light-clients/08-wasm v0.4.2-0.20240730185033-ccd4dc278e72 |
There was a problem hiding this comment.
Would appreciate linking commit next time for security critical deps
| @@ -99,7 +99,7 @@ require ( | |||
| github.com/cockroachdb/tokenbucket v0.0.0-20230807174530-cc333fc44b06 // indirect | |||
| github.com/coinbase/rosetta-sdk-go/types v1.0.0 // indirect | |||
| github.com/cosmos/gogogateway v1.2.0 // indirect | |||
| github.com/cosmos/iavl v1.1.3 // indirect | |||
| github.com/cosmos/iavl v1.2.0 // indirect | |||
There was a problem hiding this comment.
I'll do that in a separate PR if that's ok?
Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: Roman <roman@osmosis.team>
What is the purpose of the change
Upgrading wasmd to the latest version v0.52.0