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

Refactor panic to use return error #6391

Merged
merged 6 commits into from
May 29, 2024
Merged

Refactor panic to use return error #6391

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f4f3989
chore: refactor panic to use return error
chatton May 27, 2024
89281e4
Merge branch 'main' into cian/issue#6387-clean-up-panics-in-ics20v2-c…
chatton May 28, 2024
f6eb261
chore: addressing PR feedback
chatton May 28, 2024
4c88c46
Merge branch 'main' into cian/issue#6387-clean-up-panics-in-ics20v2-c…
crodriguezvega May 28, 2024
a53759b
Merge branch 'main' into cian/issue#6387-clean-up-panics-in-ics20v2-c…
crodriguezvega May 28, 2024
72ab5ec
Merge branch 'main' into cian/issue#6387-clean-up-panics-in-ics20v2-c…
chatton May 29, 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
6 changes: 1 addition & 5 deletions modules/apps/transfer/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,11 +184,7 @@ func (IBCModule) unmarshalPacketDataBytesToICS20V2(bz []byte, ics20Version strin
return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V1 transfer packet data: %s", err.Error())
}

if err := datav1.ValidateBasic(); err != nil {
return types.FungibleTokenPacketDataV2{}, err
}

return convertinternal.PacketDataV1ToV2(datav1), nil
return convertinternal.PacketDataV1ToV2(datav1)
case types.V2:
var datav2 types.FungibleTokenPacketDataV2
if err := json.Unmarshal(bz, &datav2); err != nil {
Expand Down
14 changes: 9 additions & 5 deletions modules/apps/transfer/internal/convert/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
package convert

import (
"fmt"
"strings"

errorsmod "cosmossdk.io/errors"

"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
)

// PacketDataV1ToV2 converts a v1 packet data to a v2 packet data.
func PacketDataV1ToV2(packetData types.FungibleTokenPacketData) types.FungibleTokenPacketDataV2 {
// PacketDataV1ToV2 converts a v1 packet data to a v2 packet data. The packet data is validated
// before conversion.
func PacketDataV1ToV2(packetData types.FungibleTokenPacketData) (types.FungibleTokenPacketDataV2, error) {
if err := packetData.ValidateBasic(); err != nil {
panic(err)
return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(err, "invalid packet data")
}

v2Denom, trace := ExtractDenomAndTraceFromV1Denom(packetData.Denom)
Expand All @@ -24,7 +28,7 @@ func PacketDataV1ToV2(packetData types.FungibleTokenPacketData) types.FungibleTo
Sender: packetData.Sender,
Receiver: packetData.Receiver,
Memo: packetData.Memo,
}
}, nil
}

// extractDenomAndTraceFromV1Denom extracts the base denom and remaining trace from a v1 IBC denom.
Expand All @@ -40,7 +44,7 @@ func ExtractDenomAndTraceFromV1Denom(v1Denom string) (string, []string) {

// this condition should never be reached.
if len(splitPath)%2 != 0 {
panic("pathSlice length is not even")
panic(fmt.Errorf("path slice length is not even"))
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider replacing panic with error handling to maintain consistency in error handling across the codebase.

}

// the path slices consists of entries of ports and channel ids separately,
Expand Down
14 changes: 7 additions & 7 deletions modules/apps/transfer/internal/convert/convert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func TestConvertPacketV1ToPacketV2(t *testing.T) {
name string
v1Data types.FungibleTokenPacketData
v2Data types.FungibleTokenPacketDataV2
expPanic error
expError error
Copy link
Contributor

Choose a reason for hiding this comment

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

Updated test cases to reflect new error handling in PacketDataV1ToV2.

Consider adding more edge cases to ensure comprehensive coverage.

Would you like assistance in identifying potential edge cases for additional tests?

}{
{
"success",
Expand Down Expand Up @@ -114,22 +114,22 @@ func TestConvertPacketV1ToPacketV2(t *testing.T) {
nil,
},
{
"failure: panics with empty denom",
"failure: packet data fails validation with empty denom",
types.NewFungibleTokenPacketData("", "1000", sender, receiver, ""),
types.FungibleTokenPacketDataV2{},
errorsmod.Wrap(types.ErrInvalidDenomForTransfer, "base denomination cannot be blank"),
},
}

for _, tc := range testCases {
expPass := tc.expPanic == nil
actualV2Data, err := PacketDataV1ToV2(tc.v1Data)

expPass := tc.expError == nil
if expPass {
actualV2Data := PacketDataV1ToV2(tc.v1Data)
require.NoError(t, err, "test case: %s", tc.name)
require.Equal(t, tc.v2Data, actualV2Data, "test case: %s", tc.name)
} else {
require.PanicsWithError(t, tc.expPanic.Error(), func() {
PacketDataV1ToV2(tc.v1Data)
}, "test case: %s", tc.name)
require.Error(t, err, "test case: %s", tc.name)
}
}
}
Loading