-
Notifications
You must be signed in to change notification settings - Fork 201
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
feat: base64 decoder for cw3 wasm messages #1731
Conversation
WalkthroughThe introduced changes provide a feature to decode base64-encoded protobuf messages, specifically for cw3 wasm messages in the stargate format. The update adds a new CLI command along with necessary functions and types to parse and reveal the contents of these messages, making it easier for users to understand the actions being taken by smart contracts within a blockchain context. Changes
Assessment against linked issues
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? TipsChat with CodeRabbit Bot (
|
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: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- cmd/nibid/cmd/decode_base64.go (1 hunks)
- cmd/nibid/cmd/decode_base64_test.go (1 hunks)
- cmd/nibid/cmd/root.go (1 hunks)
Additional comments: 1
cmd/nibid/cmd/root.go (1)
- 139-145: The integration of
DecodeBase64Cmd
into the root command's initialization process is done correctly. It is added as a subcommand, which will allow it to be invoked as part of the CLI.
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: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- cmd/nibid/cmd/decode_base64.go (1 hunks)
- cmd/nibid/cmd/decode_base64_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cmd/nibid/cmd/decode_base64_test.go
Additional comments: 5
cmd/nibid/cmd/decode_base64.go (5)
3-54: The import block includes a large number of dependencies. Verify that all imported packages are used within the file to avoid unnecessary imports which can lead to increased compilation time and binary size.
56-189: The
MakeMap
function initializes a map with string keys andproto.Message
values. Ensure that all message types are correctly mapped to their respective protobuf message types and that the map is used appropriately in the decoding process.192-223: The recursive function
parseStargateMsgs
is used to parse nested JSON structures intowasmvm.StargateMsg
objects. Ensure that the recursion is well-defined and terminates correctly to prevent potential stack overflow errors.225-228: The
StargateMsgDecoded
struct is defined with JSON tags, which suggests it's used for JSON marshaling/unmarshaling. Verify that the fields and tags align with the expected JSON structure for decoded messages.277-296: The
DecodeBase64Cmd
function sets up a Cobra command for decoding base64 messages. Ensure that the command line arguments are correctly handled and that the command's help text is clear and informative.
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 #1731 +/- ##
==========================================
- Coverage 73.89% 73.88% -0.01%
==========================================
Files 192 193 +1
Lines 15142 15226 +84
==========================================
+ Hits 11189 11250 +61
- Misses 3297 3314 +17
- Partials 656 662 +6
|
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: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- cmd/nibid/cmd/decode_base64.go (1 hunks)
- cmd/nibid/cmd/decode_base64_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cmd/nibid/cmd/decode_base64_test.go
Additional comments: 6
cmd/nibid/cmd/decode_base64.go (6)
3-54: The import block includes a large number of dependencies. Verify that all these imports are necessary for this file, as unused imports should be removed to keep the code clean and maintainable.
192-202: The
YieldStargateMsgs
function correctly unmarshals JSON bytes and delegates the parsing of messages to theparseStargateMsgs
function. Ensure that the error handling is consistent and that the function performs as expected with various JSON inputs.204-223: The recursive function
parseStargateMsgs
parses JSON data and constructswasmvm.StargateMsg
objects. Verify that the recursion is correctly handling all possible JSON structures and that there are no potential stack overflow issues with deeply nested JSON. Additionally, consider the performance implications of recursion for large JSON inputs.225-228: The
StargateMsgDecoded
struct is simple and correctly annotated for JSON serialization. This struct should be sufficient for holding the decoded message data.230-275: The
DecodeBase64StargateMsgs
function decodes base64-encoded protobuf messages. Ensure that the base64 decoding is correctly handled and that the function is resilient to malformed input. Additionally, the error handling should be consistent, and the function should fail early if an error is encountered to avoid unnecessary processing.277-296: The
DecodeBase64Cmd
function sets up a Cobra command for decoding base64-encoded protobuf messages. Verify that the command setup is correct, including flag definitions and error handling. Ensure that the command's help text is clear and that the command behaves correctly when invoked with various arguments.
Co-authored-by: Kevin Yang <[email protected]>
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: 5
Configuration used: CodeRabbit UI
Files selected for processing (1)
- cmd/nibid/cmd/decode_base64.go (1 hunks)
Additional comments: 1
cmd/nibid/cmd/decode_base64.go (1)
- 3-53: The import statements are extensive and include a variety of types from different packages. Ensure that all imported packages are used within the file to avoid unnecessary imports which can lead to increased compilation time and binary size.
// YieldStargateMsgs parses the JSON and sends wasmvm.StargateMsg objects to a channel | ||
func YieldStargateMsgs(jsonBz []byte) ([]wasmvm.StargateMsg, error) { | ||
var data interface{} | ||
if err := json.Unmarshal(jsonBz, &data); err != nil { | ||
return nil, err | ||
} | ||
|
||
var msgs []wasmvm.StargateMsg | ||
parseStargateMsgs(data, &msgs) | ||
return msgs, nil | ||
} |
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 YieldStargateMsgs
function correctly unmarshals JSON bytes into an interface and then parses the messages. However, there is no error handling for the parseStargateMsgs
function call. Ensure that any errors during parsing are properly handled.
func parseStargateMsgs(jsonData any, msgs *[]wasmvm.StargateMsg) { | ||
switch v := jsonData.(type) { | ||
case map[string]interface{}: | ||
if typeURL, ok := v["type_url"].(string); ok { | ||
if value, ok := v["value"].(string); ok { | ||
*msgs = append(*msgs, wasmvm.StargateMsg{ | ||
TypeURL: typeURL, | ||
Value: []byte(value), | ||
}) | ||
} | ||
} | ||
for _, value := range v { | ||
parseStargateMsgs(value, msgs) | ||
} | ||
case []interface{}: | ||
for _, value := range v { | ||
parseStargateMsgs(value, msgs) | ||
} | ||
} | ||
} |
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 recursive function parseStargateMsgs
lacks error handling. If any of the type assertions fail, it will result in a runtime panic. It's important to handle these cases gracefully and return an error if the JSON structure does not meet the expected format.
…com:NibiruChain/nibiru into mat/decode-base64
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: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- cmd/nibid/cmd/decode_base64.go (1 hunks)
- cmd/nibid/cmd/decode_base64_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cmd/nibid/cmd/decode_base64.go
Additional comments: 2
cmd/nibid/cmd/decode_base64_test.go (2)
62-115: The test cases cover a range of scenarios, including valid and invalid JSON messages. However, the test case named "empty message" (lines 100-107) is marked to not expect an error, which might be misleading if the functionality is supposed to handle empty messages without errors. Clarification or adjustment of the test case might be needed.
117-119: The loop for executing test cases is correctly set up and follows best practices for table-driven tests in Go.
func TestBase64Decode(t *testing.T) { | ||
type TestCase struct { | ||
name string | ||
json_message string | ||
expectError bool | ||
} |
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 TestCase
struct is well-defined with relevant fields for the test's purpose. However, it's good practice to include a field for the expected result to compare against the actual result in tests, not just an error expectation.
executeTest := func(t *testing.T, testCase TestCase) { | ||
tc := testCase | ||
t.Run(tc.name, func(t *testing.T) { | ||
home := t.TempDir() | ||
logger := log.NewNopLogger() | ||
cfg, err := genutiltest.CreateDefaultTendermintConfig(home) | ||
require.NoError(t, err) | ||
|
||
appCodec := app.MakeEncodingConfig().Marshaler | ||
err = genutiltest.ExecInitCmd( | ||
testModuleBasicManager, home, appCodec) | ||
require.NoError(t, err) | ||
|
||
serverCtx := server.NewContext(viper.New(), cfg, logger) | ||
clientCtx := (client.Context{}. | ||
WithCodec(appCodec). | ||
WithHomeDir(home). | ||
WithInterfaceRegistry(app.MakeEncodingConfig().InterfaceRegistry)) | ||
|
||
ctx := context.Background() | ||
ctx = context.WithValue(ctx, client.ClientContextKey, &clientCtx) | ||
ctx = context.WithValue(ctx, server.ServerContextKey, serverCtx) | ||
|
||
cmd := nibid.DecodeBase64Cmd(home) | ||
cmd.SetArgs([]string{ | ||
tc.json_message, | ||
}) | ||
|
||
if tc.expectError { | ||
require.Error(t, cmd.ExecuteContext(ctx)) | ||
} else { | ||
require.NoError(t, cmd.ExecuteContext(ctx)) | ||
} | ||
}) | ||
} |
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 executeTest
function is well-structured and uses subtests, which is a good practice for clarity and isolation of test cases. However, the use of context.WithValue
should be accompanied by a comment explaining why it's necessary to store clientCtx
and serverCtx
in the context, as this is not a common practice for contexts in tests.
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)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 53-53: The changelog entry for the new CLI command to decode Stargate base64 messages is clear and follows the established format.
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)
- cmd/nibid/cmd/decode_base64.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cmd/nibid/cmd/decode_base64.go
clientCtx := client.GetClientContextFromCmd(cmd) | ||
|
||
outMessage, err := DecodeBase64StargateMsgs([]byte(args[0]), clientCtx) | ||
fmt.Println(outMessage) |
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.
Would put if err == nil, print so that it only runs on success
* wip * feat: have a dirty funny looking solution * chore: changelog * fix: fix replace usage * Update cmd/nibid/cmd/decode_base64.go Co-authored-by: Kevin Yang <[email protected]> * fix: turns out InterfaceRegistry.Resolve does the trick * chore: changelog * fix: remove unused import --------- Co-authored-by: Kevin Yang <[email protected]>
Purpose / Abstract
Summary by CodeRabbit
New Features
Tests
Documentation