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

Implement iroha-lib separate module for Hyperledger Iroha 1 #2660

Merged
merged 1 commit into from
Oct 26, 2022

Conversation

andprogrammer
Copy link
Contributor

@andprogrammer andprogrammer commented Aug 26, 2022

This is a source code for HL Iroha C++ library.

Currently, the latest HL Iroha 1.5 release (hyperledger/iroha:latest Docker image) is supported.
Possible actions:

  • transactions
  • batch transactions
  • queries

@baziorek baziorek added 1.x api-changes Changes in the API for client libraries 1.5 and removed api-changes Changes in the API for client libraries labels Aug 26, 2022
@baziorek baziorek self-requested a review August 26, 2022 08:44
@andprogrammer andprogrammer temporarily deployed to test-env October 14, 2022 10:34 Inactive
@andprogrammer andprogrammer marked this pull request as ready for review October 14, 2022 10:48
@appetrosyan
Copy link
Contributor

Think about tests - but we need to ask Iroha-Core-Team about them. For me - if examples are working and You are wrapping generated from protobuf files: tests are not mandatory, but we have to ask @appetrosyan?

I'd recommend looking at the coverage first. Iroha 1 in general has a very different approach to testing, so IMO unit tests in this case are unnecessary.

Extra work

  • refactoring of iroha-cli, which should use the newly introduced library.

I fully agree. However, I'd be pragmatic, it seems like a good task for another intern.

Support for HL Ursa

Yes, but not now. Ursa is going to undergo a significant change, so I'd wait until at least Christmas before starting this work.

  • Wrap the library to be compatible with C language

C-abi library sounds good, but it's going to take a lot of extra work and meticulous code review. Dynamic linkage with a C++-based binary is non-trivial.

Documentation - how to install the library

Agreed. I'll help with the review.

Comment on lines +4 to +7
This is a source code for HL Iroha C++ library.

Currently, latest HL Iroha 1.5 release (`hyperledger/iroha:latest` Docker image) is supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

This readme tells the user next to nothing about what the library is supposed to do. If it's an example, you should say so. If it is a library that does stuff, similarly it should explain what it does, and why it's included in the source code of Iroha.

@appetrosyan appetrosyan temporarily deployed to test-env October 21, 2022 06:26 Inactive
@appetrosyan appetrosyan temporarily deployed to test-env October 21, 2022 06:26 Inactive
@appetrosyan appetrosyan temporarily deployed to test-env October 21, 2022 06:26 Inactive
@appetrosyan appetrosyan temporarily deployed to test-env October 21, 2022 06:26 Inactive
@andprogrammer andprogrammer temporarily deployed to test-env October 25, 2022 22:13 Inactive
@andprogrammer andprogrammer temporarily deployed to test-env October 25, 2022 22:13 Inactive
@andprogrammer andprogrammer temporarily deployed to test-env October 25, 2022 22:13 Inactive
@andprogrammer andprogrammer temporarily deployed to test-env October 25, 2022 22:13 Inactive
Copy link
Contributor

@baziorek baziorek left a comment

Choose a reason for hiding this comment

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

It looks almost for good me. Only some examples are missing and documentation (especially how to use the library), but they can be added in another PR.
That is why this PR can be merged as it is.

Good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.5 iroha1 The legacy version of Iroha.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants