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

Genesis API #3363

Closed
mversic opened this issue Apr 11, 2023 · 4 comments
Closed

Genesis API #3363

mversic opened this issue Apr 11, 2023 · 4 comments
Assignees
Labels
Chore This is a small task that can be done at any point in time and is easier than others iroha2-dev The re-implementation of a BFT hyperledger in RUST

Comments

@mversic
Copy link
Contributor

mversic commented Apr 11, 2023

          I'm not at all a fan of this API. I'd rather we had a similarly named method in two different traits. The user is left guessing as to what `<true>` means in this context. It was slightly better when we had an enum, but now I'm convinced we really need two trait extension methods and only one in scope.

Originally posted by @appetrosyan in #3356 (comment)

@mversic mversic added iroha2-dev The re-implementation of a BFT hyperledger in RUST Chore This is a small task that can be done at any point in time and is easier than others labels Apr 11, 2023
@Erigara
Copy link
Contributor

Erigara commented Apr 11, 2023

IMO we can use wrapper around function with const generic functions like:

#[inline]
fn validate<const IS_GENESIS: bool>() {
     // ...
}

fn validate_in_block() {
    validate::<false>()
}

fn validate_in_genesis() {
    validate::<true>()
}

I checked similar pattern once using godbolt and emitted assembly looks as we want (link)

@appetrosyan
Copy link
Contributor

appetrosyan commented Apr 11, 2023

The inline on a generic function is redundant. Also, the trait-based solution is arguably more descriptive, while allowing for idiomatic usage:

fn do_stuff_in_genesis( ... ) {
    use InGenesis as _;
    // ... 
    thing.validate(args);
}

fn do_stuff_in_both( ... ) {
    use InGenesis;
    use InBlock;

    // ... 
    <Thing as InGenesis>::validate(thing, args);
    <Thing as InBlock>::validate(thing, args);
}

and generating the same assembly as the const generic.

If at some point we need to change the semantics of validate in fn do_stuff_in_genesis all we need to do is import a different trait. In cases where we need to disambiguate, we can do that too, except now instead of wondering what <true> or <false> mean in this context, you know that you need to validate X in the gensis context, and something else in the regular block context.

Moreover, I doubt that we'd need to validate with genesis semantics outside the core crate, while other forms of validation might need to be exported. With a const generic, both functions are inline and you can't expose only one. With a trait, if the trait is private to a crate, you can only use it inside the crate.

@Erigara
Copy link
Contributor

Erigara commented Apr 11, 2023

Nice, i like this solution because we combine best of both worlds: DRY and descriptive behavior of the function.

@mversic
Copy link
Contributor Author

mversic commented Apr 11, 2023

There was this interesting comment by @appetrosyan on how we can extract genesis into a bootstrap process
I think we should explore all these options in the scope of this ticket

@ilchu ilchu self-assigned this May 15, 2023
ilchu added a commit to ilchu/iroha that referenced this issue May 18, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
ilchu added a commit to ilchu/iroha that referenced this issue May 18, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
ilchu added a commit to ilchu/iroha that referenced this issue May 19, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
ilchu added a commit to ilchu/iroha that referenced this issue May 19, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
ilchu added a commit to ilchu/iroha that referenced this issue May 19, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
ilchu added a commit to ilchu/iroha that referenced this issue May 19, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
mversic pushed a commit to ilchu/iroha that referenced this issue May 19, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
ilchu added a commit to ilchu/iroha that referenced this issue May 19, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
ilchu added a commit to ilchu/iroha that referenced this issue May 19, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
ilchu added a commit to ilchu/iroha that referenced this issue May 19, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
ilchu added a commit that referenced this issue May 19, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
@ilchu ilchu closed this as completed May 19, 2023
appetrosyan pushed a commit that referenced this issue Jun 5, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
mversic pushed a commit that referenced this issue Oct 17, 2023
Signed-off-by: Ilia Churin <churin.ilya@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Chore This is a small task that can be done at any point in time and is easier than others iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

No branches or pull requests

4 participants