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

REST api added for asset transfer in Golang #836

Merged
merged 3 commits into from Dec 14, 2022
Merged

Conversation

basilky
Copy link
Contributor

@basilky basilky commented Sep 23, 2022

This PR adds a REST server in Golang with endpoints for chaincode invoke and query. I I'm not an expert in Golang. Let me know if any improvements required.

@basilky basilky requested a review from a team as a code owner September 23, 2022 16:36
Signed-off-by: Basil K Y <techiebasil@gmail.com>
Signed-off-by: Basil K Y <techiebasil@gmail.com>
Signed-off-by: Basil K Y <techiebasil@gmail.com>
@jt-nti
Copy link
Member

jt-nti commented Oct 20, 2022

Hi @basilky, thanks for the contribution! It's really good to get a sample using the new Fabric Gateway!

It looks like your REST api is actually a generic server which could run transactions for any sample, rather than being specifically related to the asset transfer sample- is that right? For contrast, there is a more chaincode specific Go REST sample in the recent full stack workshop repository.

I've actually wanted a more generic REST api for a while, and neither your server or the full stack sample are equivalent to the rest-api-typescript sample, which has more focus on errors and retries, so I wonder if moving this code out of asset-transfer-basic would be better, e.g. rest-client-go or something at the root of the repository? Hopefully one of the samples maintainers can comment?

As for the Go code, I'm not an expert but it looks like you're reusing the GRPC connection/Gateway which is great. I would personally like to see more configuration done through env vars rather than being hard coded to the test-network directories but I think it would be fine to merge what you have so far and iterate on it.

Thanks again.

@basilky
Copy link
Contributor Author

basilky commented Oct 22, 2022

@jt-nti Thanks for the feedback. The REST server is written in a generic way. The channel name, chaincode name, function and arguments are received from the request. I also think it's better to move the code out of asset-transfer.

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.

I'm happy to merge this as is for now. Followup PRs to address @jt-nti 's comments would be welcome.
Thank you for your contribution!

@lehors lehors merged commit b2de360 into hyperledger:main Dec 14, 2022
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

3 participants