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

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

Merged

Conversation

Arjentix
Copy link
Contributor

@Arjentix Arjentix commented Aug 2, 2022

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

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

@Arjentix Arjentix added Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality Security This issue asks for improved security labels Aug 2, 2022
@Arjentix Arjentix self-assigned this Aug 2, 2022
@Arjentix Arjentix changed the title [refactor] 2001: EvaluatesTo static type checking [refactor] #2001: EvaluatesTo static type checking Aug 2, 2022
appetrosyan
appetrosyan previously approved these changes Aug 2, 2022
primitives/src/must_use.rs Outdated Show resolved Hide resolved
data_model/src/lib.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
core/src/queue.rs Show resolved Hide resolved
@appetrosyan appetrosyan dismissed their stale review August 2, 2022 16:13

Failing test

data_model/src/expression.rs Outdated Show resolved Hide resolved
data_model/src/account.rs Outdated Show resolved Hide resolved
@appetrosyan
Copy link
Contributor

appetrosyan commented Aug 2, 2022

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.

  1. 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(()).

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

@Arjentix
Copy link
Contributor Author

Arjentix commented Aug 2, 2022

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
Copy link
Contributor Author

Arjentix commented Aug 2, 2022

@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?

@Erigara Erigara self-assigned this Aug 3, 2022
Copy link
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

Solution looks good in general, left few suggestions.

Some thoughts: i think when user need to wrpar expression into EvaluatesTo::new_unchecked it will be reason for double check, however current solution starts to get wordy when it comes nested expressions.

data_model/src/expression.rs Outdated Show resolved Hide resolved
primitives/src/must_use.rs Show resolved Hide resolved
core/src/tx.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/sumeragi/fault.rs Outdated Show resolved Hide resolved
primitives/src/must_use.rs Outdated Show resolved Hide resolved
data_model/src/lib.rs Outdated Show resolved Hide resolved
@appetrosyan appetrosyan self-assigned this Aug 5, 2022
@Arjentix Arjentix mentioned this pull request Aug 8, 2022
@Arjentix
Copy link
Contributor Author

Arjentix commented Aug 8, 2022

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

@Arjentix
Copy link
Contributor Author

Arjentix commented Aug 8, 2022

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

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
data_model/src/expression.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
appetrosyan
appetrosyan previously approved these changes Aug 16, 2022
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
appetrosyan
appetrosyan previously approved these changes Aug 16, 2022
core/src/queue.rs Outdated Show resolved Hide resolved
data_model/src/expression.rs Show resolved Hide resolved
data_model/src/expression.rs Outdated Show resolved Hide resolved
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Copy link
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

Good job

@appetrosyan appetrosyan merged commit a2b373d into hyperledger:iroha2-dev Aug 16, 2022
@Arjentix Arjentix deleted the evaluates_to_type_checking branch August 16, 2022 12:53
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 6, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 6, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 6, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 7, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
BAStos525 pushed a commit to mversic/iroha that referenced this pull request Sep 7, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: BAStos525 <jungle.vas@yandex.ru>
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 8, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 9, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 9, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
BAStos525 pushed a commit to mversic/iroha that referenced this pull request Sep 9, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 9, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
BAStos525 pushed a commit to mversic/iroha that referenced this pull request Sep 9, 2022
…rledger#2582)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality Security This issue asks for improved security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants