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

Abstract over the various data types needed by the consensus engine #8

Merged
merged 9 commits into from
Oct 25, 2023

Conversation

romac
Copy link
Member

@romac romac commented Oct 23, 2023

Closes: #11

This gist of this PR is that it adds a Consensus trait which defines the various datatypes that the consensus engine operates over, as well as a trait per such datatype that defines the requirement for said datatype.

pub trait Consensus
where
    Self: Sized,
{
    type Address: Address;
    type Height: Height;
    type Proposal: Proposal<Self>;
    type PublicKey: PublicKey;
    type Validator: Validator<Self>;
    type ValidatorSet: ValidatorSet<Self>;
    type Value: Value;
    type Vote: Vote<Self>;
pub trait Height
where
    Self: Clone + Debug + PartialEq + Eq + PartialOrd + Ord,
{
}

etc.

In test code, we define our own simple versions of these datatypes as well as a TestConsensus struct, for which we implement the Consensus trait.

#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub struct Address(u64);

impl Address {
    pub const fn new(value: u64) -> Self {
        Self(value)
    }
}

impl malachite_common::Address for Address {}
pub struct TestConsensus;

impl Consensus for TestConsensus {
    type Address = Address;
    type Height = Height;
    type Proposal = Proposal;
    type PublicKey = PublicKey;
    type ValidatorSet = ValidatorSet;
    type Validator = Validator;
    type Value = Value;
    type Vote = Vote;
}

@romac romac force-pushed the romac/data-generic branch 2 times, most recently from 796f6d5 to 4e36931 Compare October 23, 2023 15:50
@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #8 (5909cf1) into romac/rust-state-machine (ccc12e6) will decrease coverage by 2.48%.
The diff coverage is 88.14%.

@@                     Coverage Diff                      @@
##           romac/rust-state-machine       #8      +/-   ##
============================================================
- Coverage                     85.60%   83.12%   -2.48%     
============================================================
  Files                            15       17       +2     
  Lines                          1007      794     -213     
============================================================
- Hits                            862      660     -202     
+ Misses                          145      134      -11     
Files Coverage Δ
Code/common/src/vote.rs 100.00% <ø> (ø)
Code/round/src/events.rs 100.00% <ø> (ø)
Code/round/src/state.rs 98.39% <100.00%> (+0.31%) ⬆️
Code/test/src/proposal.rs 100.00% <100.00%> (ø)
Code/test/src/vote.rs 100.00% <100.00%> (ø)
Code/vote/src/lib.rs 92.86% <100.00%> (ø)
Code/common/src/round.rs 92.00% <90.91%> (+10.00%) ⬆️
Code/test/src/consensus.rs 90.00% <90.00%> (ø)
Code/vote/src/count.rs 80.65% <90.00%> (-9.60%) ⬇️
Code/vote/src/keeper.rs 65.91% <80.00%> (-21.01%) ⬇️
... and 7 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@romac romac changed the title Abstract over data types Abstract over the various data types needed by the consensus engine Oct 23, 2023
Base automatically changed from anca/propose_checks to romac/consensus October 24, 2023 07:25
@romac romac force-pushed the romac/data-generic branch from 4e36931 to 94c8651 Compare October 24, 2023 07:28
@romac romac force-pushed the romac/data-generic branch 2 times, most recently from a9af4d9 to 1d0ec33 Compare October 24, 2023 08:00
Base automatically changed from romac/consensus to romac/rust-state-machine October 24, 2023 13:44
romac added 4 commits October 24, 2023 15:50
* Move tests and associated data types into their own crate

* Add small threshold to codecov

* Small cleanup

* Move tests into `tests` folder

* Adjust base when computing coverage in the presence of removed code

See: https://docs.codecov.com/docs/commit-status#removed_code_behavior
@romac romac force-pushed the romac/data-generic branch from 60c20f6 to 59e04b3 Compare October 24, 2023 13:50
@romac
Copy link
Member Author

romac commented Oct 24, 2023

I have also been experimenting with finer-grained generics instead of stuffing everything into a single Consensus trait, but it’s getting very convoluted and pretty hairy.

It’s possible that I am going at this the wrong way but so far I would be in favor of keeping the Consensus trait rather than have finer-grained generics.

@romac romac marked this pull request as ready for review October 24, 2023 14:05
@romac
Copy link
Member Author

romac commented Oct 24, 2023

Drop in coverage is likely because a bunch of code was moved to the test crate, ie. deleted from the main crates and added to the test crate.

@romac romac force-pushed the romac/data-generic branch from d8b4928 to ddd0d8d Compare October 24, 2023 16:09
@romac romac requested a review from ancazamfir October 24, 2023 18:13
@romac romac merged commit 3fea113 into romac/rust-state-machine Oct 25, 2023
7 checks passed
@romac romac deleted the romac/data-generic branch October 25, 2023 07:22
romac added a commit that referenced this pull request Nov 8, 2023
Closes: #11

This gist of this PR is that it adds a `Consensus` trait which defines the various datatypes that the consensus engine operates over, as well as a trait per such datatype that defines the requirement for said datatype.

```rust
pub trait Consensus
where
    Self: Sized,
{
    type Address: Address;
    type Height: Height;
    type Proposal: Proposal<Self>;
    type PublicKey: PublicKey;
    type Validator: Validator<Self>;
    type ValidatorSet: ValidatorSet<Self>;
    type Value: Value;
    type Vote: Vote<Self>;
```

```rust
pub trait Height
where
    Self: Clone + Debug + PartialEq + Eq + PartialOrd + Ord,
{
}
```

etc.

In test code, we define our own simple versions of these datatypes as well as a `TestConsensus` struct, for which we implement the `Consensus` trait.


```rust
#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub struct Address(u64);

impl Address {
    pub const fn new(value: u64) -> Self {
        Self(value)
    }
}

impl malachite_common::Address for Address {}
```

```rust
pub struct TestConsensus;

impl Consensus for TestConsensus {
    type Address = Address;
    type Height = Height;
    type Proposal = Proposal;
    type PublicKey = PublicKey;
    type ValidatorSet = ValidatorSet;
    type Validator = Validator;
    type Value = Value;
    type Vote = Vote;
}
```

---

* Attempt to abstract over the various data types needed by the consensus engine

* Fix tests

* Fix lints names

* Move tests and associated data types into their own crate (#10)

* Move tests and associated data types into their own crate

* Add small threshold to codecov

* Small cleanup

* Move tests into `tests` folder

* Adjust base when computing coverage in the presence of removed code

See: https://docs.codecov.com/docs/commit-status#removed_code_behavior

* Document the `Round` type and rename `Round::None` to `Round::Nil`

* More doc comments

* Remove `PublicKey::hash` requirement

* Few more comments

* Add propose+precommit timeout test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant