Skip to content
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.

Sign up for GitHub

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

fix: allow string callback id #315

Merged
merged 2 commits into from
Dec 3, 2024
Merged

fix: allow string callback id #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31e1cc1
fix: allow string callback id
beer-1 Dec 3, 2024
6a93737
align variable name
beer-1 Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"github.com/cosmos/cosmos-sdk/types/module"
)

const upgradeName = "0.6.2"
const upgradeName = "0.6.4"

// RegisterUpgradeHandlers returns upgrade handlers
func (app *InitiaApp) RegisterUpgradeHandlers(cfg module.Configurator) {
Expand Down
44 changes: 44 additions & 0 deletions x/ibc-hooks/move-hooks/message.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package move_hooks

import (
"encoding/json"

movetypes "github.com/initia-labs/initia/x/move/types"
)

Expand Down Expand Up @@ -50,3 +52,45 @@ type HookData struct {
// sender chain.
AsyncCallback *AsyncCallback `json:"async_callback,omitempty"`
}

// asyncCallback is same as AsyncCallback.
type asyncCallback struct {
// callback id should be issued form the executor contract
Id uint64 `json:"id"`
ModuleAddress string `json:"module_address"`
ModuleName string `json:"module_name"`
}

// asyncCallbackStringID is same as AsyncCallback but
// it has Id as string.
type asyncCallbackStringID struct {
// callback id should be issued form the executor contract
Id uint64 `json:"id,string"`
ModuleAddress string `json:"module_address"`
ModuleName string `json:"module_name"`
}

// UnmarshalJSON implements the json unmarshaler interface.
// custom unmarshaler is required because we have to handle
// id as string and uint64.
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
}
Comment on lines +76 to +96
Copy link

@coderabbitai coderabbitai bot Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error handling and add validation

The UnmarshalJSON implementation could be improved in several ways:

  1. Add validation for ModuleAddress format
  2. Return more specific error messages
  3. 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"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make it as issue for later update

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

33 changes: 33 additions & 0 deletions x/ibc-hooks/move-hooks/message_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package move_hooks_test

import (
"encoding/json"
"testing"

movehooks "github.com/initia-labs/initia/x/ibc-hooks/move-hooks"
"github.com/stretchr/testify/require"
)

func Test_Unmarshal_AsyncCallback(t *testing.T) {
var callback movehooks.AsyncCallback
err := json.Unmarshal([]byte(`{
"id": 99,
"module_address": "0x1",
"module_name": "Counter"
}`), &callback)
require.NoError(t, err)
require.Equal(t, movehooks.AsyncCallback{
Id: 99,
ModuleAddress: "0x1",
ModuleName: "Counter",
}, callback)

var callbackStringID movehooks.AsyncCallback
err = json.Unmarshal([]byte(`{
"id": "99",
"module_address": "0x1",
"module_name": "Counter"
}`), &callbackStringID)
require.NoError(t, err)
require.Equal(t, callback, callbackStringID)
}
Loading