Fix incorrect wasm storage directory #1131
Conversation
vexxvakan
requested review from
JakeHartnell,
dimiandre and
niilptr
as code owners
WalkthroughThis change modifies the keeper's initialization to support WebAssembly. The modifications update import statements by adding Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant VM as WasmVM
participant Keeper as WasmKeeper
App->>VM: Call NewVM(wasmDir, wasmCapabilities, 32, contractDebugMode, memoryCacheSize)
VM-->>App: Return wasmEngine instance or error
alt Error Occurs
App->>App: Panic with formatted error message
else Successfully Created
App->>Keeper: Append wasmEngine via WithWasmEngine
Keeper-->>App: Initialize keeper with dataDir
end
Poem
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
app/keepers/keepers.go (1)
540-544: Consider adding proper cleanup for the WebAssembly VM.The WebAssembly VM is initialized but there doesn't appear to be an explicit cleanup mechanism. Consider adding proper cleanup to ensure resources are released when the application shuts down.
For proper resource management, you could add a cleanup method to the AppKeepers struct:
func (appKeepers *AppKeepers) CleanupWasmVM() { if wasmer, ok := appKeepers.WasmKeeper.GetWasmEngine().(*wasmvm.VM); ok { wasmer.Cleanup() } }And then call this method during application shutdown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/keepers/keepers.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: e2e-tests (ictest-clock)
- GitHub Check: e2e-tests (ictest-cwhooks)
- GitHub Check: e2e-tests (ictest-burn)
- GitHub Check: e2e-tests (ictest-feepay)
- GitHub Check: e2e-tests (ictest-drip)
- GitHub Check: e2e-tests (ictest-unity-deploy)
- GitHub Check: e2e-tests (ictest-ibc)
- GitHub Check: e2e-tests (ictest-upgrade)
- GitHub Check: e2e-tests (ictest-globalfee)
- GitHub Check: e2e-tests (ictest-pfm)
- GitHub Check: e2e-tests (ictest-feeshare)
- GitHub Check: e2e-tests (ictest-tokenfactory)
- GitHub Check: e2e-tests (ictest-ibchooks)
- GitHub Check: e2e-tests (ictest-statesync)
- GitHub Check: e2e-tests (ictest-basic)
- GitHub Check: Analyze
🔇 Additional comments (4)
app/keepers/keepers.go (4)
4-6: Import updates aligned with WebAssembly implementation.The changes to imports are appropriate:
- Adding "fmt" for error formatting
- Switching from "filepath" to "path" for cross-platform path handling
- Adding "wasmvm" package for WebAssembly virtual machine functionality
Also applies to: 11-11
223-224: Path construction improved for WebAssembly directory.The updated path construction using
path.Joinis correct and more maintainable than the previous implementation. The code now properly derives the WASM directory from the data directory.
540-544: WebAssembly VM initialization properly implemented.The explicit creation of the WebAssembly VM with proper error handling is a good improvement. This ensures that any issues with the WASM engine initialization are caught early with a clear error message.
560-560: Fixed incorrect WebAssembly storage directory parameter.Passing
dataDirinstead ofwasmDirto the WasmKeeper is the key fix in this PR. This ensures the keeper has access to the correct parent directory for WASM storage.
|
fixes are included in #1141 because of a merge delay - changes already released as v28.0.1 |
Summary by CodeRabbit