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

raulk commented 5 years ago

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

[x] 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.
[x] 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 (#107).
[ ] Code commented, documented and linted.
[ ] README and wiki updated listing and showcasing this functionality.

gitcoinbot commented 5 years ago

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.

If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
- Want to chip in? Add your own contribution here.
- Questions? Checkout Gitcoin Help or the Gitcoin Slack
- $40,714.62 more funded OSS Work available on the Gitcoin Issue Explorer

gitcoinbot commented 5 years ago

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 commented 5 years ago

💰 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 commented 5 years ago

@raulk Hello, any update :eyes:?

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

spm32 commented 5 years ago

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

raulk commented 5 years ago

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

leonprou commented 5 years ago

@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 commented 5 years ago

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

elie222 commented 5 years ago

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

gitcoinbot commented 5 years ago

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

[x] warning (3 days)
[ ] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

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

[x] warning (3 days)
[ ] escalation to mods (6 days)

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

leonprou commented 5 years ago

@gitcoinbot sure :)

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

gitcoinbot commented 5 years ago

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

[x] warning (3 days)
[ ] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

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

[x] warning (3 days)
[ ] escalation to mods (6 days)

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

leonprou commented 5 years ago

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 commented 5 years ago

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

[x] warning (3 days)
[ ] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

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

[x] warning (3 days)
[ ] escalation to mods (6 days)

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

leonprou commented 5 years ago

@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 commented 5 years ago

I believe Kitties is the ERC721 before it was fully finalised. A lot of games implement the Kitties ERC721 standard. Another example is CryptoFighters.

On Mon, 12 Nov 2018 at 13:46, Leon Prouger notifications@github.com wrote:

@raulk https://github.com/raulk @ceresstation https://github.com/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 https://github.com/leonprou/ethql/blob/erc165_erc721_cryptokitties/src/erc721/resolvers/index.ts#L6), but I doesn't make too much sense. So I'm thinking to delete this condition at all.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ConsenSys/ethql/issues/109#issuecomment-437988387, or mute the thread https://github.com/notifications/unsubscribe-auth/AC8oX-CMlBhMluzFNNbhMx86KxZkozzRks5uucH5gaJpZM4XBHMa .

leonprou commented 5 years ago

@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 commented 5 years ago

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

[x] reminder (3 days)
[ ] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

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

[x] reminder (3 days)
[ ] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

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!

[x] reminder (3 days)
[x] escalation to mods (6 days)

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

leonprou commented 5 years ago

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

raulk commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

Hey @leonprou any updates here? :)

leonprou commented 5 years ago

@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 commented 5 years ago

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

leonprou commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@raulk LGTM 👍

leonprou commented 5 years ago

@raulk @ceresstation so waiting for the transfer :eyes:

spm32 commented 5 years ago

Sorry about that @leonprou just paid you out :)

gitcoinbot commented 5 years ago

⚡️ 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.

$59218.98 in Funded OSS Work Available at: https://gitcoin.co/explorer
Incentivize contributions to your repo: Send a Tip or Fund a PR
No Email? Get help on the Gitcoin Slack

gitcoinbot commented 5 years ago

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.

Learn more on the Gitcoin Issue Details page
Questions? Checkout Gitcoin Help or the Gitcoin Slack
$59,418.98 more funded OSS Work available on the Gitcoin Issue Explorer

leonprou commented 5 years ago

@ceresstation Great, thanks!

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

Consensys / ethql