integritee-cli: separate into library and binary

[n-prat]

Hello,

Following up on #1167 I have started a basic refactoring of the cli.
I needed to use integritee-cli from Android(and soon iOs) as a library so I cherry-picked and cleanup the required changes from our current branch and I am proposing this refactor.

NOTE: For this first draft, I aimed for the "minimal changeset" and not really for the "cleanest lib interface"...
I am open for feedback!

Overview of the changes:

• most code from main.rs moved to lib.rs; with added structs CliResultOk, CliError, CliResult
• all *Commands return CliResult
• most of the logic changes are in cli/src/trusted_operation.rs
  • perform_trusted_operation now return new struct TrustedOpResult
  • I tried to correctly identify the previous "None means an error" vs "None means there was nothing to return"

TBD:

• do you want me to "go deeper" refactoring and expose a "proper lib"
  ie expose all Subcommand directly as pub fn[ideally keeping the integritee-cli as-is to not break anything]
• I could also in the PR ensure the cli works on Android; to do this two options: patch sp-wasm-interface and remove deps on wasmi+wasmtime; or patch wasmtime to allow compiling on Android+Emulator
  • For this option, not sure how to implement it; in my project I have a dirty "patch" but ideally it should be done in this repo instead

[clangenb]

Thanks for the PR, I am very sorry that I did not have the time yet to have a look at it. I hope to find time next week!

[n-prat]

No rush! This is not blocking for me.
Thanks for the update.

[clangenb]

Thanks again for your contribution!

Sorry, for coming back so late to this! I must say that I very much like the approach and I think we don't need many further adjustments for this to merge it!

I just have minor remarks and a question. I would ask you to make this PR up-to-date, so we can run it in our CI. :)

[clangenb]

Are you going to pick this up again? We are actually now having a business interest in this PR too, so if you don't work on this soonish, do you mind if someone from our team takes this task?

[clangenb]

Regarding your questions:

do you want me to "go deeper" refactoring and expose a "proper lib"
ie expose all Subcommand directly as pub fn[ideally keeping the integritee-cli as-is to not break anything]

I think the current state is ok to merge it. In the future, we can have a deeper refactoring so that the crate has a more lib-like interface, but i think it it fine for now.

I could also in the PR ensure the cli works on Android; to do this two options: patch sp-wasm-interface and remove deps on wasmi+wasmtime; or patch wasmtime to allow compiling on Android+Emulator. For this option, not sure how to implement it; in my project I have a dirty "patch" but ideally it should be done in this repo instead.

In essence, this would be a cool feature, but we are probably going to have a pallet-contracts dependency (which needs wasm) soon to allow confidential smart contract execution; hence we can't just patch the wasm stuff. (as patches globally apply to the workspace). However, with some smart refactoring we might be able to get rid of the wasm stuff in the CLI and hence we don't need to patch anything at all.

[n-prat]

I am definitely still interested in this feature.
But I can't work on it right now. Next week is doable.
Would that work for you? Else I don't mind if someone from your team wants to take over.

About the process: would you be ok with a rebase+force push to get up to date?
Or rather a merge?

[clangenb]

Very cool! We won't be able to tackle it immediately either, I was just poking to see if you are still interested in continuing. :)

You can bring this PR up-to-date by whatever fits you. We only do squash & merge, so it will be one single commit in the end anyhow.

[n-prat]

I have just squashed+rebased on master to clean up the dozen of commits I had on this branch.

Still some work to done about your comments (snafu to remove, cleanup, etc.) and I have probably left some TODO.
Also need to add some basic tests.
And need to at least compile the integritee-cli for Android or some other target.

[clangenb]

Thanks for the update. I have approved your PR now as a test because we are trying to make our CI run for external contributors. We have recently migrated to self-hosted runners, and we don't know yet, how we can enable it to you.

[OverOrion]

Hey @nathanprat,

Could you please add an empty commit to this PR? I believe it will help the CI get out of the stuck state.

[n-prat]

Done.

Note that I had a [ci skip] in the previous commit so that could explain things... My bad.

[OverOrion]

