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

[suggestion] Make illegal instructions unrepresentable #3612

Closed
appetrosyan opened this issue Jun 15, 2023 · 3 comments
Closed

[suggestion] Make illegal instructions unrepresentable #3612

appetrosyan opened this issue Jun 15, 2023 · 3 comments
Assignees
Labels
Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST

Comments

@appetrosyan
Copy link
Contributor

appetrosyan commented Jun 15, 2023

Feature request

Some combinations of instructions are not legal and never will be, for example Mint<String>. As such we may be able to constrain the expression language to prevent illegal values to be representable and as such stop them from being decoded.

Motivation

This was not previously possible because of lack of GATs in stable Rust. Now that the support had been stabilised, we can express the correct constraints in the trait bounds. And perhaps refactor the Expression and Instruction systems to be more strict.

One suggestion is to rethink the expressions as not just something that generates data for an instruction, but an entire transaction. So instead of

pub enum Executable {
    Instruction(Vec<InstructionBox>), 
    WASM(WasmBlob), 
}

// Where instructions have a nested expressionBox for every value

we can have

pub enum Transaction {
    Register(RegisterBox),
    Mint(MintBox), 
    Burn(BurnBox), 
    Transfer(TransferBox),
    // ... 
    Expression(ExpressionBox), 
    Wasm(WasmBlob),
}

This has the advantage of allowing us to use an optimised code path for common instructions. Removing the redundant top-level Expression::Sequence. Remove the need for Expression::Raw. Allow injecting the expected type information into the expression and fail eagerly.

More importantly, it allows us to clean out arithmetic from string-related code, string-related code from numerical asset logic, booleans from almost all places, and potentially inline queries whose type is known ahead of time. This is important, because we will then not have to deal with FindAllAccounts as an argument to Transfer. Finally, this trivialises #2409.

This has one significant drawback and I propose only considering it, if we are certain that the benefits outweigh the costs.

Who can help?

@

@appetrosyan appetrosyan added Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST labels Jun 15, 2023
@mversic
Copy link
Contributor

mversic commented Jun 15, 2023

Remove the need for Expression::Raw. Allow injecting the expected type information into the expression and fail eagerly.

If I understand you correctly, you're saying that we can have our expression system inherently strongly typed. Since we kinda already have this with EvaluatesTo<T> not much should change from the API standpoint but we would get more optimized underlying representation of expression, faster deserialization, etc. This would entail removing EvaluatesTo<T> and using Expression<T> instead of Expression. We can do this because we know type expression will evaluate to when constructing the expression. Some parsers don't do it this way, namely Rust syn library. I'd like to hear why some parsers choose to be strongly typed and some don't

One suggestion is to rethink the expressions as not just something that generates data for an instruction, but an entire transaction.

I don't see this as viable and we should discuss this separately from the previous point. You're basically just making WasmSmartContract an InstructionBox variant. Logically, there will always be an entity Instruction and an entity representing a collection of instructions (Vec<InstructionBox> or commonly referred to as Transaction). I don't see the benefit of expression generating a collection of instructions instead of a single instruction. TBH I don't see much point in having InstructionBox::Sequence or InstructionBox::Pair since we can pack multiple instructions into a transaction

What we can consider doing is:

  1. move Executable::Wasm into InstructionBox and remove Executable
  2. make transaction generic over Executable variant. We would have Transaction<InstructionBox>/Transaction<WasmSmartContract>

@mversic
Copy link
Contributor

mversic commented Jun 15, 2023

potentially inline queries whose type is known ahead of time

but every query output type is known at compile time?

@Arjentix
Copy link
Contributor

Arjentix commented Dec 5, 2023

Done in #4089

Arjentix added a commit to Arjentix/iroha that referenced this issue Dec 13, 2023
Arjentix added a commit to Arjentix/iroha that referenced this issue Dec 14, 2023
Arjentix added a commit that referenced this issue Dec 14, 2023
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this issue Jan 9, 2024
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this issue Jan 22, 2024
@mversic mversic closed this as completed Jan 24, 2024
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this issue Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

No branches or pull requests

3 participants