fix: update swap keys for possibly overlapped keys (#1365) (#1367)

* 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 b33165b.

* chore: add denom validation for string type denom

(cherry picked from commit e31eb43)

Co-authored-by: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com>

CHANGELOG.md

@@ -69,6 +69,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
 

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 {

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...)
 }

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)
+ })
+ }
+}

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
 }
 

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