Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parseParameters requires a parameter to be JSON #11

Open
kueckermann opened this issue May 23, 2018 · 3 comments
Open

parseParameters requires a parameter to be JSON #11

kueckermann opened this issue May 23, 2018 · 3 comments

Comments

@kueckermann
Copy link

TL;DR parseParameters shouldn't write a error level log for failure to JSON parse an arg. Make it debug level.


parseParameters works fine if you are sending your chaincode arguments as JSON objects or numbers. But litters error logs if you don't... which seems unnecessary.

Example:
['changeOwner', 'asset_1', 'user_2'] would result in 2 error logs

  • "failed to parse param user_1"
  • "failed to parse param user_2"

I guess the assumption is that developers will send those arguments as an array:
['changeOwner', ['asset_1', 'user_2']]
However, doing this makes destructing parsedParameters in ChaincodeBase.js:105 less useful
let payload = await method.call(this, stub, this.getTransactionHelperFor(stub), ...parsedParameters);

Example ideal use case:

// In chaincode
function changeOwner(asset, new_owner){
// Change the asset owner.
}
@janb87
Copy link
Contributor

janb87 commented May 24, 2018

I also agree on this. It should not be required to pass an argument as an object.

@daanporon Can we change https://github.com/Kunstmaan/hyperledger-fabric-node-chaincode-utils/blob/master/src/lib/ChaincodeBase.js#L58 to use logger.debug?

@daanporon
Copy link
Contributor

@janb87 @kueckermann i agree it's a bit weird that we are throwing an error there, while we are actually resolving the issue. It's ok for me to replace this with just a debug log.

@kueckermann do you want to provide a PR for this?

@kueckermann
Copy link
Author

Thanks @daanporon
You can PR when you guys are ready, its not an urgent fix.

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

No branches or pull requests

3 participants