Add ERC721 non-fungible token sample for javascript Chaincode #406

yukiknd · 2021-01-20T01:46:15Z

This PR adds a new non-fungible token sample using ERC721
functionalities. It includes javascript Chaincode and the README
explaining how to mint and transfer a non-fungible token in the
Fabric's test-network.

Signed-off-by: Yuki Kondo yuki.kondo.ob@hitachi.com

sstone1 · 2021-01-20T14:36:24Z

token-erc-721/chaincode-javascript/lib/tokenERC721.js

+     * @param {String} tokenURI URI containing metadata of the minted non-fungible token
+     * @returns {Boolean} Return whether the burn was successful or not
+    */
+    async SetTokenURI(ctx, tokenId, tokenURI) {


Should there be an access control check here to ensure that the caller owns the token and is permitted to set the token URI?

Thanks! I will add the access control to check the ownership like Redeem

SetTokenURI was removed because we use MintWithTokenURI to set a token URI in a single function.

sstone1 · 2021-01-20T14:37:31Z

token-erc-721/chaincode-javascript/lib/tokenERC721.js

+     * @param {String} tokenId Unique ID of the non-fungible token to be minted
+     * @returns {Object} Return the non-fungible token object
+    */
+    async Mint(ctx, tokenId) {


Have you considered allowing a user to mint a new token and set the token URI in a single call, rather than two calls (mint + set token URI)?

Yes, I have thought about it before. Mint() is the out of scope of ERC721 specification and there is no restriction. As a reference, I looked at OpenZeppelin's implementation. It has a separated function to put the token URI.
https://docs.openzeppelin.com/contracts/2.x/api/token/erc721#ERC721Metadata-_setTokenURI-uint256-string-

The OpenZeppelin implementation also seems to have a mintWithTokenURI: https://docs.openzeppelin.com/contracts/2.x/api/token/erc721#ERC721MetadataMintable-mintWithTokenURI-address-uint256-string-

Thanks. I missed that part. It seems we have two options: 1) Use separated functions Mint + SetTokenURI, 2) Use a single function MintWithTokenURI. Which is better for this sample?

I would prefer a single function MintWithTokenURI.

Done. The updated code uses a single function MintWithTokenURI to mint a new token.

token-erc-721/chaincode-javascript/lib/tokenERC721.js

sstone1

Looks great - will give it a try. Couple of initial comments based on a quick read through the code.

token-erc-721/chaincode-javascript/lib/tokenERC721.js

sstone1

I gave the contract a go yesterday and it works well; I was able to mint, transfer and burn tokens fine.

Apart from the minor niggles above, I do have a concern about the use of JSON arrays in state - one for the global list of tokens, and one each per user for their list of owned tokens. The more tokens you store in this contract, the larger these JSON arrays will become, and the slower it will become. With very large numbers of tokens, trying to handle and parse these JSON arrays may also result in out of memory crashes.

yukiknd

Thank you for trying the code and comments. I will fix minor issues first.

The concern about the global JSON array is very important. It's a great suggestion.
I came up with an idea to use getStateByPartialCompositeKey() to get a list of non-fungible tokens without a single global state managing the list. For example, we can change TotalSupply() like the below. Do you have any good solutions?

async TotalSupply(ctx) {
    // This will execute a key range query on all non-fungible tokens
    const iterator = ctx.stub.getStateByPartialCompositeKey(nftPrefix, []);
    let result = await iterator.next();
    let totalSupply = 0;
    while (!result.done) {
        totalSupply++;
        result = await iterator.next();
    }
    return totalSupply;
}

token-erc-721/chaincode-javascript/lib/tokenERC721.js

yukiknd · 2021-01-21T09:43:19Z

token-erc-721/chaincode-javascript/lib/tokenERC721.js

