Skip to content
This repository has been archived by the owner on Jan 21, 2022. It is now read-only.

ERC721 (non-fungible tokens), ERC165 (introspection) and CryptoKitties #109

Open
2 of 7 tasks
raulk opened this issue Sep 30, 2018 · 37 comments
Open
2 of 7 tasks

ERC721 (non-fungible tokens), ERC165 (introspection) and CryptoKitties #109

raulk opened this issue Sep 30, 2018 · 37 comments
Labels
bounty Type: Feature New feature or request

Comments

@raulk
Copy link
Contributor

raulk commented Sep 30, 2018 •
edited
Loading

Motivation

EthQL can currently decode ERC20 transactions, and can support any ABI through its decoder service. We now want to extend the breadth the transaction decoding in EthQL by adding support for ERC721 (NFT) and ERC165 (supportsInterface). The latter is a fundamental step towards supporting more contract types in the near-future.

We also want to introduce the notion of application-specific queries with CryptoKitties as an example.

Requirements

  • Create erc165 plugin that adds a supportsInterface field in account to query whether that contract supports the interface provided as an argument.

    • Additionally, this plugin should add a service to make it possible for other plugins to query if a contract supports an interface.

  • Create erc721 plugin, adding a decoder configuration for the ERC721 log events and transactions.

    • Add a top-level query nftContract(address: "0x...") that exposes the ABI's view operations and public fields, including any relevant field edges (e.g. accounts mainly).
    • ERC721* transaction/log types (in the same spirit as ERC20* transaction/log types) so they can be used from decoded fields.
    • This plugin should use the service exposed by the erc165 plugin to query if a contract is an NFT.

  • Create a cryptokitties plugin.

    • Analyse and propose which operations from the CryptoKitties contract should be exposed as EthQL queries.
    • Decide together with the EthQL team the query/field design.
    • Implement all agreed fields according to the definition of done below.

Definition of done

  • Test cases with full coverage.
  • PR accepted and merged, introducing a new top-level directory for each of the plugins generating an NPM module each (Lerna build #107).
  • Code commented, documented and linted.
  • README and wiki updated listing and showcasing this functionality.
@raulk raulk added Type: Feature New feature or request bounty labels Sep 30, 2018
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 600.0 DAI (600.0 USD @ $1.0/DAI) attached to it.

@gitcoinbot
Copy link

gitcoinbot commented Oct 14, 2018 •
edited
Loading

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 3 months, 4 weeks ago.
Please review their action plans below:

1) leonprou has been approved to start work.

Hey,
I'm a Dapp and Fullstack developer, excited to be part of developing the Ethereum ecosystem :)

Plan of work:
First I'd like to implement the ERC165 plugin and present it to the team. Then based on their review I'll make the RC721 plugin and the cryptokitties plugin after it.
Of course I'll make my work with the same structure like the current ERC20 plugin.

Learn more on the Gitcoin Issue Details page.

@gitcoinbot
Copy link

💰 A crowdfund contribution worth 55.00000 DAI (55.0 USD @ $1.0/DAI) has been attached to this funded issue from @.💰

Want to chip in also? Add your own contribution here.

@leonprou
Copy link

@raulk Hello, any update 👀?

I've got over the codebase and graphql docs, so I think I can handle this.

@spm32
Copy link

spm32 commented Oct 17, 2018

Hey @leonprou you're good to go! Sorry for the delay on this.

@raulk
Copy link
Contributor Author

raulk commented Oct 18, 2018

@leonprou 🎉 let me know if you have any questions!

@leonprou
Copy link

leonprou commented Oct 19, 2018 •
edited
Loading

@ceresstation @raulk thanks guys.

Started working, you can view my progress here. For now I've implemented the ERC165 plugin (though I've done this using a simple resolver).

Also I added a Bytes4 scalar type to the core module, I reckon if it's a part of the Ethereum bytecode it should be there.

Update
Implementing ERC721 I'm noticing strange relationship between it and ERC20. Not sure if one should be depend on the other or some token plugin can be extracted from ERC20.

@elie222
Copy link

elie222 commented Oct 22, 2018

Erc20 and 721 are very similar. But for example, you don't transfer 100 tokens in Erc721. You transfer kitty with id 123

1 similar comment
@elie222
Copy link

elie222 commented Oct 22, 2018

Erc20 and 721 are very similar. But for example, you don't transfer 100 tokens in Erc721. You transfer kitty with id 123

@gitcoinbot
Copy link

@leonprou Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

@leonprou Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@leonprou
Copy link

@gitcoinbot sure :)

@elie222 Yes I understand the ERC's. My question was more about code structuring.

@gitcoinbot
Copy link

@leonprou Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

@leonprou Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@leonprou leonprou mentioned this issue Nov 1, 2018
Open
@leonprou
Copy link

leonprou commented Nov 1, 2018

Yes, I'm in DevCon now so didn't have time to work on this issue. Will work on this at the weekend. That's my WIP PR

Yeah I have an obstacle that I would like to discuss. I started to test the ERC721 plugin, but I'm struggling to find transactions signatures for tests. For example I want to test the transfer method, so I looked for one on etherscan (mainnet), but I've found out that most of the time ERC tokens are received via an action method. It's hard to find it manually, maybe I should write some automation script with web3.

@gitcoinbot
Copy link

@leonprou Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

@leonprou Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@leonprou
Copy link

@raulk @ceresstation Hey, I'm finishing with the ERC721 plugin.

Fun part: technically Cryptokitties isn't ERC721 token. It's missing some ERC721 methods and got a different signature for `supportsInterface function. So now I check the two signatures (here), but I doesn't make too much sense. So I'm thinking to delete this condition at all.

@elie222
Copy link

elie222 commented Nov 12, 2018 via email

@leonprou
Copy link

@elie222 yeah, this what I though about too.

Anyway, I've done the implementation of ERC721. Actually, I don't think it's so convenient to use right now, because:

  • NFT's are using different flavours of ERC721. Slightly different variable names is requires a rewrite of all the plugin. onwer vs _owner, tokenId vs assetId and etc.
  • NFT's prefer to use their own functions and not the one of the standard. There's a lot of claim, bid, and transfer (which is actually not a part of ERC721). They do use the correct events most of the time.
  • That's rather technical, but the ERC20 plugin is 'shadows' my ERC721. Maybe a generic ERC721Transaction type can solve this issue.

So, as the current ERC721 implemented, I don't think Cryptokitties plugin can use it at all.

@gitcoinbot
Copy link

@leonprou Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

@leonprou Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

@leonprou due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@leonprou
Copy link

leonprou commented Dec 2, 2018 •
edited
Loading

@raulk @elie222 Actually I'm waiting from the bounty founders input here.

@raulk
Copy link
Contributor Author

raulk commented Dec 2, 2018

Sorry for the delay here, @leonprou. I think we're off to a very good start. I provided some initial feedback on the PR. Regarding the ERC20 vs. ERC721 duality, for now let's decouple them from one another, even if that implies minimal code duplication. It's too early to introduce a wider "token" abstraction.

Regarding the function/property names differences in ERC721, it sucks that things are not standard. And this surprises me a lot. Do we have concrete examples? I'm thinking we can use some kind of fallback, e.g. first try X, then try Y, then try Z before giving up.

Do you have a headstart with the Cryptokitties plugin? When I evaluated the contract, I realised it differs much from ERC721, so it belongs in a separate plugin.

If we find that the method interfaces are the same except for naming, we could consider exposing a service from the ERC721 plugin that takes a property/method name mapping.

@leonprou
Copy link

leonprou commented Dec 4, 2018

@raulk Thanks for the comments, already started to do the fixing and pondering about them 😊.

About the differences with Cryptokitties and other NFT's. Yes, because of the big delta I didn't start with it yet. For example Cryptokitties doesn't include underscore for the variable names in the events (not _tokenId but tokenId for example). I could try both variables in the my plugin, but the problem actually roots to the decoded plugin. It doesn't decode the params because of the ABI differences.

it's hard for me to estimate how much such deltas are common. Looks like though the new NFT's are more strict to follow the ERC721.

Also, looking into NFT's I see that the most important are the events, cause for most of the time the NFT's are using their own functionality.

@spm32
Copy link

spm32 commented Dec 28, 2018

Hey @leonprou any updates here? :)

@leonprou
Copy link

@ceresstation @raulk Hey, sorry for being slow on this.
I answered to most of the comments in the PR. Also, waiting for feedback regarding my last comment about differences of Cryptokitties and ERC721.

Maybe we should chat about the missing parts to make it faster

@rmshea
Copy link

rmshea commented Jan 30, 2019

hey @raulk have you had a chance to take a look at the comments in the PR?

@leonprou
Copy link

leonprou commented Feb 3, 2019

@ceresstation @ryan-shea Hey guys, I don't think I want to continue to work on this bounty. The rare context switches on this are taking too much resources..

I believe that I made something close to the bounty's goals.. So it would be nice If I could receive some fractional compensation for my work.

@raulk
Copy link
Contributor Author

raulk commented Feb 3, 2019

@leonprou That's totally fair. To be honest I'm not entirely sure about how to move forward with this, as the standards seem somewhat non-normative. I think it would've been great to try and develop a Cryptokitties plugin by reverse engineering the ABI anyway, but I'm also not sure what the value of such an undertaking is.

@ceresstation Can we release half the bounty to @leonprou for his work?

@leonprou
Copy link

leonprou commented Feb 5, 2019

@raulk LGTM 👍

@leonprou
Copy link

@raulk @ceresstation so waiting for the transfer 👀

@spm32
Copy link

spm32 commented Feb 20, 2019

Sorry about that @leonprou just paid you out :)

@gitcoinbot
Copy link

⚡️ A tip worth 300.00000 DAI (300.0 USD @ $1.0/DAI) has been granted to @leonprou for this issue from @ceresstation. ⚡️

Nice work @leonprou! Your tip has automatically been deposited in the ETH address we have on file.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This Bounty has been completed.

Additional Tips for this Bounty:

  • ceresstation tipped 300.0000 DAI worth 300.0 USD to leonprou.

@leonprou
Copy link

@ceresstation Great, thanks!

Seems some part of the tip is still stuck on the network. The gas fee is 3 wei 😅
https://etherscan.io/tx/0x507dcf8439b2aac605ea528877c75c387b4068be461abc6dd2fcf46fbb50b9bd

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bounty Type: Feature New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@raulk @elie222 @leonprou @rmshea @spm32 @gitcoinbot