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

Static type checking of EvaluatesTo not working #2001

Closed
mversic opened this issue Mar 23, 2022 · 1 comment
Closed

Static type checking of EvaluatesTo not working #2001

mversic opened this issue Mar 23, 2022 · 1 comment
Assignees
Labels
Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST

Comments

@mversic
Copy link
Contributor

mversic commented Mar 23, 2022

I've noticed that our type checking of EvaluatesTo is not giving out compile errors when it is expected to do so.

The following test should not compile, yet it does:

#[test]
fn genesis_block_is_commited_with_some_offline_peers() {
    // Given
    let rt = Runtime::test();
    let (network, mut iroha_client) = rt.block_on(<Network>::start_test_with_offline(4, 1, 1));
    wait_for_genesis_committed(&network.clients(), 1);

    //When
    // NOTE: I have used `AssetDefinitionId` instead of `AccountId` for demonstration purpose
    let alice_id: AssetDefinitionId = "alice#wonderland".parse().expect("Valid");					
    let alice_has_roses = 13;

    //Then
    let assets = iroha_client
        // NOTE: This should not compile because `client::asset::by_account_id`
        // should only accept expressions that evaluate to `AccountId`!
        .request(client::asset::by_account_id(alice_id))
        .expect("Failed to execute request.");
    let asset = assets
        .iter()
        .find(|asset| asset.id().definition_id == "rose#wonderland".parse().expect("Valid"))
        .unwrap();
    assert_eq!(AssetValue::Quantity(alice_has_roses), *asset.value());
}

Notice that client::asset::by_account_id as it's argument accepts impl Into<EvaluatesTo<AccountId>>, yet when given AssetDefinitionId it still compiles as if there is an implementation AssetDefinitionId: Into<AccountId>.

@mversic mversic added Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST labels Mar 23, 2022
@Erigara Erigara self-assigned this Jun 9, 2022
@Erigara
Copy link
Contributor

Erigara commented Jun 9, 2022

@appetrosyan, @mversic if we look at:

impl<V: TryFrom<Value>, E: Into<ExpressionBox>> From<E> for EvaluatesTo<V> {
    fn from(expression: E) -> Self {
        Self {
            expression: expression.into(),
            _value_type: PhantomData::default(),
        }
    }
}

Nothing prevent us from having:

impl From<AssetDefinitionId> for EvaluatesTo<AccountId> {
    fn from(expression: AssetDefinitionId) -> Self {
        Self {
            // AssetDefinitionId into ExpressionBox
            expression: expression.into(),
            _value_type: PhantomData::default(),
        }
    }
}

impl From<AccountId> for EvaluatesTo<AccountId> {
    fn from(expression: AccountId) -> Self {
        Self {
           // AccountId into ExpressionBox
            expression: expression.into(),
            _value_type: PhantomData::default(),
        }
    }
}

EDIT: so the problem is that when we require type value: impl Into<EvaluatesTo<V>, we really only require value: impl Into<ExpressionBox>.

@Erigara Erigara removed their assignment Jul 28, 2022
@Arjentix Arjentix self-assigned this Aug 1, 2022
appetrosyan pushed a commit that referenced this issue Aug 16, 2022
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
mversic pushed a commit to mversic/iroha that referenced this issue Sep 6, 2022
…rledger#2582)

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

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

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

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

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

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

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

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

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
BAStos525 pushed a commit to mversic/iroha that referenced this issue 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
Projects
None yet
Development

No branches or pull requests

4 participants