search

hyperledger / fabric-samples

Samples for Hyperledger Fabric
https://wiki.hyperledger.org/display/fabric
Apache License 2.0
2.77k stars 3.36k forks source link

token-erc-721 and token-utxo samples are not working per Readme #762

Closed denyeart closed 2 years ago

denyeart commented 2 years ago

https://github.com/hyperledger/fabric-samples/tree/main/token-erc-721 sample is not working per the readme.

The Readme has the user invoke Initialize with args: "Args":["some name", "some symbol", "2"]

However the chaincode function only takes two parameters (no decimal parameter).

I then tried with "Args":["some name", "some symbol"]. This time the chaincode function is invoked successfully, but it returns an error "Error: contract options are already set, client is not authorized to change them", even though it hasn't been initialized yet. This works for the Go chaincode but not the Javascript chaincode, so I suspect the check at https://github.com/hyperledger/fabric-samples/blob/main/token-erc-721/chaincode-javascript/lib/tokenERC721.js#L315-L317 is not correct.

It looks like the same issue will apply to the token-utxo readme (readme has three parameters but chaincode only has two parameters). I assume this was just a copy/paste issue in the Readme.

rajat-dlt commented 2 years ago

@denyeart, I worked on ERC721 and had to resolve this in my dev environment. I've created a PR regarding the same. Let me know if we can proceed with this or if further changes are needed in the PR.

denyeart commented 2 years ago

@rajat-dlt Nice! Could you apply the same existence check fix for https://github.com/hyperledger/fabric-samples/blob/main/token-erc-20/chaincode-javascript/lib/tokenERC20.js ?

rajat-dlt commented 2 years ago

@denyeart, the same check is added for ERC20. Thanks for pointing this out.

denyeart commented 2 years ago

Merged the existence check fixes, thanks @rajat-dlt !

@fraVlaca I think you just need to align the number of parameters across the readmes and sample chaincodes now (2 versus 3).

jkneubuh commented 2 years ago

The README / docs update are good, but it would be better to CLOSE this issue once and for all by including an end-to-end test script that exercises the overall flow. Even better if it's a matrix based test that exercises the flow for multiple chaincode implementations. And even, even, even better if it tests across alternate orchestration engines / network environments.

READMEs never work for me, or worse: trigger the dreaded WORKSFORME resolved flag in Bugzilla. If everyone can run the E2E script on a local dev box, it's a huge step forward as it's largely 100% reproducible.

I recommend keeping this issue open and adding an E2E test script to Azure. Then it's "done" and "done forever."