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

Add account-based token sample (go chaincode) #318

Merged
merged 1 commit into from Oct 14, 2020
Merged

Add account-based token sample (go chaincode) #318

merged 1 commit into from Oct 14, 2020

Conversation

denyeart
Copy link
Contributor

@denyeart denyeart commented Sep 1, 2020

Add an account-based token sample go chaincode, as well as an associated readme tutorial.

Signed-off-by: David Enyeart enyeart@us.ibm.com

@denyeart denyeart requested a review from a team as a code owner September 1, 2020 11:12
Copy link
Contributor

@nikhil550 nikhil550 left a comment

Choose a reason for hiding this comment

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

Ran the sample on my local machine. Looks good, but left some nits on the readme.

token-account-based/README.md Show resolved Hide resolved
token-account-based/README.md Outdated Show resolved Hide resolved
token-account-based/README.md Outdated Show resolved Hide resolved
token-account-based/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lehors lehors left a comment

Choose a reason for hiding this comment

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

LGTM overall. Just a few suggestions for improvement along those made by other reviewers.

token-account-based/README.md Show resolved Hide resolved
token-account-based/README.md Outdated Show resolved Hide resolved
token-account-based/README.md Outdated Show resolved Hide resolved
@denyeart
Copy link
Contributor Author

Thanks folks. I'll make the updates here. And I will also add the remaining ERC20 functions to make it functionally identical with the Javascript PR: #327

Copy link
Contributor

@sijocherian sijocherian left a comment

Choose a reason for hiding this comment

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

ERC20 interface signature sounds better for adoption.

We may want to be conscious of the Terminology we use in the sample: like AccountID, ClientID , Owner , User, Recipient

ERC20 uses the terminology "balance of an account with address _owner"
https://eips.ethereum.org/EIPS/eip-20

Account & Balance seem like the most compatible ones. I am on the fence about "address"

Add an account-based token sample go chaincode, as well as an associated readme tutorial.

Signed-off-by: David Enyeart <enyeart@us.ibm.com>
Copy link
Contributor

@lehors lehors left a comment

Choose a reason for hiding this comment

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

LGTM!

@lehors lehors merged commit fa0529c into hyperledger:master Oct 14, 2020
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

Successfully merging this pull request may close these issues.

None yet

4 participants