From 8ea4861c59559cb7a28d5feb459f74fe235ef6c1 Mon Sep 17 00:00:00 2001 From: "jaeseung.bae" Date: Wed, 8 May 2024 22:53:42 +0900 Subject: [PATCH 1/6] fix: update swap keys for possibly overlapped keys --- x/fswap/keeper/keys.go | 27 +++++++++++++++---- x/fswap/keeper/keys_test.go | 53 +++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 x/fswap/keeper/keys_test.go 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..9e814e418c --- /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) + }) + } +} From f2ce0bf2bb1570caf6634c71e553109871ee88a9 Mon Sep 17 00:00:00 2001 From: "jaeseung.bae" Date: Wed, 8 May 2024 22:57:32 +0900 Subject: [PATCH 2/6] chore: lint fix --- x/fswap/keeper/keys_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/fswap/keeper/keys_test.go b/x/fswap/keeper/keys_test.go index 9e814e418c..9027b29a58 100644 --- a/x/fswap/keeper/keys_test.go +++ b/x/fswap/keeper/keys_test.go @@ -18,7 +18,7 @@ func TestSwapKey(t *testing.T) { 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")...)...)...), + // expectedKey: append(swapPrefix, append(append([]byte{byte(len("cony"))}, []byte("cony")...), append([]byte{byte(len("peb"))}, []byte("peb")...)...)...), }, } for _, tc := range tests { @@ -41,7 +41,7 @@ func TestSwappedKey(t *testing.T) { 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")...)...)...), + // expectedKey: append(swappedKeyPrefix, append(append([]byte{byte(len("cony"))}, []byte("cony")...), append([]byte{byte(len("peb"))}, []byte("peb")...)...)...), }, } for _, tc := range tests { From 9c06dafef940be2d2ca1f2b466658cb7347b0f99 Mon Sep 17 00:00:00 2001 From: "jaeseung.bae" Date: Wed, 8 May 2024 23:00:34 +0900 Subject: [PATCH 3/6] chore: update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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 From b33165bfea1bd671102f646cc140c4a0c5b79261 Mon Sep 17 00:00:00 2001 From: "jaeseung.bae" Date: Wed, 8 May 2024 23:27:05 +0900 Subject: [PATCH 4/6] chore: use address.LengthPrefix to check length --- x/fswap/keeper/keys.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/x/fswap/keeper/keys.go b/x/fswap/keeper/keys.go index e9a4652926..bd289809a9 100644 --- a/x/fswap/keeper/keys.go +++ b/x/fswap/keeper/keys.go @@ -1,5 +1,10 @@ package keeper +import ( + "github.com/Finschia/finschia-sdk/types/address" + sdkerrors "github.com/Finschia/finschia-sdk/types/errors" +) + var ( swapPrefix = []byte{0x01} swapStatsKey = []byte{0x02} @@ -19,18 +24,13 @@ func swappedKey(fromDenom, toDenom string) []byte { } 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 + lengthPrefixedFromDenom, err := address.LengthPrefix([]byte(fromDenom)) + if err != nil { + panic(sdkerrors.ErrInvalidRequest.Wrapf("fromDenom length should be max %d bytes, got %d", address.MaxAddrLen, len(fromDenom))) } - - return append([]byte{byte(bzLen)}, bz...) + lengthPrefixedToDenom, err := address.LengthPrefix([]byte(toDenom)) + if err != nil { + panic(sdkerrors.ErrInvalidRequest.Wrapf("toDenom length should be max %d bytes, got %d", address.MaxAddrLen, len(toDenom))) + } + return append(lengthPrefixedFromDenom, lengthPrefixedToDenom...) } From c76058ce945d6b01202dc3e9fab5c602bb52cee8 Mon Sep 17 00:00:00 2001 From: "jaeseung.bae" Date: Wed, 8 May 2024 23:59:06 +0900 Subject: [PATCH 5/6] Revert "chore: use address.LengthPrefix to check length" This reverts commit b33165bfea1bd671102f646cc140c4a0c5b79261. --- x/fswap/keeper/keys.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/x/fswap/keeper/keys.go b/x/fswap/keeper/keys.go index bd289809a9..e9a4652926 100644 --- a/x/fswap/keeper/keys.go +++ b/x/fswap/keeper/keys.go @@ -1,10 +1,5 @@ package keeper -import ( - "github.com/Finschia/finschia-sdk/types/address" - sdkerrors "github.com/Finschia/finschia-sdk/types/errors" -) - var ( swapPrefix = []byte{0x01} swapStatsKey = []byte{0x02} @@ -24,13 +19,18 @@ func swappedKey(fromDenom, toDenom string) []byte { } func combineDenoms(fromDenom, toDenom string) []byte { - lengthPrefixedFromDenom, err := address.LengthPrefix([]byte(fromDenom)) - if err != nil { - panic(sdkerrors.ErrInvalidRequest.Wrapf("fromDenom length should be max %d bytes, got %d", address.MaxAddrLen, len(fromDenom))) - } - lengthPrefixedToDenom, err := address.LengthPrefix([]byte(toDenom)) - if err != nil { - panic(sdkerrors.ErrInvalidRequest.Wrapf("toDenom length should be max %d bytes, got %d", address.MaxAddrLen, len(toDenom))) - } + 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...) +} From 0b9cc3c2f869cd29bd3a16bb7902232eccba39dc Mon Sep 17 00:00:00 2001 From: "jaeseung.bae" Date: Thu, 9 May 2024 00:09:51 +0900 Subject: [PATCH 6/6] chore: add denom validation for string type denom --- x/fswap/keeper/grpc_query.go | 13 +++++++++++++ x/fswap/types/fswap.go | 13 +++++++++---- x/fswap/types/msgs.go | 12 ++++++------ 3 files changed, 28 insertions(+), 10 deletions(-) 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/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