From e31eb434ba0fc34408cfc302c8bca62f99a12cdc Mon Sep 17 00:00:00 2001 From: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com> Date: Thu, 9 May 2024 00:51:38 +0900 Subject: [PATCH] fix: update swap keys for possibly overlapped keys (#1365) * fix: update swap keys for possibly overlapped keys * chore: lint fix * chore: update changelog * chore: use address.LengthPrefix to check length * Revert "chore: use address.LengthPrefix to check length" This reverts commit b33165bfea1bd671102f646cc140c4a0c5b79261. * chore: add denom validation for string type denom --- CHANGELOG.md | 1 + x/fswap/keeper/grpc_query.go | 13 +++++++++ x/fswap/keeper/keys.go | 27 ++++++++++++++---- x/fswap/keeper/keys_test.go | 53 ++++++++++++++++++++++++++++++++++++ x/fswap/types/fswap.go | 13 ++++++--- x/fswap/types/msgs.go | 12 ++++---- 6 files changed, 104 insertions(+), 15 deletions(-) create mode 100644 x/fswap/keeper/keys_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index c14f707a2f..9b21e67aa5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/mint, x/slashing) [\#1323](https://github.com/Finschia/finschia-sdk/pull/1323) add missing nil check for params validation * (x/server) [\#1337](https://github.com/Finschia/finschia-sdk/pull/1337) fix panic when defining minimum gas config as `100stake;100uatom`. Use a `,` delimiter instead of `;`. Fixes the server config getter to use the correct delimiter (backport cosmos/cosmos-sdk#18537) * (x/fbridge) [\#1361](https://github.com/Finschia/finschia-sdk/pull/1361) Fixes fbridge auth checking bug +* (x/fswap) [\#1365](https://github.com/Finschia/finschia-sdk/pull/1365) fix update swap keys for possibly overlapped keys(`(hello,world) should be different to (hel,loworld)`) ### Removed diff --git a/x/fswap/keeper/grpc_query.go b/x/fswap/keeper/grpc_query.go index def3ee5ffb..8e2e94a5e2 100644 --- a/x/fswap/keeper/grpc_query.go +++ b/x/fswap/keeper/grpc_query.go @@ -5,6 +5,7 @@ import ( "github.com/Finschia/finschia-sdk/store/prefix" sdk "github.com/Finschia/finschia-sdk/types" + sdkerrors "github.com/Finschia/finschia-sdk/types/errors" "github.com/Finschia/finschia-sdk/types/query" "github.com/Finschia/finschia-sdk/x/fswap/types" ) @@ -23,6 +24,12 @@ func NewQueryServer(keeper Keeper) *QueryServer { func (s QueryServer) Swapped(ctx context.Context, req *types.QuerySwappedRequest) (*types.QuerySwappedResponse, error) { c := sdk.UnwrapSDKContext(ctx) + if err := sdk.ValidateDenom(req.GetFromDenom()); err != nil { + return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) + } + if err := sdk.ValidateDenom(req.GetToDenom()); err != nil { + return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) + } swapped, err := s.Keeper.getSwapped(c, req.GetFromDenom(), req.GetToDenom()) if err != nil { @@ -37,6 +44,12 @@ func (s QueryServer) Swapped(ctx context.Context, req *types.QuerySwappedRequest func (s QueryServer) TotalSwappableToCoinAmount(ctx context.Context, req *types.QueryTotalSwappableToCoinAmountRequest) (*types.QueryTotalSwappableToCoinAmountResponse, error) { c := sdk.UnwrapSDKContext(ctx) + if err := sdk.ValidateDenom(req.GetFromDenom()); err != nil { + return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) + } + if err := sdk.ValidateDenom(req.GetToDenom()); err != nil { + return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) + } amount, err := s.Keeper.getSwappableNewCoinAmount(c, req.GetFromDenom(), req.GetToDenom()) if err != nil { diff --git a/x/fswap/keeper/keys.go b/x/fswap/keeper/keys.go index 04e36660f8..e9a4652926 100644 --- a/x/fswap/keeper/keys.go +++ b/x/fswap/keeper/keys.go @@ -8,12 +8,29 @@ var ( // swapKey key(prefix + fromDenom + toDenom) func swapKey(fromDenom, toDenom string) []byte { - key := append(swapPrefix, fromDenom...) - return append(key, toDenom...) + denoms := combineDenoms(fromDenom, toDenom) + return append(swapPrefix, denoms...) } -// swappedKey key(prefix + fromDenom + toDenom) +// swappedKey key(prefix + (lengthPrefixed+)fromDenom + (lengthPrefixed+)toDenom) func swappedKey(fromDenom, toDenom string) []byte { - key := append(swappedKeyPrefix, fromDenom...) - return append(key, toDenom...) + denoms := combineDenoms(fromDenom, toDenom) + return append(swappedKeyPrefix, denoms...) +} + +func combineDenoms(fromDenom, toDenom string) []byte { + lengthPrefixedFromDenom := lengthPrefix([]byte(fromDenom)) + lengthPrefixedToDenom := lengthPrefix([]byte(toDenom)) + return append(lengthPrefixedFromDenom, lengthPrefixedToDenom...) +} + +// lengthPrefix prefixes the address bytes with its length, this is used +// for example for variable-length components in store keys. +func lengthPrefix(bz []byte) []byte { + bzLen := len(bz) + if bzLen == 0 { + return bz + } + + return append([]byte{byte(bzLen)}, bz...) } diff --git a/x/fswap/keeper/keys_test.go b/x/fswap/keeper/keys_test.go new file mode 100644 index 0000000000..9027b29a58 --- /dev/null +++ b/x/fswap/keeper/keys_test.go @@ -0,0 +1,53 @@ +package keeper + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestSwapKey(t *testing.T) { + tests := []struct { + name string + fromDenom string + toDenom string + expectedKey []byte + }{ + { + name: "swapKey", + fromDenom: "cony", + toDenom: "peb", + expectedKey: []byte{0x1, 0x4, 0x63, 0x6f, 0x6e, 0x79, 0x3, 0x70, 0x65, 0x62}, + // expectedKey: append(swapPrefix, append(append([]byte{byte(len("cony"))}, []byte("cony")...), append([]byte{byte(len("peb"))}, []byte("peb")...)...)...), + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + actualKey := swapKey(tc.fromDenom, tc.toDenom) + require.Equal(t, tc.expectedKey, actualKey) + }) + } +} + +func TestSwappedKey(t *testing.T) { + tests := []struct { + name string + fromDenom string + toDenom string + expectedKey []byte + }{ + { + name: "swappedKey", + fromDenom: "cony", + toDenom: "peb", + expectedKey: []byte{0x3, 0x4, 0x63, 0x6f, 0x6e, 0x79, 0x3, 0x70, 0x65, 0x62}, + // expectedKey: append(swappedKeyPrefix, append(append([]byte{byte(len("cony"))}, []byte("cony")...), append([]byte{byte(len("peb"))}, []byte("peb")...)...)...), + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + actualKey := swappedKey(tc.fromDenom, tc.toDenom) + require.Equal(t, tc.expectedKey, actualKey) + }) + } +} diff --git a/x/fswap/types/fswap.go b/x/fswap/types/fswap.go index c307c826b3..f45d2b2d89 100644 --- a/x/fswap/types/fswap.go +++ b/x/fswap/types/fswap.go @@ -9,21 +9,26 @@ import ( // ValidateBasic validates the set of Swap func (s *Swap) ValidateBasic() error { - if s.FromDenom == "" { - return sdkerrors.ErrInvalidRequest.Wrap("from denomination cannot be empty") + if err := sdk.ValidateDenom(s.FromDenom); err != nil { + return sdkerrors.ErrInvalidRequest.Wrap(err.Error()) } - if s.ToDenom == "" { - return sdkerrors.ErrInvalidRequest.Wrap("to denomination cannot be empty") + + if err := sdk.ValidateDenom(s.ToDenom); err != nil { + return sdkerrors.ErrInvalidRequest.Wrap(err.Error()) } + if s.FromDenom == s.ToDenom { return sdkerrors.ErrInvalidRequest.Wrap("from denomination cannot be equal to to denomination") } + if s.AmountCapForToDenom.LT(sdk.OneInt()) { return sdkerrors.ErrInvalidRequest.Wrap("amount cannot be less than one") } + if s.SwapRate.IsZero() { return sdkerrors.ErrInvalidRequest.Wrap("swap rate cannot be zero") } + return nil } diff --git a/x/fswap/types/msgs.go b/x/fswap/types/msgs.go index eae8c6a2c2..f94e123cae 100644 --- a/x/fswap/types/msgs.go +++ b/x/fswap/types/msgs.go @@ -23,8 +23,8 @@ func (m *MsgSwap) ValidateBasic() error { return sdkerrors.ErrInvalidCoins.Wrap(m.FromCoinAmount.String()) } - if len(m.GetToDenom()) == 0 { - return sdkerrors.ErrInvalidRequest.Wrap("invalid denom (empty denom)") + if err := sdk.ValidateDenom(m.GetToDenom()); err != nil { + return sdkerrors.ErrInvalidRequest.Wrap(err.Error()) } return nil @@ -53,12 +53,12 @@ func (m *MsgSwapAll) ValidateBasic() error { return sdkerrors.ErrInvalidAddress.Wrapf("Invalid address (%s)", err) } - if len(m.GetFromDenom()) == 0 { - return sdkerrors.ErrInvalidRequest.Wrap("invalid denom (empty denom)") + if err := sdk.ValidateDenom(m.FromDenom); err != nil { + return sdkerrors.ErrInvalidRequest.Wrap(err.Error()) } - if len(m.GetToDenom()) == 0 { - return sdkerrors.ErrInvalidRequest.Wrap("invalid denom (empty denom)") + if err := sdk.ValidateDenom(m.ToDenom); err != nil { + return sdkerrors.ErrInvalidRequest.Wrap(err.Error()) } return nil