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

GetHistoryForKey added to the chaincode-typescript #1226

Closed
wants to merge 1 commit into from

Conversation

ajiths10
Copy link

Added a chaincode transaction to retrieve the complete history of an asset using its asset ID.

@ajiths10 ajiths10 requested a review from a team as a code owner June 19, 2024 09:52
Signed-off-by: Ajiths10 <ajithsnair10@gmail.com>
Copy link
Contributor

@satota2 satota2 left a comment

Choose a reason for hiding this comment

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

@ajiths10 Thank you for submitting the PR.

Since asset-transfer-basic is the most fundamental sample, it should provide only the minimal necessary functions. Adding your proposed GetHistoryForKey to this sample might be too advanced.

If you believe that a sample TypeScript implementation of GetHistoryForKey is necessary, it would be more appropriate to implement the TypeScript version of the asset-transfer-ledger-queries sample, as it already includes the GetHistoryForKey functionality. What do you think about this approach?

@denyeart
Copy link
Contributor

@ajiths10 Thank you for submitting the PR.

Since asset-transfer-basic is the most fundamental sample, it should provide only the minimal necessary functions. Adding your proposed GetHistoryForKey to this sample might be too advanced.

If you believe that a sample TypeScript implementation of GetHistoryForKey is necessary, it would be more appropriate to implement the TypeScript version of the asset-transfer-ledger-queries sample, as it already includes the GetHistoryForKey functionality. What do you think about this approach?

Makes sense to me. Note that there is a chaincode-javascript reference with GetHistoryForKey() that can be used as reference. Ideally the various languages would have the same support in the asset-transfer-ledger-queries.

@ajiths10
Copy link
Author

Hi @satota2 ,

I believe GetHistoryForKey is a fundamental function available in many other languages, including JavaScript. Adding this functionality to TypeScript would be highly beneficial and valuable to the TypeScript community. Since I found it challenging to locate this code in community spaces, I decided to include it in a TypeScript sample myself.

What are your thoughts?

@satota2
Copy link
Contributor

satota2 commented Jun 26, 2024

@ajiths10

Thank you for your response.

While GetHistoryForKey is indeed a fundamental function as you mentioned, compared to the basic asset CRUD and transfer operations handled in asset-transfer-basic, its general usage frequency are relatively lower. Therefore, as I previously mentioned, I believe adding it to this simple scenario sample might be too much.

On the other hand, I agree that having a TypeScript sample for GetHistoryForKey would be very beneficial. So, as I previously suggested, I propose implementing a TypeScript version of the asset-transfer-ledger-queries sample. This sample includes other ledger query functionalities in addition to GetHistoryForKey, making it more valuable for TypeScript users.

Regarding the difficulty in finding the GetHistoryForKey functionality, I agree that it needs improvement. As a potential solution, including a note in the asset-transfer-ledger-queries description that mentions it contains GetHistoryForKey might help navigate users better.

What do you think?

@ajiths10
Copy link
Author

@ajiths10

Thank you for your response.

While GetHistoryForKey is indeed a fundamental function as you mentioned, compared to the basic asset CRUD and transfer operations handled in asset-transfer-basic, its general usage frequency are relatively lower. Therefore, as I previously mentioned, I believe adding it to this simple scenario sample might be too much.

On the other hand, I agree that having a TypeScript sample for GetHistoryForKey would be very beneficial. So, as I previously suggested, I propose implementing a TypeScript version of the asset-transfer-ledger-queries sample. This sample includes other ledger query functionalities in addition to GetHistoryForKey, making it more valuable for TypeScript users.

Regarding the difficulty in finding the GetHistoryForKey functionality, I agree that it needs improvement. As a potential solution, including a note in the asset-transfer-ledger-queries description that mentions it contains GetHistoryForKey might help navigate users better.

What do you think?

@satota2 ,
Yes, that sounds good!

@satota2
Copy link
Contributor

satota2 commented Jun 26, 2024

@ajiths10 Thank you for agreeing.
I have created two issues based on the above discussion, so I would like to close this PR.

If you are interested in helping resolve these issues, I would greatly appreciate your collaboration.
Would you be willing to help? Of course, even if you can assist with just one of the issues, it would be very welcome.

@ajiths10
Copy link
Author

@ajiths10 Thank you for agreeing. I have created two issues based on the above discussion, so I would like to close this PR.

If you are interested in helping resolve these issues, I would greatly appreciate your collaboration. Would you be willing to help? Of course, even if you can assist with just one of the issues, it would be very welcome.

@satota2 Thanks for addressing this. I look forward to it.

@satota2
Copy link
Contributor

satota2 commented Jun 26, 2024

@ajiths10 OK. I close this PR.
If you are interested in taking on any of these issues, please leave a comment on the respective issue. I will then assign the issue to you. This will help avoid duplicate work by other comunity members (including possibly myself) on the same issue.

@satota2 satota2 closed this Jun 26, 2024
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.

3 participants