R4R: NFT find is broken (#5035)

* nft find is broken

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
 

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

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

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