[refactor] #2001: `EvaluatesTo` static type checking

[Arjentix]

Description of the Change

EvaluatesTo (#2001)

• Into EvaluatesTo<V> conversion now has stricter bounds which provides static type checking
• For some cases there is no possibility to make a compile-time check (i.e. Where, ContextValue expressions and all queries) so we need a way to explicitly construct EvaluatesTo<V> omitting the type checking. That's why I've created EvaluatedTo::<V>::new_unchecked()
• Since all expressions and queries return impl TryFrom<Value> it's always safe to construct EvaluatesTo<Value>. That's why there is EvaluatesTo::<Value>::new_evaluates_to_value()
• Added ui test which ensures that there is no way to implicitly construct EvaluatesTo from uncompatible type

While doing that I've found a bug in Queue and fixed it. See next section.

Tx signature condition checking bug (#2580)

• Added MustUse type wrapper to primitives
• MustUse used for a checking function
• Added test which ensures that you get unused_must_use warning will be generated if MustUse type is unused
• Fixed actual bug

Issue

• Closes #2001
• Closes #2580

After succesfull review I'll squash my commits into two -- one per resolved issue

Benefits

EvaluatesTo (#2001)

• Now it's easier to write correct tests
• Found the important bug

Tx signature condition checking bug (#2580)

• Added helpful MustUse wrapper type
• Important bug fixed

Possible Drawbacks

Harder to construct EvaluatesTo from types which require runtime check

Usage Examples or Tests

You can run existing test, because a bunch of them where updated.

Also there are two new tests with trybuild:

EvaluatesTo

cargo test --package iroha_data_model --test ui -- ui --exact --nocapture 

MustUse

cargo test --package iroha_primitives --test ui -- ui --exact --nocapture

Alternate Designs

@appetrosyan suggested to add ok_or(err) function to MustUse which should work something like this:

impl MustUse<bool> {
    pub fn ok_or<E>(err: E) -> Result<(), E> {
        if self.0 {
            Ok(())
        } else {
            Err(E)
        }
    }
}

But I have next the thoughts:

1. It's not that usable as it may look
2. std ok_or does not force you to return () as Ok
3. It does not follow the Unix rule and Single-responsibility rule

[appetrosyan]

I'd like to defend my proposal.

First, I proposed introducing a trait, not an inherent impl, which in this case can work transitively:

pub trait ValueIsError<E: Error> {
  const VALUE: Self;

  fn ok_or(&self, err: E) -> Result<E> {
    if *self == VALUE {
      err
    } else {
      Ok(())
    }
  }
} 

impl<E: Error> ValueIsError<(), E> for bool {
  const VALUE: Self = false;
} 

1. bool has the function then which converts the true value into an option. It is the equivalent of Result::map. We need a similar pattern, except we want to convert a boolean into a short-circuit with an error message.

The point of this isn't to reduce the typing by orders of magnitude, but to allow monadic chaining of such conversions. It might seem stupid to use map and or_else in most cases, but it allows you to flatten the decision trees.

2. If we want to convert it to something else, we can use map. We can also use then, followed by ok_or(Err), in case we want to also have a value other than Ok(()).

3. If you're referring to "do one thing and do it well", then neither does then.

[Arjentix]

Failing test

Yes, it seems like we have contradictory requirements for transaction Queue in our tests:

• iroha_core::queue::tests::push_tx_signature_condition_failure — requires to deny transaction if signature condition failied
• iroha_client::mod::integration::multisignature_transaction::multisignature_transactions_should_wait_for_all_signatures — requires to allow transactions from multisig account so that it can wait for other signatures

We didn't catch that before because the first test was wrong

[Arjentix]

@appetrosyan , alright, now the idea of ok_or seems more reasonable to me. However I stil have one question: there is T in trait definition and it's used as ok_or Ok value, but you return Ok(()). So maybe remove T at all?

[Arjentix]

@appetrosyan and I have decided to ignore multisignature test for now. That's will be fixed in #2595

[Arjentix]

@appetrosyan , I've created an issue for ValueIsError: #2598

[Arjentix]

CI is broken :(

[codecov[bot]]

Codecov Report

Merging #2582 (0d8ad32) into iroha2-dev (a16d9c3) will decrease coverage by 2.03%. The diff coverage is 66.47%.

:exclamation: Current head 0d8ad32 differs from pull request most recent head cf8f9c8. Consider uploading reports for the commit cf8f9c8 to get more accurate results

@@              Coverage Diff               @@
##           iroha2-dev    #2582      +/-   ##
==============================================
- Coverage       67.61%   65.57%   -2.04%     
==============================================
  Files             140      157      +17     
  Lines           26173    28357    +2184     
==============================================
+ Hits            17696    18596     +900     
- Misses           8477     9761    +1284     

Impacted Files	Coverage Δ
cli/derive/src/lib.rs	74.72% <ø> (ø)
cli/src/torii/mod.rs	28.88% <ø> (ø)
cli/src/torii/routing.rs	69.92% <0.00%> (-2.87%)	:arrow_down:
client/src/client.rs	43.36% <0.00%> (-2.96%)	:arrow_down:
client_cli/src/main.rs	0.25% <0.00%> (-0.01%)	:arrow_down:
config/base/src/runtime_upgrades.rs	35.63% <ø> (ø)
core/src/block.rs	70.00% <ø> (ø)
core/src/smartcontracts/isi/asset.rs	54.82% <ø> (ø)
core/src/sumeragi/message.rs	8.20% <0.00%> (ø)
core/src/sumeragi/mod.rs	0.00% <ø> (ø)
... and 164 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.