fix: allow string callback id #315
Conversation
📝 WalkthroughWalkthroughThe pull request includes updates to two files: Changes
Possibly related PRs
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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
![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 (3)
x/ibc-hooks/move-hooks/message_test.go (1)
11-33: Add test cases for edge scenariosWhile the current tests cover the basic functionality, consider adding test cases for:
- Invalid JSON format
- Negative numbers
- Numbers exceeding uint64 max value
- Malformed string IDs (e.g., "abc", "-123", "1.23")
Here's a suggested addition:
func Test_Unmarshal_AsyncCallback(t *testing.T) { + testCases := []struct { + name string + json string + wantErr bool + want movehooks.AsyncCallback + }{ + { + name: "valid uint64", + json: `{"id": 99, "module_address": "0x1", "module_name": "Counter"}`, + want: movehooks.AsyncCallback{Id: 99, ModuleAddress: "0x1", ModuleName: "Counter"}, + }, + { + name: "valid string id", + json: `{"id": "99", "module_address": "0x1", "module_name": "Counter"}`, + want: movehooks.AsyncCallback{Id: 99, ModuleAddress: "0x1", ModuleName: "Counter"}, + }, + { + name: "negative number", + json: `{"id": -1, "module_address": "0x1", "module_name": "Counter"}`, + wantErr: true, + }, + { + name: "overflow uint64", + json: `{"id": 18446744073709551616, "module_address": "0x1", "module_name": "Counter"}`, + wantErr: true, + }, + { + name: "invalid string id", + json: `{"id": "abc", "module_address": "0x1", "module_name": "Counter"}`, + wantErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var callback movehooks.AsyncCallback + err := json.Unmarshal([]byte(tc.json), &callback) + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tc.want, callback) + }) + }x/ibc-hooks/move-hooks/message.go (2)
56-62: Consider using type embedding to reduce code duplicationThe
asyncCallbackstruct is identical toAsyncCallback. Consider using type embedding to reduce maintenance burden:-type asyncCallback struct { - Id uint64 `json:"id"` - ModuleAddress string `json:"module_address"` - ModuleName string `json:"module_name"` -} +type asyncCallback AsyncCallback
64-71: Consider using type embedding for string ID variantSimilar to the previous comment,
asyncCallbackStringIDcould use type embedding with just the ID field being different:-type asyncCallbackStringID struct { - Id uint64 `json:"id,string"` - ModuleAddress string `json:"module_address"` - ModuleName string `json:"module_name"` -} +type asyncCallbackStringID struct { + AsyncCallback `json:",inline"` + Id uint64 `json:"id,string"` +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/upgrade.go(1 hunks)x/ibc-hooks/move-hooks/message.go(2 hunks)x/ibc-hooks/move-hooks/message_test.go(1 hunks)
🔇 Additional comments (1)
app/upgrade.go (1)
10-10: Verify version bump from 0.6.2 to 0.6.4
The version increment seems large for this change. Could you clarify if there are other changes that justify skipping 0.6.3?
| func (a *AsyncCallback) UnmarshalJSON(bz []byte) error { | ||
| var ac asyncCallback | ||
| err := json.Unmarshal(bz, &ac) | ||
| if err != nil { | ||
| var acStr asyncCallbackStringID | ||
| err := json.Unmarshal(bz, &acStr) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| a.Id = acStr.Id | ||
| a.ModuleAddress = acStr.ModuleAddress | ||
| a.ModuleName = acStr.ModuleName | ||
| return nil | ||
| } | ||
|
|
||
| a.Id = ac.Id | ||
| a.ModuleAddress = ac.ModuleAddress | ||
| a.ModuleName = ac.ModuleName | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add validation
The UnmarshalJSON implementation could be improved in several ways:
- Add validation for ModuleAddress format
- Return more specific error messages
- Optimize memory allocation by using a single intermediate struct
func (a *AsyncCallback) UnmarshalJSON(bz []byte) error {
- var ac asyncCallback
- err := json.Unmarshal(bz, &ac)
- if err != nil {
- var acStr asyncCallbackStringID
- err := json.Unmarshal(bz, &acStr)
- if err != nil {
- return err
- }
+ var raw struct {
+ Id interface{} `json:"id"`
+ ModuleAddress string `json:"module_address"`
+ ModuleName string `json:"module_name"`
+ }
- a.Id = acStr.Id
- a.ModuleAddress = acStr.ModuleAddress
- a.ModuleName = acStr.ModuleName
- return nil
- }
+ if err := json.Unmarshal(bz, &raw); err != nil {
+ return fmt.Errorf("failed to unmarshal AsyncCallback: %w", err)
+ }
- a.Id = ac.Id
- a.ModuleAddress = ac.ModuleAddress
- a.ModuleName = ac.ModuleName
+ // Validate ModuleAddress format
+ if !strings.HasPrefix(raw.ModuleAddress, "0x") {
+ return fmt.Errorf("invalid module address format: %s", raw.ModuleAddress)
+ }
+
+ // Handle both string and number ID formats
+ switch v := raw.Id.(type) {
+ case float64:
+ if v < 0 || v > math.MaxUint64 {
+ return fmt.Errorf("id %v out of range for uint64", v)
+ }
+ a.Id = uint64(v)
+ case string:
+ id, err := strconv.ParseUint(v, 10, 64)
+ if err != nil {
+ return fmt.Errorf("invalid string id %q: %w", v, err)
+ }
+ a.Id = id
+ default:
+ return fmt.Errorf("id must be a number or string, got %T", raw.Id)
+ }
+
+ a.ModuleAddress = raw.ModuleAddress
+ a.ModuleName = raw.ModuleName
return nil
}Don't forget to add the required imports:
import (
"fmt"
"math"
"strconv"
"strings"
)There was a problem hiding this comment.
Please make it as issue for later update
There was a problem hiding this comment.
@beer-1, I have created a new GitHub issue to track this enhancement: #317.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Enhance error handling and add validation in UnmarshalJSON" \
--body "As per the discussion in PR #315 (https://github.com/initia-labs/initia/pull/315#discussion_r1867109085), @beer-1 requested to enhance the error handling and add validation in the UnmarshalJSON implementation of AsyncCallback. This issue will track the enhancement for a future update." \
--assignee beer-1
Length of output: 448
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #315 +/- ##
==========================================
+ Coverage 41.11% 41.14% +0.02%
==========================================
Files 268 269 +1
Lines 25682 25699 +17
==========================================
+ Hits 10559 10573 +14
- Misses 13486 13488 +2
- Partials 1637 1638 +1
|
Description
The IBC callback is using a standard JSON decoder, which allows only numbers as valid async callback IDs. However, our MoveVM JSON encoder encodes uint64 values as strings. To maintain consistency with this implementation, our
AsyncCallbackuses a customUnmarshalJSONmethod, which accommodates both string and numeric callback IDs.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...