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

[WIP] Implementation of plugins for ERC165, ERC721 and cryptokitties #116

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

leonprou
Copy link

@leonprou leonprou commented Nov 1, 2018

PR for issue #109

@CLAassistant
Copy link

CLAassistant commented Nov 1, 2018

CLA assistant check
All committers have signed the CLA.

@leonprou leonprou changed the title [WIP] Erc165 erc721 cryptokitties [WIP] Implementation of plugins for ERC165, ERC721 and cryptokitties Nov 1, 2018
Copy link
Contributor

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Some initial feedback. I think we're off to a very good start!

src/__tests__/erc165/erc165.test.ts Outdated Show resolved Hide resolved
const query = `
{
account(address:"0x06012c8cf97BEaD5deAe237070F9587f8E7A266d") {
supportsInterface(interfaceID: "0x01ffc9a7")
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer strict camel casing.

Suggested change
supportsInterface(interfaceID: "0x01ffc9a7")
supportsInterface(interfaceId: "0x01ffc9a7")

Copy link
Author

Choose a reason for hiding this comment

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

I prefer too, but that's the language of the original EIP - https://github.com/ethereum/EIPs/blob/master/EIPS/eip-165.md. Of course we can do interfaceId, your call.

src/__tests__/erc165/erc165.test.ts Outdated Show resolved Hide resolved
const query = `
{
account(address:"0x06012c8cf97BEaD5deAe237070F9587f8E7A266d") {
supportsInterface(interfaceID: "0x9a20483d")
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we add a a field an an enum type for known interface IDs?

Copy link
Author

Choose a reason for hiding this comment

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

as a new grapql method (like supportsInterfaceByEnum) or just at the backend?

test('erc721: nftToken balanceOf query #1', async () => {
const query = `
{
nftToken(address:"0x06012c8cf97BEaD5deAe237070F9587f8E7A266d") {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to pollute the global namespace. How about adding a if this plugin extends the Account type with an nftToken field? We'd need to add the token field in the core plugin schema. A query would look like this:

{
  account(address: "0x06012c8cf97BEaD5deAe237070F9587f8E7A266d") {
    contract {
      nftContract {
      }
    }
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

Interesting, yeah this looks cleaner. But I'm not sure why we need the contract fields as a intermediary (for logical distinction?). For now I did this syntax:

  {
  account(address:"0x6EbeAf8e8E946F0716E6533A6f2cefc83f60e8Ab") {
    nftToken {
      balanceOf(owner: "0xD418c5d0c4a3D87a6c555B7aA41f13EF87485Ec6")
      }
    }    
  }

If I would add the contract to Account, what should it return if plugins are used? An empty object?

src/core/services/decoder/impl/simple.ts Outdated Show resolved Hide resolved
src/erc165/model/index.ts Outdated Show resolved Hide resolved
src/erc165/decoders/index.ts Outdated Show resolved Hide resolved
src/erc165/index.ts Outdated Show resolved Hide resolved
entity: Entity
standard: String
operation: String
from: TokenHolder
Copy link
Contributor

Choose a reason for hiding this comment

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

This is referencing the ERC20 TokenHolder, right? I think we need to rename TokenHolder to ERC20TokenHolder, and we might need to add a field owns(tokenId: Long) that calls ownerOf on the token ID and returns true if we own it.

Copy link
Author

Choose a reason for hiding this comment

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

Yes this makes sense. Just didn't want to touch the old code

Copy link
Author

@leonprou leonprou Dec 28, 2018

Choose a reason for hiding this comment

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

about the method owns, what's the proposed graphql sytnax? If it's:

account(address:"0x06012c8cf97BEaD5deAe237070F9587f8E7A266d") {
owns(tokenId: 1)
}

The account address is the address of the token owner or of the token contract?

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

Successfully merging this pull request may close these issues.

None yet

3 participants