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

don't approve me, work in progress for cryptokitties xml #39

Closed
wants to merge 2 commits into from

Conversation

bitcoinwarrior1
Copy link
Contributor

@bitcoinwarrior1 bitcoinwarrior1 commented Aug 4, 2018

for #34
@James-Sangalli sorted stuff into sub-directories in this pR>

@hboon
Copy link
Member

hboon commented Aug 4, 2018

@James-Sangalli

  1. Is it supposed to be complete? For example, there's "coralsunrise" for eye-colour that isn't in the XML.

  2. For https://www.cryptokitties.co/kitty/800058, I tried mapping with getKitty(800058) (and getting genes) to get 689278284593481476509607714409998114129040247814513095176469160709554223 which converted to binary is 011000111101111011000000101001011010010001010000110001100011000100001000010011110110000110001010111101111010010111101111000110010101101000100010101000010100111101100000000111000001000000111101111010110001000001001000101000111000000000101111

And with the XML, I'm getting:

pattern-type 00100 = 4 = ? (should be amur)
eye-color 00011 = 3 = mintgreen (should be coralsunrise)

Am I doing it wrong?

  1. We need to call getKitty(kittyId) (genes is the last element in the result) and this isn't part of ERC721. Maybe we need a way to define the getKitty() call and how to interpret the results in the asset definition too? Or hardcode that call for CryptoKitties until we can generalise this?

Update: was looking up wrong kitty ID (80059 instead of 80058). Have updated above

@hboon
Copy link
Member

hboon commented Aug 4, 2018

Also, do we support specifying the bitmask as both binary and hex strings? i.e. the parser has to read the length to determine if it is binary/hex? Or should we just use hex? (kitty's is in binary and ticket's is in hex now)

@bitcoinwarrior1
Copy link
Contributor Author

@hboon hmmm, seems the image I am basing off from the article does not match the reality?

@bitcoinwarrior1
Copy link
Contributor Author

@colourful-land bump

@hboon
Copy link
Member

hboon commented Aug 5, 2018

@James-Sangalli I might be parsing the numbers incorrectly. Would be good if you can help verify. But it does seem to be missing some of the cattribute values.

@hboon
Copy link
Member

hboon commented Aug 6, 2018

@James-Sangalli it seems the XML's bitmask assumes a reversed order of the gene groups:

For example, body which is this in the XML:

000000000000000111110000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

should be:

000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001111 body

Similarly:

000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000011111000000000000000000000000000000000000000000000000000000000000 eye-type/shape
000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001111100000000000000000000 pattern-type

But unless there's another source, we'll have to figure out the missing cattribute values ourselves.

Copy link

@SmartLayer SmartLayer left a comment

Choose a reason for hiding this comment

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

I can't approve this unless the repo server is updated to handle directory hierarchy since you created a subdirectory in this branch.

@SmartLayer
Copy link

SmartLayer commented Aug 20, 2018

@James-Sangalli 1) suggest to split this into 2 pr in the future since "sort into folders" is a big change. 2) Approve this first so I can approve yours: AlphaWallet/alpha-wallet-android#299

@SmartLayer SmartLayer mentioned this pull request Aug 20, 2018
@SmartLayer
Copy link

@James-Sangalli says this one should wait - don't approve it because he is still working on it

@SmartLayer SmartLayer changed the title sort into folders and add cryptokitties xml don't approve me, work in progress for cryptokitties xml Aug 20, 2018
@bitcoinwarrior1
Copy link
Contributor Author

Replaced by services like OpenSea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants