diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ec01950f7..6ac24ddc3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -138,6 +138,7 @@ in `TxResponse` is deprecated and will be removed in the next major release. * [\#4979](https://github.com/cosmos/cosmos-sdk/issues/4979) Use `Signal(os.Interrupt)` over `os.Exit(0)` during configured halting to allow any `defer` calls to be executed. +* [\#5034](https://github.com/cosmos/cosmos-sdk/issues/5034) Binary search in NFT Module wasn't working on larger sets. ## [v0.37.0] - 2019-08-21 diff --git a/x/nft/client/cli/query.go b/x/nft/client/cli/query.go index d79f88ebde..22ae227058 100644 --- a/x/nft/client/cli/query.go +++ b/x/nft/client/cli/query.go @@ -169,10 +169,6 @@ $ %s query %s collection cripto-kitties } } -type stringArray []string - -func (s stringArray) String() string { return strings.Join(s[:], ",") } - // GetCmdQueryDenoms queries all denoms func GetCmdQueryDenoms(queryRoute string, cdc *codec.Codec) *cobra.Command { return &cobra.Command{ @@ -196,7 +192,7 @@ func GetCmdQueryDenoms(queryRoute string, cdc *codec.Codec) *cobra.Command { return err } - var out stringArray + var out types.SortedStringArray err = cdc.UnmarshalJSON(res, &out) if err != nil { return err @@ -216,7 +212,7 @@ func GetCmdQueryNFT(queryRoute string, cdc *codec.Codec) *cobra.Command { fmt.Sprintf(`Get an NFT from a collection that has the given ID (SHA-256 hex hash). Example: -$ %s query %s token cripto-kitties d04b98f48e8f8bcc15c6ae5ac050801cd6dcfd428fb5f9e65c4e16e7807340fa +$ %s query %s token crypto-kitties d04b98f48e8f8bcc15c6ae5ac050801cd6dcfd428fb5f9e65c4e16e7807340fa `, version.ClientName, types.ModuleName, ), ), diff --git a/x/nft/internal/keeper/querier.go b/x/nft/internal/keeper/querier.go index 2a08782f64..06c4ed3fb3 100644 --- a/x/nft/internal/keeper/querier.go +++ b/x/nft/internal/keeper/querier.go @@ -89,7 +89,7 @@ func queryOwnerByDenom(ctx sdk.Context, path []string, req abci.RequestQuery, k idCollection, _ := k.GetOwnerByDenom(ctx, params.Owner, params.Denom) owner.Address = params.Owner - owner.IDCollections = append(owner.IDCollections, idCollection) + owner.IDCollections = append(owner.IDCollections, idCollection).Sort() bz, err := types.ModuleCdc.MarshalJSON(owner) if err != nil { diff --git a/x/nft/internal/types/collection.go b/x/nft/internal/types/collection.go index a93c80e5e2..777439346c 100644 --- a/x/nft/internal/types/collection.go +++ b/x/nft/internal/types/collection.go @@ -31,10 +31,9 @@ func EmptyCollection() Collection { // GetNFT gets a NFT from the collection func (collection Collection) GetNFT(id string) (nft exported.NFT, err sdk.Error) { - for _, nft := range collection.NFTs { - if nft.GetID() == id { - return nft, nil - } + nft, found := collection.NFTs.Find(id) + if found { + return nft, nil } return nil, ErrUnknownNFT(DefaultCodespace, fmt.Sprintf("NFT #%s doesn't exist in collection %s", id, collection.Denom), @@ -56,7 +55,7 @@ func (collection Collection) AddNFT(nft exported.NFT) (Collection, sdk.Error) { fmt.Sprintf("NFT #%s already exists in collection %s", id, collection.Denom), ) } - collection.NFTs = append(collection.NFTs, nft) + collection.NFTs = collection.NFTs.Append(nft) return collection, nil } @@ -112,12 +111,12 @@ func NewCollections(collections ...Collection) Collections { if len(collections) == 0 { return Collections{} } - return Collections(collections) + return Collections(collections).Sort() } -// Add appends two sets of Collections -func (collections Collections) Add(collectionsB Collections) Collections { - return append(collections, collectionsB...) +// Append appends two sets of Collections +func (collections Collections) Append(collectionsB ...Collection) Collections { + return append(collections, collectionsB...).Sort() } // Find returns the searched collection from the set @@ -158,23 +157,7 @@ func (collections Collections) Empty() bool { } func (collections Collections) find(denom string) (idx int) { - if len(collections) == 0 { - return -1 - } - // TODO: ensure this is already sorted - // collections.Sort() - - midIdx := len(collections) / 2 - midCollection := collections[midIdx] - - switch { - case strings.Compare(denom, midCollection.Denom) == -1: - return collections[:midIdx].find(denom) - case midCollection.Denom == denom: - return midIdx - default: - return collections[midIdx+1:].find(denom) - } + return FindUtil(collections, denom) } // ---------------------------------------------------------------------------- @@ -212,10 +195,10 @@ func (collections *Collections) UnmarshalJSON(b []byte) error { } //----------------------------------------------------------------------------- -// Sort interface +// Sort & Findable interfaces -//nolint -func (collections Collections) Len() int { return len(collections) } +func (collections Collections) ElAtIndex(index int) string { return collections[index].Denom } +func (collections Collections) Len() int { return len(collections) } func (collections Collections) Less(i, j int) bool { return strings.Compare(collections[i].Denom, collections[j].Denom) == -1 } diff --git a/x/nft/internal/types/collection_test.go b/x/nft/internal/types/collection_test.go index 9551a4b999..f810e05535 100644 --- a/x/nft/internal/types/collection_test.go +++ b/x/nft/internal/types/collection_test.go @@ -172,9 +172,7 @@ func TestNewCollections(t *testing.T) { require.Equal(t, len(collections), 2) } - -func TestCollectionsAddMethod(t *testing.T) { - +func TestCollectionsAppendMethod(t *testing.T) { testNFT := NewBaseNFT(id, address, tokenURI) nfts := NewNFTs(&testNFT) collection := NewCollection(denom, nfts) @@ -186,7 +184,7 @@ func TestCollectionsAddMethod(t *testing.T) { collection2 := NewCollection(denom2, nfts2) collections2 := NewCollections(collection2) - collections = collections.Add(collections2) + collections = collections.Append(collections2...) require.Equal(t, len(collections), 2) } diff --git a/x/nft/internal/types/nft.go b/x/nft/internal/types/nft.go index 7a4e5489e1..3581503ea1 100644 --- a/x/nft/internal/types/nft.go +++ b/x/nft/internal/types/nft.go @@ -68,12 +68,12 @@ func NewNFTs(nfts ...exported.NFT) NFTs { if len(nfts) == 0 { return NFTs{} } - return NFTs(nfts) + return NFTs(nfts).Sort() } -// Add appends two sets of NFTs -func (nfts NFTs) Add(nftsB NFTs) NFTs { - return append(nfts, nftsB...) +// Append appends two sets of NFTs +func (nfts NFTs) Append(nftsB ...exported.NFT) NFTs { + return append(nfts, nftsB...).Sort() } // Find returns the searched collection from the set @@ -124,21 +124,7 @@ func (nfts NFTs) Empty() bool { } func (nfts NFTs) find(id string) int { - if len(nfts) == 0 { - return -1 - } - - midIdx := len(nfts) / 2 - nft := nfts[midIdx] - - switch { - case strings.Compare(id, nft.GetID()) == -1: - return nfts[:midIdx].find(id) - case id == nft.GetID(): - return midIdx - default: - return nfts[midIdx+1:].find(id) - } + return FindUtil(nfts, id) } // ---------------------------------------------------------------------------- @@ -172,17 +158,15 @@ func (nfts *NFTs) UnmarshalJSON(b []byte) error { return nil } -//----------------------------------------------------------------------------- -// Sort interface - -//nolint -func (nfts NFTs) Len() int { return len(nfts) } -func (nfts NFTs) Less(i, j int) bool { return strings.Compare(nfts[i].GetID(), nfts[j].GetID()) == -1 } -func (nfts NFTs) Swap(i, j int) { nfts[i], nfts[j] = nfts[j], nfts[i] } +// Findable and Sort interfaces +func (nfts NFTs) ElAtIndex(index int) string { return nfts[index].GetID() } +func (nfts NFTs) Len() int { return len(nfts) } +func (nfts NFTs) Less(i, j int) bool { return strings.Compare(nfts[i].GetID(), nfts[j].GetID()) == -1 } +func (nfts NFTs) Swap(i, j int) { nfts[i], nfts[j] = nfts[j], nfts[i] } var _ sort.Interface = NFTs{} -// Sort is a helper function to sort the set of coins inplace +// Sort is a helper function to sort the set of coins in place func (nfts NFTs) Sort() NFTs { sort.Sort(nfts) return nfts diff --git a/x/nft/internal/types/nft_test.go b/x/nft/internal/types/nft_test.go index 5e23d550f4..5e42a8fe6b 100644 --- a/x/nft/internal/types/nft_test.go +++ b/x/nft/internal/types/nft_test.go @@ -55,7 +55,7 @@ func TestNewNFTs(t *testing.T) { } -func TestNFTsAddMethod(t *testing.T) { +func TestNFTsAppendMethod(t *testing.T) { testNFT := NewBaseNFT(id, address, tokenURI) nfts := NewNFTs(&testNFT) require.Equal(t, len(nfts), 1) @@ -63,20 +63,67 @@ func TestNFTsAddMethod(t *testing.T) { testNFT2 := NewBaseNFT(id2, address, tokenURI) nfts2 := NewNFTs(&testNFT2) - nfts = nfts.Add(nfts2) + nfts = nfts.Append(nfts2...) require.Equal(t, len(nfts), 2) + + var id3 = string('3') + var id4 = string('4') + var id5 = string('5') + testNFT3 := NewBaseNFT(id3, address, tokenURI) + testNFT4 := NewBaseNFT(id4, address, tokenURI) + testNFT5 := NewBaseNFT(id5, address, tokenURI) + + nfts3 := NewNFTs(&testNFT5, &testNFT3, &testNFT4) + nfts = nfts.Append(nfts3...) + require.Equal(t, len(nfts), 5) + + nft, found := nfts.Find(id2) + require.True(t, found) + require.Equal(t, nft.String(), testNFT2.String()) + + nft, found = nfts.Find(id5) + require.True(t, found) + require.Equal(t, nft.String(), testNFT5.String()) + + nft, found = nfts.Find(id3) + require.True(t, found) + require.Equal(t, nft.String(), testNFT3.String()) } func TestNFTsFindMethod(t *testing.T) { testNFT := NewBaseNFT(id, address, tokenURI) testNFT2 := NewBaseNFT(id2, address, tokenURI) - nfts := NewNFTs(&testNFT, &testNFT2) + var id3 = string('3') + var id4 = string('4') + var id5 = string('5') + testNFT3 := NewBaseNFT(id3, address, tokenURI) + testNFT4 := NewBaseNFT(id4, address, tokenURI) + testNFT5 := NewBaseNFT(id5, address, tokenURI) + + nfts := NewNFTs(&testNFT, &testNFT3, &testNFT4, &testNFT5, &testNFT2) nft, found := nfts.Find(id) require.True(t, found) require.Equal(t, nft.String(), testNFT.String()) + nft, found = nfts.Find(id2) + require.True(t, found) + require.Equal(t, nft.String(), testNFT2.String()) + nft, found = nfts.Find(id3) + require.True(t, found) + require.Equal(t, nft.String(), testNFT3.String()) + + nft, found = nfts.Find(id4) + require.True(t, found) + require.Equal(t, nft.String(), testNFT4.String()) + + nft, found = nfts.Find(id5) + require.True(t, found) + require.Equal(t, nft.String(), testNFT5.String()) + + var id6 = string('6') + nft, found = nfts.Find(id6) require.False(t, found) require.Nil(t, nft) } diff --git a/x/nft/internal/types/owners.go b/x/nft/internal/types/owners.go index 0e6beed793..a17c6d52e1 100644 --- a/x/nft/internal/types/owners.go +++ b/x/nft/internal/types/owners.go @@ -2,6 +2,7 @@ package types import ( "fmt" + "sort" "strings" sdk "github.com/cosmos/cosmos-sdk/types" @@ -10,38 +11,39 @@ import ( // IDCollection defines a set of nft ids that belong to a specific // collection type IDCollection struct { - Denom string `json:"denom" yaml:"denom"` - IDs []string `json:"ids" yaml:"ids"` + Denom string `json:"denom" yaml:"denom"` + IDs SortedStringArray `json:"ids" yaml:"ids"` } +// SortedStringArray is an array of strings whose sole purpose is to help with find +type SortedStringArray []string + +// String is the string representation +func (sa SortedStringArray) String() string { return strings.Join(sa[:], ",") } + // NewIDCollection creates a new IDCollection instance func NewIDCollection(denom string, ids []string) IDCollection { return IDCollection{ Denom: strings.TrimSpace(denom), - IDs: ids, + IDs: SortedStringArray(ids).Sort(), } } // Exists determines whether an ID is in the IDCollection func (idCollection IDCollection) Exists(id string) (exists bool) { - // TODO: improve performance - for _, _id := range idCollection.IDs { - if _id == id { - return true - } - } - return false + index := idCollection.IDs.find(id) + return index != -1 } // AddID adds an ID to the idCollection func (idCollection IDCollection) AddID(id string) IDCollection { - idCollection.IDs = append(idCollection.IDs, id) + idCollection.IDs = append(idCollection.IDs, id).Sort() return idCollection } // DeleteID deletes an ID from an ID Collection func (idCollection IDCollection) DeleteID(id string) (IDCollection, sdk.Error) { - index := stringArray(idCollection.IDs).find(id) + index := idCollection.IDs.find(id) if index == -1 { return idCollection, ErrUnknownNFT(DefaultCodespace, fmt.Sprintf("ID #%s doesn't exist on ID Collection %s", id, idCollection.Denom), @@ -86,22 +88,12 @@ func (idCollections IDCollections) String() string { return out[:len(out)-1] } -func (idCollections IDCollections) find(el string) int { - if len(idCollections) == 0 { - return -1 - } - - midIdx := len(idCollections) / 2 - midIDCollection := idCollections[midIdx] - - switch { - case strings.Compare(el, midIDCollection.Denom) == -1: - return idCollections[:midIdx].find(el) - case midIDCollection.Denom == el: - return midIdx - default: - return idCollections[midIdx+1:].find(el) - } +// Append appends IDCollections to IDCollections +func (idCollections IDCollections) Append(idCollections2 ...IDCollection) IDCollections { + return append(idCollections, idCollections2...).Sort() +} +func (idCollections IDCollections) find(denom string) int { + return FindUtil(idCollections, denom) } // Owner of non fungible tokens @@ -179,24 +171,45 @@ func (owner Owner) String() string { owner.IDCollections.String(), ) } - -// stringArray is an array of strings whose sole purpose is to help with find -type stringArray []string - -func (sa stringArray) find(el string) (idx int) { - if len(sa) == 0 { - return -1 - } - - midIdx := len(sa) / 2 - stringArrayEl := sa[midIdx] - - switch { - case strings.Compare(el, stringArrayEl) == -1: - return sa[:midIdx].find(el) - case stringArrayEl == el: - return midIdx - default: - return sa[midIdx+1:].find(el) - } +func (sa SortedStringArray) find(el string) (idx int) { + return FindUtil(sa, el) +} + +//----------------------------------------------------------------------------- +// Sort and Findable interface for SortedStringArray + +func (sa SortedStringArray) ElAtIndex(index int) string { return sa[index] } +func (sa SortedStringArray) Len() int { return len(sa) } +func (sa SortedStringArray) Less(i, j int) bool { + return strings.Compare(sa[i], sa[j]) == -1 +} +func (sa SortedStringArray) Swap(i, j int) { + sa[i], sa[j] = sa[j], sa[i] +} + +var _ sort.Interface = SortedStringArray{} + +// Sort is a helper function to sort the set of strings in place +func (sa SortedStringArray) Sort() SortedStringArray { + sort.Sort(sa) + return sa +} + +// Sort and Findable interface for IDCollections + +func (idCollections IDCollections) ElAtIndex(index int) string { return idCollections[index].Denom } +func (idCollections IDCollections) Len() int { return len(idCollections) } +func (idCollections IDCollections) Less(i, j int) bool { + return strings.Compare(idCollections[i].Denom, idCollections[j].Denom) == -1 +} +func (idCollections IDCollections) Swap(i, j int) { + idCollections[i].Denom, idCollections[j].Denom = idCollections[j].Denom, idCollections[i].Denom +} + +var _ sort.Interface = IDCollections{} + +// Sort is a helper function to sort the set of strings in place +func (idCollections IDCollections) Sort() IDCollections { + sort.Sort(idCollections) + return idCollections } diff --git a/x/nft/internal/types/owners_test.go b/x/nft/internal/types/owners_test.go index c3ac374265..b8a2462e2f 100644 --- a/x/nft/internal/types/owners_test.go +++ b/x/nft/internal/types/owners_test.go @@ -17,7 +17,7 @@ func TestNewIDCollection(t *testing.T) { } func TestIDCollectionExistsMethod(t *testing.T) { - ids := []string{id, id2} + ids := []string{id2, id} idCollection := NewIDCollection(denom, ids) require.True(t, idCollection.Exists(id)) require.True(t, idCollection.Exists(id2)) diff --git a/x/nft/internal/types/utils.go b/x/nft/internal/types/utils.go new file mode 100644 index 0000000000..310d9f5980 --- /dev/null +++ b/x/nft/internal/types/utils.go @@ -0,0 +1,34 @@ +package types + +import "strings" + +// Findable is an interface for iterable types that allows the FindUtil function to work +type Findable interface { + ElAtIndex(index int) string + Len() int +} + +// FindUtil is a binary search funcion for types that support the Findable interface (elements must be sorted) +func FindUtil(group Findable, el string) int { + if group.Len() == 0 { + return -1 + } + low := 0 + high := group.Len() - 1 + median := 0 + for low <= high { + median = (low + high) / 2 + switch compare := strings.Compare(group.ElAtIndex(median), el); { + case compare == 0: + // if group[median].element == el + return median + case compare == -1: + // if group[median].element < el + low = median + 1 + default: + // if group[median].element > el + high = median - 1 + } + } + return -1 +} diff --git a/x/nft/simulation/genesis.go b/x/nft/simulation/genesis.go index 216ebc9633..34c264a12c 100644 --- a/x/nft/simulation/genesis.go +++ b/x/nft/simulation/genesis.go @@ -10,13 +10,13 @@ import ( ) const ( - kities = "crypto-kities" - doggos = "crypto-doggos" + kitties = "crypto-kitties" + doggos = "crypto-doggos" ) // RandomizedGenState generates a random GenesisState for nft func RandomizedGenState(simState *module.SimulationState) { - collections := types.NewCollections(types.NewCollection(kities, types.NFTs{}), types.NewCollection(doggos, types.NFTs{})) + collections := types.NewCollections(types.NewCollection(kitties, types.NFTs{}), types.NewCollection(doggos, types.NFTs{})) var ownerships []types.Owner for _, acc := range simState.Accounts { @@ -39,7 +39,7 @@ func RandomizedGenState(simState *module.SimulationState) { if err != nil { panic(err) } - idCollection = types.NewIDCollection(kities, []string{baseNFT.ID}) + idCollection = types.NewIDCollection(kitties, []string{baseNFT.ID}) } else { collections[1], err = collections[1].AddNFT(&baseNFT) if err != nil {