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
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0b5da35
Add ERC165 support
leonprou Oct 19, 2018
517300c
adding tests for ERC165, starting implementing 721
leonprou Oct 21, 2018
d1dacdb
adding more functions
leonprou Oct 27, 2018
c0ec9e1
add erc165 as a service
leonprou Nov 3, 2018
67b07b7
adding all erc721 view methods
leonprou Nov 9, 2018
4e1b86c
adding function
leonprou Nov 16, 2018
9b7df7d
Update src/__tests__/erc165/erc165.test.ts
raulk Dec 3, 2018
bafc00f
delete blank line
leonprou Dec 3, 2018
f17707e
Merge branch 'erc165_erc721_cryptokitties' of https://github.com/leon…
leonprou Dec 3, 2018
73abf18
small changes
leonprou Dec 3, 2018
d644ebc
deleting unnecessary code
leonprou Dec 3, 2018
da4d6bc
move nftToken to Account
leonprou Dec 15, 2018
1feaef0
add ERC20 prefix to sgraphql types
leonprou Dec 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/__tests__/erc165/erc165.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import core from '../../core';
import erc165 from '../../erc165';
import { testGraphql } from '../utils';

const { execQuery } = testGraphql({ optsOverride: { plugins: [core, erc165] } });

test('erc165: Cryptokities supports ERC165 interface', async () => {
leonprou marked this conversation as resolved.
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.

}
}
`;

const result = await execQuery(query);
expect(result.errors).toBeUndefined();
expect(result.data).not.toBeUndefined();
expect(result.data.account.supportsInterface).toEqual(true);
});

test('erc165: Cryptokities supports ERC721 interface', async () => {
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?

}
}
`;

const result = await execQuery(query);
expect(result.errors).toBeUndefined();
expect(result.data).not.toBeUndefined();

expect(result.data.account.supportsInterface).toEqual(true);
});

test('erc165: OmiseGO does not supports ERC165', async () => {
leonprou marked this conversation as resolved.
Show resolved Hide resolved
const query = `
{
account(address:"0xd26114cd6EE289AccF82350c8d8487fedB8A0C07") {
address,
supportsInterface(interfaceID: "0x01ffc9a7")
}
}
`;

const result = await execQuery(query);
expect(result.errors).toBeUndefined();
expect(result.data).not.toBeUndefined();

expect(result.data.account.supportsInterface).toEqual(false);
});
267 changes: 267 additions & 0 deletions src/__tests__/erc721/erc721.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,267 @@
import core from '../../core';
import erc165 from '../../erc165';
import erc20 from '../../erc20';
import erc721 from '../../erc721';
import { testGraphql } from '../utils';

const { execQuery } = testGraphql({ optsOverride: { plugins: [core, erc20, erc165, erc721] } });

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?

balanceOf(owner: "0xD418c5d0c4a3D87a6c555B7aA41f13EF87485Ec6")
}
}
`;
const result = await execQuery(query);
expect(result.errors).toBeUndefined();
expect(result.data).not.toBeUndefined();

expect(result.data).toEqual({
nftToken: {
balanceOf: 0,
},
});
});

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

Choose a reason for hiding this comment

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

What if the balance of these accounts changes? The tests will fail.

Could you please add comments indicating what these addresses correspond to?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's a problem. But I guess the same goes the all tests that didn't query past transactions.
I changed the tests to use my accounts, with some GodsUnchained tokens I bought for tests.
I wanted to test more functionality with them but it turned out these tokens cannot be moved.

}
}
`;
const result = await execQuery(query);
expect(result.errors).toBeUndefined();
expect(result.data).not.toBeUndefined();

expect(result.data).toEqual({
nftToken: {
balanceOf: 72,
},
});
});

test('erc721: nftToken ownerOf query', async () => {
const query = `
{
nftToken(address:"0x06012c8cf97BEaD5deAe237070F9587f8E7A266d") {
ownerOf(tokenId: 384978)
}
}
`;
const result = await execQuery(query);
expect(result.errors).toBeUndefined();
expect(result.data).not.toBeUndefined();

expect(result.data).toEqual({
nftToken: {
ownerOf: '0x595a6aA36Ab9fFB4b5940D4E189d6F2AB3958aeb',
},
});
});

test('erc721: nftToken getApproved query', async () => {
const query = `
{
nftToken(address:"0x6EbeAf8e8E946F0716E6533A6f2cefc83f60e8Ab") {
getApproved(tokenId: 33525)
Copy link
Contributor

Choose a reason for hiding this comment

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

We shy away from mapping 1:1 functions to fields. We prefer to represent the data model. In this case, it could look like this:

{
  nftContract {
    token(id: 12345) {
      approved    # this needs to return an array of Address
      owner         # this needs to return an Address
      ...
    }
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

Hey, well I just defined defined tried to to resemble the most ERC721. what you suggest is good for the function ownerOf and getApproved, but the functions like balanceOf and isApprovedForAll doesn't receive the tokenId

}
}
`;
const result = await execQuery(query);
expect(result.errors).toBeUndefined();
expect(result.data).not.toBeUndefined();

expect(result.data).toEqual({
nftToken: {
getApproved: '0x0000000000000000000000000000000000000000',
},
});
});

test('erc721: nftToken isApprovedForAll query', async () => {
const query = `
{
nftToken(address:"0x6EbeAf8e8E946F0716E6533A6f2cefc83f60e8Ab") {
isApprovedForAll(owner: "0xb85e9bdfCA73a536BE641bB5eacBA0772eA3E61E", operator: "0xD418c5d0c4a3D87a6c555B7aA41f13EF87485Ec6")
}
}
`;
const result = await execQuery(query);
expect(result.errors).toBeUndefined();
expect(result.data).not.toBeUndefined();

expect(result.data).toEqual({
nftToken: {
isApprovedForAll: false,
},
});
});

//44

test('erc721: not decodable', async () => {
const query = `
{
block(number: 5000000) {
hash
transactionAt(index: 66) {
value
decoded {
standard
operation
... on ERC721TransferFrom {
from {
account {
address
}
}
to {
account {
address
}
}
tokenId
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, nice test.

}
}
}
}
}
`;

const result = await execQuery(query);
expect(result.errors).toBeUndefined();
expect(result.data).not.toBeUndefined();

const tx = result.data.block.transactionAt;
expect(tx.value).toBeGreaterThan(0);
expect(tx.decoded).toEqual(null);
});
//
// test('erc721: decode transfer log', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the comment here?

// const query = `
// {
// block(number: 5000000) {
// hash
// transactionAt(index: 64) {
// logs {
// decoded {
// entity
// standard
// event
// ... on ERC721TransferEvent {
// from {
// account {
// address
// }
// }
// to {
// account {
// address
// }
// }
// tokenId
// }
// }
// }
// }
// }
// }`;
//
// const result = await execQuery(query);
// expect(result.errors).toBeUndefined();
// expect(result.data).not.toBeUndefined();
//
// expect(result.data).toEqual({
// block: {
// hash: '0x7d5a4369273c723454ac137f48a4f142b097aa2779464e6505f1b1c5e37b5382',
// transactionAt: {
// logs: [
// {
// decoded: {
// entity: 'token',
// standard: 'ERC721',
// event: 'Transfer',
// from: {
// account: {
// address: '0x89eacd3f14e387faa9f3d1f3f917ebdf8221d430',
// },
// },
// to: {
// account: {
// address: '0xb1690c08e213a35ed9bab7b318de14420fb57d8c',
// },
// },
// tokenId: 489475,
// },
// },
// {
// decoded: null,
// },
// ],
// },
// },
// });
// });

//
// test('erc721: transfer transaction', async () => {
// const query = `
// {
// block(number: 5000000) {
// hash
// transactionAt(index: 64) {
// value
// hash
// decoded {
// standard
// operation
// ... on ERC721TransferFrom {
// from {
// account {
// address
// }
// }
// to {
// account {
// address
// }
// }
// tokenId
// }
// }
// }
// }
// }
// `;
//
// const result = await execQuery(query);
// console.log(result.data);
//
// expect(result.errors).toBeUndefined();
// expect(result.data).not.toBeUndefined();
//
// const decoded = {
// standard: 'ERC721',
// operation: 'transfer',
// from: {
// account: {
// address: '0x89eAcD3F14e387faA9F3D1F3f917eBdf8221D430',
// },
// },
// to: {
// account: {
// address: '0xb1690C08E213a35Ed9bAb7B318DE14420FB57d8C',
// },
// },
// tokenId: '489475',
// };
//
// const tx = result.data.block.transactionAt;
// console.log(tx);
// expect(tx.value).toBe(0);
// expect(tx.decoded).toEqual(decoded);
// });
10 changes: 6 additions & 4 deletions src/abi/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ We classify supported ABIs in two types:

## Supported ABIs

| Standard | Type | Entity | Specification | Comments |
| -------- | --------- | ------ | ------------- | --------------------------------------------------------------- |
| ERC20 | Entity |  Token | [link][1] | |
| ERC223 | Extension |  Token | [link][2] | Private ERC20 implementation change. No action needed in ethql. |
| Standard | Type | Entity | Specification | Comments |
| -------- | --------- | --------- | ------------- | --------------------------------------------------------------- |
| ERC20 | Entity |  Token | [link][1] | |
| ERC223 | Extension |  Token | [link][2] | Private ERC20 implementation change. No action needed in ethql. |
| ERC165 | Entity | Interface | [link][3] | |
leonprou marked this conversation as resolved.
Show resolved Hide resolved

## ethql naming scheme

Expand All @@ -28,3 +29,4 @@ logs pertaining to several standards that relate to the same entity.

[1]: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md
[2]: https://github.com/ethereum/EIPs/issues/223
[3]: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-165.md
21 changes: 21 additions & 0 deletions src/abi/erc165.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
[
Copy link
Contributor

Choose a reason for hiding this comment

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

Watch the indentation here! Check out erc20.json.

Copy link
Author

Choose a reason for hiding this comment

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

what plugin did you use to indentation?

{
"constant": true,
"inputs": [
{
"name": "interfaceID",
"type": "bytes4"
}
],
"name": "supportsInterface",
"outputs": [
{
"name": "",
"type": "bool"
}
],
"payable": false,
"stateMutability": "view",
"type": "function"
}
]
1 change: 1 addition & 0 deletions src/abi/erc721.json
View file
Open in desktop

Large diffs are not rendered by default.

21 changes: 21 additions & 0 deletions src/core/resolvers/scalars.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,26 @@ const Bytes32 = new GraphQLScalarType({
},
});

// tslint:disable-next-line
const Bytes4 = new GraphQLScalarType({
name: 'Bytes4',
description: 'A 4-byte value in hex format.',
serialize: String,
parseValue: input => {
return !Web3.utils.isHexStrict(input) || Web3.utils.hexToBytes(input).length !== 4 ? undefined : input;
},
parseLiteral: ast => {
if (
ast.kind !== Kind.STRING ||
!Web3.utils.isHexStrict(ast.value) ||
Web3.utils.hexToBytes(ast.value).length !== 4
) {
return undefined;
}
return String(ast.value);
},
});

//tslint:disable-next-line
const Long = new GraphQLScalarType({
name: 'Long',
Expand All @@ -69,4 +89,5 @@ export default {
BlockNumber,
Address,
Bytes32,
Bytes4,
};
Loading