+     * @param {String} tokenURI URI containing metadata of the minted non-fungible token
+     * @returns {Boolean} Return whether the burn was successful or not
+    */
+    async SetTokenURI(ctx, tokenId, tokenURI) {


Thanks! I will add the access control to check the ownership like Redeem

yukiknd · 2021-01-21T09:46:53Z

token-erc-721/chaincode-javascript/lib/tokenERC721.js

+     * @param {String} tokenId Unique ID of the non-fungible token to be minted
+     * @returns {Object} Return the non-fungible token object
+    */
+    async Mint(ctx, tokenId) {


Yes, I have thought about it before. Mint() is the out of scope of ERC721 specification and there is no restriction. As a reference, I looked at OpenZeppelin's implementation. It has a separated function to put the token URI.
https://docs.openzeppelin.com/contracts/2.x/api/token/erc721#ERC721Metadata-_setTokenURI-uint256-string-

token-erc-721/chaincode-javascript/lib/tokenERC721.js

sstone1 · 2021-01-21T14:25:07Z

@yukiknd that would work, but it must be expensive to read through all the matching entries to get the count of matching keys. I guess you'd also have to read through all the entries when you were trying to find the Nth key for indexed access?

@denyeart have you got any ideas on how to best implement this?

yukiknd · 2021-01-22T05:01:00Z

@sstone1 That is just as you pointed out. The idea above avoids parsing a big JSON array but it has to read through many entries to get the total supply and the Nth key.

yukiknd · 2021-01-26T05:54:22Z

@sstone1 Hello. I updated the code to fix the minor issues. Could you take a look again?

denyeart · 2021-01-27T07:29:40Z

@yukiknd @sstone1 I agree with the concerns about large JSON arrays stored in a key/value, and I agree individual records using a composite key is better, such that you can query by partial composite key to get all the records.

Going one step further, can we get by without the global index altogether? TotalSupply() could use a counter key instead of actually counting the records at query time. Just increment the counter every time you mint a NFT, and decrement when you burn a NFT. And since TokenByIndex() is optional in the spec, we could simply remove it. I believe the intent of that function is so that a client can iterate through all the NFTs. But unlike Ethereum, Fabric has query support in the contract, therefore we could have a function that queries and returns all the NFTs using GetStateByPartialCompositeKey on the nftPrefix. And if we are worried about a large set getting returned to client, we can also have a paged function that uses GetStateByPartialCompositeKeyWithPagination. Hopefully I am not missing something important.

And then there is the balance JSON array. That one could also get large if a user owns many NFTs. It could be replaced with individual records using a composite key like owner.tokenId. This would enable a query function that does a partial composite key query on owner.*. Again, we can use a paged query in case owner has many NFTs. And BalanceOf() and ClientAccountBalance() could use counter keys instead of actually counting them at query time.

yukiknd · 2021-01-27T10:42:17Z

@denyeart Thank you so much! It sounds great to use Fabric's query functions to meet the original intention. The ERC-721 specification explains the enumeration extension is OPTIONAL and it allows a contract to publish its full list of NFTs and make them discoverable. Since Fabric can get a list by using GetStateByPartialCompositeKey, I agree with removing TokenByIndex() and tokenOfOwnerByIndex().

For total supply, I have a little concern about using a counter key because it may cause frequent MVCC conflicts. The counter key will be updated by all transactions issuing and redeeming tokens. The counter key for each owner's balance is also update by Mint(), Transfer() and Burn(). To avoid MVCC conflicts, counting the numbers of composite keys at query time may be a work around. Do you have any best practices to avoid MVCC conflicts?

denyeart · 2021-01-27T13:55:01Z

@yukiknd Ah, you are right, counter key is bad from MVCC conflict perspective. So I would leave it as a key per record, with the key being a composite key, and then to get the count do a GetStateByPartialComposite key and iterate through all the records to get a count. This should be possible for both the total supply count and the owner balance count.

yukiknd · 2021-01-28T01:04:10Z

@denyeart Thank you for summarizing the design. I will update the code.

lindluni · 2021-01-29T21:51:04Z

/azp run

azure-pipelines · 2021-01-29T21:51:15Z

Azure Pipelines successfully started running 1 pipeline(s).

lindluni · 2021-01-29T22:33:18Z

Currently an outage at Google Cloud where our Artifactory instance is hosted, which is causing the failure here as the Maven repo is timing out trying to fetch the artifact, will retry later when the outage is resolved:

https://status.jfrog.io/

lehors · 2021-02-01T09:20:36Z

/azp run

azure-pipelines · 2021-02-01T09:20:50Z

Azure Pipelines successfully started running 1 pipeline(s).

yukiknd · 2021-02-03T02:56:07Z

@denyeart Could you take a look at the code again? I updated the code to count the total supply and the balance of each owner at the query time with getStateByPartialCompositeKey().

denyeart

The approach looks good and efficient now. Just a few minor comments.

denyeart · 2021-02-03T05:01:27Z

token-erc-721/chaincode-javascript/lib/tokenERC721.js

+        await ctx.stub.putState(nftKey, Buffer.from(JSON.stringify(nft)));
+
+        // Save a composite key to count the balance of an owner
+        // Only the key name is stored to count the balance by getStateByPartialCompositeKey()


This comment is not clear to me.
I would provide an example, e.g. composite key would be balancePrefix.owner.tokenId, which enables partial composite key query to find and count all records matching balance.owner.*

token-erc-721/chaincode-javascript/lib/tokenERC721.js

denyeart · 2021-02-03T05:07:55Z

token-erc-721/chaincode-javascript/lib/tokenERC721.js

+        // Assign a non-fungible token to the new owner.
+        nft.owner = to;
+        const nftKey = ctx.stub.createCompositeKey(nftPrefix, [tokenId]);
+        await ctx.stub.putState(nftKey, Buffer.from(JSON.stringify(nft)));


I would do this code block prior to the balance key code blocks. At least for me, it is more logical to first write the main token record, then handle the balance records.

In the comment, instead of 'Assign' , I'd recommend the word 'Overwrite' so that people know we are overwriting the existing token record.

denyeart · 2021-02-03T05:15:25Z

token-erc-721/chaincode-javascript/lib/tokenERC721.js

+     * @returns {Number} The number of non-fungible tokens owned by the owner, possibly zero
+     */
+    async BalanceOf(ctx, owner) {
+        // Execute a partial composite key query on all non-fungible tokens owned by the given owner


Since this is early in the file, in the comment, explain the concept of the composite key and query, e.g.:
There is a key record for every nft in the format of
balancePrefix.owner.tokenId.
BalanceOf queries for and counts all records matching balancePrefix.owner.*

yukiknd · 2021-02-03T09:19:21Z

@denyeart Thank you so much! The advices on comments are very helpful. I fixed comment messages as you pointed out. I also updated the comment on TotalSupply to be consistent with BalanceOf.

denyeart · 2021-02-08T17:22:44Z

It looks good to me now, @sstone1 is also going to take another look.
The CI issues should be fixed now if the PR is rebased, let's see if mergifyio can deal with it...

denyeart · 2021-02-08T17:22:57Z

@Mergifyio rebase

denyeart · 2021-02-08T17:25:47Z

Nevermind, we don't have Mergify configured on fabric-samples. I'm not concerned, but if you want to make the CI failures go away you can rebase on master and force push your branch again.

lindluni · 2021-02-08T17:44:37Z

@Mergifyio rebase

This PR adds a new non-fungible token sample using ERC721 functionalities. It includes javascript Chaincode and the README explaining how to mint and transfer a non-fungible token in the Fabric's test-network. Signed-off-by: Yuki Kondo <yuki.kondo.ob@hitachi.com>

mergify · 2021-02-08T17:45:22Z

Command rebase: success

Branch has been successfully rebased

lindluni · 2021-02-08T17:47:15Z

Nevermind, we don't have Mergify configured on fabric-samples. I'm not concerned, but if you want to make the CI failures go away you can rebase on master and force push your branch again.

I had Ry enable mergify across the fabric repos and I rebase this PR, so no need to rebase it manually now

yukiknd · 2021-02-09T02:08:58Z

Thanks for your help solving the CI issues.

sstone1

LGTM

yukiknd requested a review from a team as a code owner January 20, 2021 01:46

sstone1 reviewed Jan 20, 2021

View reviewed changes

token-erc-721/chaincode-javascript/lib/tokenERC721.js Show resolved Hide resolved

sstone1 reviewed Jan 20, 2021

View reviewed changes

token-erc-721/chaincode-javascript/lib/tokenERC721.js Show resolved Hide resolved

sstone1 reviewed Jan 21, 2021

View reviewed changes

yukiknd commented Jan 21, 2021

View reviewed changes

yukiknd force-pushed the token-erc721 branch from 1ec52d6 to 8648a99 Compare January 26, 2021 05:27

yukiknd force-pushed the token-erc721 branch from 8648a99 to ca51bbc Compare February 3, 2021 02:43

denyeart reviewed Feb 3, 2021

View reviewed changes

yukiknd force-pushed the token-erc721 branch from ca51bbc to 0a4974f Compare February 3, 2021 09:13

sstone1 force-pushed the token-erc721 branch from 0a4974f to 51071cd Compare February 8, 2021 17:45

sstone1 approved these changes Feb 24, 2021

View reviewed changes

sstone1 merged commit b88ec9f into hyperledger:master Feb 24, 2021

Add ERC721 non-fungible token sample for javascript Chaincode #406

Add ERC721 non-fungible token sample for javascript Chaincode #406

Conversation

yukiknd commented Jan 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstone1 left a comment

Choose a reason for hiding this comment

sstone1 left a comment

Choose a reason for hiding this comment

yukiknd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstone1 commented Jan 21, 2021

yukiknd commented Jan 22, 2021

yukiknd commented Jan 26, 2021

denyeart commented Jan 27, 2021

yukiknd commented Jan 27, 2021

denyeart commented Jan 27, 2021

yukiknd commented Jan 28, 2021

lindluni commented Jan 29, 2021

azure-pipelines bot commented Jan 29, 2021

lindluni commented Jan 29, 2021 • edited Loading

lehors commented Feb 1, 2021

azure-pipelines bot commented Feb 1, 2021

yukiknd commented Feb 3, 2021

denyeart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yukiknd commented Feb 3, 2021

denyeart commented Feb 8, 2021

denyeart commented Feb 8, 2021

denyeart commented Feb 8, 2021

lindluni commented Feb 8, 2021

mergify bot commented Feb 8, 2021

lindluni commented Feb 8, 2021

yukiknd commented Feb 9, 2021

sstone1 left a comment

Choose a reason for hiding this comment

lindluni commented Jan 29, 2021 •

edited

Loading