Well, only EVM feature seems to be broken, which I think is great, seems to be an easy fix:
(I paste the CI compilation failure in case you do not have access to the logs)

#16 124.9 error[E0308]: mismatched types
#16 124.9   --> cli/src/evm/commands/evm_command_utils.rs:24:29
#16 124.9    |
#16 124.9 24 |         let nonce: Index = if let Some(n) = res {
#16 124.9    |                                   ^^^^^^^   --- this expression has type `std::result::Result<std::option::Option<Vec<u8>>, TrustedOperationError>`
#16 124.9    |                                   |
#16 124.9    |                                   expected enum `std::result::Result`, found enum `std::option::Option`
#16 124.9    |
#16 124.9   ::: cli/src/evm/commands/evm_call.rs:66:25
#16 124.9    |
#16 124.9 66 |         let evm_nonce = get_layer_two_evm_nonce!(sender, cli, trusted_args);
#16 124.9    |                         --------------------------------------------------- in this macro invocation
#16 124.9    |
#16 124.9    = note: expected enum `std::result::Result<std::option::Option<Vec<u8>>, TrustedOperationError>`
#16 124.9               found enum `std::option::Option<_>`
#16 124.9    = note: this error originates in the macro `get_layer_two_evm_nonce` (in Nightly builds, run with -Z macro-backtrace for more info)
#16 124.9 help: try wrapping the pattern in `Ok`
#16 124.9    |
#16 124.9 24 |         let nonce: Index = if let Ok(Some(n)) = res {
#16 124.9    |                                   +++       +
#16 124.9 

And this type of error is repeated like 4 times.

[n-prat]

Quick commits to fix the issues raised by CI :

• update in cli/src/evm/commands/; cargo clippy --features evm -- -D warnings now passes
• "taplo": taplo fmt --check now passes

[clangenb]

Thanks for the fixes!

I think we now have a state that is ready to merge, or do you have any open points?

If not, I would like to ask you to merge master into your branch and we merge your PR afterwards.

[n-prat]

I still need to handle what you said about indentation(I'm doing it now). While I'm at it, I will rebase to get it up to date.

The only open point I see is the TODO on CliResultOk::None: I put it here as a placeholder/reminder that some functions are not properly exposed in the cli.
Ideally it should be removed and all the calls to eg cmd.run(cli) should directly return a CliResult.
But I am fine with merging it that way now, and down the line the needs arise for new variants, it will be a quick PR away.

edit: I apparently need to fix test_version, and also call them from CI b/c this not yet the case 🤦

[n-prat]

• updated: get_layer_two_evm_nonce and get_layer_two_nonce with your remark about indentation
• updated: cli/tests/basic_tests.rs: tests were not passing
• rebased branch to get it up-to-date

Still open:

• cli/tests/basic_tests.rs is NOT called from CI; I don't really understand how you CI is structured so I am going to need some pointers if you want me to call these in CI
• cf "TODO on CliResultOk::None"

[clangenb]

Hi @n-prat,

Thanks for the update! I can say though that your tests are being executed. The snippet below is from the insanely long log of the Build Worker and Cargo test stage of the last run. However, I agree that we should simplify our CI to make this more obvious, which I think should be very much possible:

 #16 351.9 test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
#16 351.9 
#16 351.9      Running tests/basic_tests.rs (target/release/deps/basic_tests-e0bbeb8dbde7f650)
#16 351.9 
#16 351.9 running 2 tests
#16 351.9 test test_version ... ok
#16 351.9 test test_help ... ok

The question is now, these tests should have been called before too, which means that the CI might not catch when certain tests failed, see #1299, but investigation should not be done in your PR.

That being said, I think I am fine with the return values for now, and I would favor a follow-up PR to improve the current code.

I just have the small question from the above comment, otherwise, I think we are ready to merge it!

[n-prat]

I had forgotten to replace Snafu by thiserror.
Should be good now!

[clangenb]

Thanks a lot, I was even going to accept snafu for now, but this is even better. 👍

I very much appreciate the collaboration we had in this PR!

Cheers

[n-prat]

Same! Thanks for merging this!
Cheers