LaconicNetwork/cosmos-sdk
7
0
Fork 0
Code Issues Pull Requests Packages Projects Releases 2 Wiki Activity

R4R: NFT find is broken (#5035)

Browse Source 
* nft find is broken
This commit is contained in:
billy rennekamp 2019-09-30 10:39:07 -07:00 committed by Federico Kunze
parent 46cd6112e1
commit 6db739823b
11 changed files with 179 additions and 123 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

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

8
x/nft/client/cli/query.go
View File

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

2
x/nft/internal/keeper/querier.go
View File

@ -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 {

41
x/nft/internal/types/collection.go
View File

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

6
x/nft/internal/types/collection_test.go
View File

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

38
x/nft/internal/types/nft.go
View File

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

53
x/nft/internal/types/nft_test.go
View File

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

109
x/nft/internal/types/owners.go
View File

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

2
x/nft/internal/types/owners_test.go
View File

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

34
x/nft/internal/types/utils.go Normal file
View File

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

8
x/nft/simulation/genesis.go
View File

@ -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 {