Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TokenIndexes MultiIndex #64

Merged
merged 2 commits into from
May 27, 2022
Merged

Fix TokenIndexes MultiIndex #64

merged 2 commits into from
May 27, 2022

Conversation

humanalgorithm
Copy link
Contributor

@humanalgorithm humanalgorithm commented May 26, 2022

The Multiindex for TokenIndexes works by accident and not by design

The key on a token index should be a String and not an Addr
The conversion of Addr to String is why existing setup does not fail any unit tests

An example usage below:
If we have this data

"1": {owner: <addr1>, name: name1} 
"2": {owner: <addr1>, name: name2} 
"3": {owner: <addr2>, name: name3} 

The type structure being

Map {
String: TokenInfo {
       "owner": <Addr>
       "data": <somedata>
    }, 
String: TokenInfo {
        "owner": <Addr>
        "data": <somedata>
    }, 
    ...
 }

To uniquely identify each entry, I need to differentiate by the key, which in this case is a String
The purpose of the index, is to get token ids by providing an owner address. So,
input: <addr1>
output: ["1", "2"]

so the index is by owner and then I get back the keys associated with it ["1", "2"]

Note that an owner (addr) can have multiple token ids (string)
So index by owner, is similar to tokens "group by" owner

@shanev shanev changed the title Fix TokenIndexes Multiindex Fix TokenIndexes MultiIndex May 26, 2022
Copy link
Member

@shanev shanev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! :P

Copy link
Contributor

@orkunkl orkunkl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!

@orkunkl orkunkl self-requested a review May 27, 2022 10:40
Copy link
Contributor

@orkunkl orkunkl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I made the change back when the development speed was fast.
it is funny that,Addr is an abstraction to allow different address format validation, but in storage, it is simply an utf8 string. Lesson learned!

Copy link
Collaborator

@the-frey the-frey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, good spot.

@the-frey the-frey merged commit 4e26419 into public-awesome:main May 27, 2022
@shanev shanev deleted the humanalgorithm/fix-tokenindexes-multiindex branch May 28, 2022 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants