-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(wasmbinding)!: remove wasmbinding custom executors #1766
Conversation
WalkthroughThe recent updates reflect a shift in the Nibiru Chain's approach to handling Wasm bindings and options. The changes suggest a move away from custom bindings towards using MsgServer and StargateMsg, aligning with the project's goal to simplify and enhance the way smart contracts interact with the blockchain's modules. Changes
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (6)
- wasmbinding/bindings/execute_msg.json
- wasmbinding/bindings/query_resp.json
- wasmbinding/test/testdata/cw_nameservice.wasm
- wasmbinding/wasmbin/bindings_perp.wasm
- wasmbinding/wasmbin/controller.wasm
- wasmbinding/wasmbin/shifter.wasm
Files selected for processing (4)
- README.md (2 hunks)
- app/app.go (2 hunks)
- app/keepers.go (1 hunks)
- x/tokenfactory/keeper/msg_server.go (6 hunks)
Files skipped from review due to trivial changes (3)
- README.md
- app/keepers.go
- x/tokenfactory/keeper/msg_server.go
Additional comments: 3
app/app.go (3)
43-46: The removal of the
wasmbinding
import statement is consistent with the PR's objective to remove thewasmbindings
module. This change should not affect other parts of the file as long as there are no references to the removed module elsewhere in the code.113-116: The removal of
wasmbinding.NibiruWasmOptions
from theGetWasmOpts
function aligns with the PR's goal. Ensure that no other part of the application relies on the removed functionality, which could lead to runtime errors.Verification successful
The search for references to
NibiruWasmOptions
in the codebase did not yield any results. This suggests that the removal ofwasmbinding.NibiruWasmOptions
from theGetWasmOpts
function inapp/app.go
has not left any orphaned references in the Go files.* 43-48: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-242]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to `NibiruWasmOptions` in the codebase. rg --type go 'NibiruWasmOptions'Length of output: 32
Given the scope of the changes, it's important to verify that the removal of the
wasmbindings
module does not affect other modules or functionality within the application. This includes checking for any indirect dependencies that might be broken due to the removal.Verification successful
The search for references to the
wasmbindings
module in Go files did not yield any results. This suggests that there are no remaining indirect references or dependencies on thewasmbindings
module within the Go files of the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any indirect references or dependencies on the `wasmbindings` module. rg --type go 'wasmbindings'Length of output: 27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1766 +/- ##
==========================================
- Coverage 73.40% 73.24% -0.16%
==========================================
Files 196 191 -5
Lines 15547 15436 -111
==========================================
- Hits 11412 11306 -106
+ Misses 3473 3470 -3
+ Partials 662 660 -2
|
The stargate queries are still supposed to be there. If that part is removed, contracts can't use the query server at all |
README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks the links for me
app/app.go
Outdated
// Add the bindings to the app's set of []wasmkeeper.Option. | ||
wasmOpts = append(wasmOpts, wasmbinding.NibiruWasmOptions( | ||
nibiru.GRPCQueryRouter(), | ||
nibiru.appCodec, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stargate queries are still supposed to be there. If that part is removed, contracts can't use the query server at all.
So the only part to remove here is the keeper executor stuff, not the entire querier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- app/wasmext/wasm_cli_test/testdata/cw_nameservice.wasm
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- app/app.go (3 hunks)
- app/wasmext/stargate_query.go (2 hunks)
- app/wasmext/stargate_query_test.go (2 hunks)
- app/wasmext/wasm.go (2 hunks)
Files skipped from review due to trivial changes (2)
- app/wasmext/stargate_query.go
- app/wasmext/stargate_query_test.go
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- app/app.go
Additional comments: 3
app/wasmext/wasm.go (3)
- 1-16: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-23]
The removal of parameters related to
perpv2
,sudoKeeper
, andoracleKeeper
from theNibiruWasmOptions
function simplifies the configuration. Please ensure that this change does not impact other modules or functionalities that might depend on these keepers.
1-10: The removal of unused imports is a good practice for maintaining clean code. Please confirm that there are no unresolved references due to these removals.
1-1: The renaming of the package should be reflected across all files that import this package. Please verify that all necessary changes have been made.
Verification successful
The search results indicate that the
wasmext
package is still being referenced in several files within the codebase. Specifically, theapp.go
file imports thewasmext
package and uses theNibiruWasmOptions
function from it. Additionally, thestargate_query_test.go
file references thewasmext
package in a test package declaration and imports it with an aliaswasmbinding
.Given this information, it appears that the package renaming has been accounted for in the codebase, as the references to
wasmext
are consistent with the package name shown in thewasm.go
file.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any old package references that need to be updated. rg --type go 'wasmext'Length of output: 371
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- app/wasmext/wasm_cli_test/cli_test.go (3 hunks)
Files skipped from review due to trivial changes (1)
- app/wasmext/wasm_cli_test/cli_test.go
Purpose / Abstract
Summary by CodeRabbit
Summary by CodeRabbit
README.md
.README.md
.CHANGELOG.md
to reflect the removal of thewasmbindings
module.