Skip to content
New issue

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

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

Already on GitHub? # to your account

[refactor]: split iroha_torii from iroha #4139

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

0x009922
Copy link
Contributor

Description

This PR splits Torii (web server functionality), located in the main CLI, into a separate iroha_torii crate.

The rationale is to make a step towards making the main CLI exclusively a binary. Currently iroha is a library as well, exposing functionality to run Iroha in tests. That includes Torii. Since Torii is quite self-contained, making it a separate crate seem to make sense.

A further step would be to move Iroha itself and iroha::samples out of the CLI, so that test_network doesn't depend on iroha anymore, and iroha might be exclusively a binary.

Additionally:

  • fix telemetry feature flag. Torii code didn't compile if this feature was disabled.
  • clean dependencies of iroha CLI

Linked issue

Addresses #4136, but doesn't close it.

@0x009922 0x009922 added iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality Chore This is a small task that can be done at any point in time and is easier than others labels Dec 12, 2023
@0x009922 0x009922 self-assigned this Dec 12, 2023
@0x009922 0x009922 mentioned this pull request Dec 12, 2023
6 tasks
@coveralls
Copy link

Pull Request Test Coverage Report for Build 7190952353

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 1 of 241 (0.41%) changed or added relevant lines in 4 files are covered.
  • 6881 unchanged lines in 124 files lost coverage.
  • Overall coverage decreased (-3.2%) to 56.211%

Changes Missing Coverage Covered Lines Changed/Added Lines %
torii/src/utils.rs 0 1 0.0%
torii/src/routing.rs 0 9 0.0%
torii/src/lib.rs 0 230 0.0%
Files with Coverage Reduction New Missed Lines %
config/base/derive/src/view.rs 1 99.37%
config/src/block_sync.rs 1 95.0%
config/src/network.rs 1 93.75%
config/src/torii.rs 1 95.45%
config/src/wasm.rs 1 87.5%
core/src/smartcontracts/isi/block.rs 1 87.5%
ffi/src/option.rs 2 71.43%
config/src/genesis.rs 3 72.92%
config/src/logger.rs 3 92.68%
data_model/derive/src/has_origin.rs 3 95.16%
Totals Coverage Status
Change from base Build 5423219773: -3.2%
Covered Lines: 23054
Relevant Lines: 41013

💛 - Coveralls

@mversic
Copy link
Contributor

mversic commented Dec 13, 2023

I'm not sure we need this. I'd like to get others opinions on this. Having multiple crates might speed up compilation, but I don't see it in this case specifically

@6r1d
Copy link
Contributor

6r1d commented Dec 13, 2023

Given the rationale, I don't see the compilation speed as a main aspect. I agree with the rationale, and I'm in favour of the change.

@mversic
Copy link
Contributor

mversic commented Dec 14, 2023

The rationale is to make a step towards making the main CLI exclusively a binary. Currently iroha is a library as well, exposing functionality to run Iroha in tests. That includes Torii. Since Torii is quite self-contained, making it a separate crate seem to make sense.

iroha shouldn't be a library, that's only an artifact of our test_network which we should remove from the codebase. It was a poor design choice for integration tests. At that point I'm not sure if it would make much sense to separate iroha and torii as separate crates anymore since torii will ever only be a dependency of iroha

mversic
mversic previously approved these changes Dec 14, 2023
@Arjentix Arjentix self-assigned this Dec 14, 2023
Arjentix
Arjentix previously approved these changes Dec 14, 2023
cli/Cargo.toml Show resolved Hide resolved
torii/derive/src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
@0x009922 0x009922 dismissed stale reviews from Arjentix and mversic via 0c07859 December 14, 2023 23:38
@Erigara
Copy link
Contributor

Erigara commented Dec 15, 2023

I just got another idea why using splitting torii might be good idea.
I think for some network setups it might be useful to have nodes for fault tolerance, but this nodes want serve users requests and will only participate in consensus.
So torii could be excluded from compilation.

@Erigara Erigara assigned mversic and Erigara and unassigned mversic Dec 15, 2023
@0x009922
Copy link
Contributor Author

So torii could be excluded from compilation.

Well, Torii might be excluded with feature flags anyway, without necessarily being a separate crate.

@0x009922 0x009922 merged commit e33208d into hyperledger-iroha:iroha2-dev Dec 15, 2023
11 checks passed
@0x009922 0x009922 deleted the torii-as-crate branch December 15, 2023 08:09
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this pull request Jan 9, 2024
* [refactor]: split `iroha_torii` from `iroha`

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>

* [test]: fix doctest

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>

* [refactor]: rename `iroha_torii_<derive>` to `*_<macro>`

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>

---------

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this pull request Jan 22, 2024
* [refactor]: split `iroha_torii` from `iroha`

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>

* [test]: fix doctest

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>

* [refactor]: rename `iroha_torii_<derive>` to `*_<macro>`

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>

---------

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
Signed-off-by: Asem-Abdelhady <asemshawkey@gmail.com>
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this pull request Feb 9, 2024
* [refactor]: split `iroha_torii` from `iroha`

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>

* [test]: fix doctest

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>

* [refactor]: rename `iroha_torii_<derive>` to `*_<macro>`

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>

---------

Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Chore This is a small task that can be done at any point in time and is easier than others iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants