R4R: NFT find is broken

CHANGELOG.md

@@ -116,6 +116,7 @@ in `TxResponse` is deprecated and will be removed in the next major release.
 * (keys) Fix ledger custom coin type support bug
 * [\#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
 

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,
 			),
 		),

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 {

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
 }

x/nft/internal/types/collection_test.go

@@ -173,7 +173,7 @@ func TestNewCollections(t *testing.T) {
 
 }
 
-func TestCollectionsAddMethod(t *testing.T) {
+func TestCollectionsAppendMethod(t *testing.T) {
 
 	testNFT := NewBaseNFT(id, address, tokenURI)
 	nfts := NewNFTs(&testNFT)
@@ -186,7 +186,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)
 
 }

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

x/nft/internal/types/nft_test.go

@@ -55,28 +55,75 @@ 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)
 
 	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)
 }