Define interfaces for consensus #1923
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Draft because this depends on #1902. |
mrBliss
force-pushed
the
mrBliss/era-translations
branch
2 times, most recently
from
df5427e
to
9d16f70
Compare
mrBliss
force-pushed
the
mrBliss/consensus-interfaces
branch
from
2cd6fed
to
0c5f00d
Compare
mrBliss
force-pushed
the
mrBliss/consensus-interfaces
branch
from
0c5f00d
to
1e3f617
Compare
|
#1902 has been merged. I have rebased this PR on master. Ready for review. |
mrBliss
commented
Oct 21, 2020
Comment on lines
53
to
73
| ( Eq (Block era), | ||
| Show (Block era), | ||
| NoThunks (Block era), | ||
| FromCBOR (Annotator (Block era)), | ||
| ToCBOR (Block era), | ||
| Eq (BHeader (Crypto era)), | ||
| Show (BHeader (Crypto era)), | ||
| NoThunks (BHeader (Crypto era)), | ||
| FromCBOR (Annotator (BHeader (Crypto era))), | ||
| ToCBOR (Block era), | ||
| Eq (ShelleyState era), | ||
| Show (ShelleyState era), | ||
| NoThunks (ShelleyState era), | ||
| FromCBOR (ShelleyState era), | ||
| ToCBOR (ShelleyState era), | ||
| Eq (BlockTransitionError era), | ||
| Show (BlockTransitionError era), | ||
| NoThunks (BlockTransitionError era), | ||
| Eq (STS.PredicateFailure (STS.CHAIN era)), | ||
| Show (STS.PredicateFailure (STS.CHAIN era)), | ||
| NoThunks (STS.PredicateFailure (STS.CHAIN era)) |
There was a problem hiding this comment.
This list is already rather long, but I expect some things are missing. So at some point, I might have to add more constraints here.
nc6
approved these changes
Oct 21, 2020
| import Shelley.Spec.Ledger.TxBody (EraIndependentTxBody) | ||
|
|
||
| -- TODO #1304: add reapplyTxs | ||
| class |
There was a problem hiding this comment.
I might move these class definitions to Cardano.Ledger.API since we now expect them to apply across eras. But can do that in a separate PR
There was a problem hiding this comment.
We decided to postpone this.
| Show (Tx era), | ||
| NoThunks (Tx era), | ||
| FromCBOR (Annotator (Tx era)), | ||
| ToCBOR (Tx era), |
There was a problem hiding this comment.
Tim has proposed packaging these things up as ChainData, which would make this list somewhat cleaner.
There was a problem hiding this comment.
See the last commit.
| -- few header level checks, such as size constraints. | ||
| applyTick :: | ||
| Globals -> | ||
| ShelleyState era -> |
There was a problem hiding this comment.
[bikeshedding] ShelleyState was an alias for downstream use, but now feels a bit awkward, since generally this won't be shelley. We could use the wrapped type NewEpochState, or come up with a better name (but all the obvious ones are taken...)
There was a problem hiding this comment.
I have gone for NewEpochState for now.
mrBliss
force-pushed
the
mrBliss/consensus-interfaces
branch
from
1e3f617
to
40610c5
Compare
In the `Shelley.Spec.Ledger.API.*` modules, define type classes for the most important functionality consensus. These classes are parametric in the era. Instances should be provided for each era (Shelley, Allegra, Mary, etc.). At the moment, only instances for `ShelleyEra` are provided. These classes list a bunch of super-class constraints that consensus relies on. I have tried to be complete, but there's a good chance I forgot to add a few constraints that are currently already satisfied. Default implementations are provided for each method so that an empty instance declaration should be enough for each class + era combination. The constraints on these instances are what matters, they should be minimal, e.g., just `Crypto crypto` and `DSignable ..`. Note: the super-class constraints of the classes are what consensus needs, *not* what the default implementations of the methods need. If a default implementation needs more constraints, add them to the *default signature*.
It will be used by all eras, so having Shelley in the name is confusing.
mrBliss
force-pushed
the
mrBliss/consensus-interfaces
branch
from
40610c5
to
6cb87f9
Compare
These constraint synonyms make the contexts of the API classes much more readable.
mrBliss
force-pushed
the
mrBliss/consensus-interfaces
branch
from
6cb87f9
to
a0d58d3
Compare
nc6
approved these changes
Oct 21, 2020
mrBliss
added a commit
to IntersectMBO/ouroboros-network
that referenced
this pull request
Oct 22, 2020
mrBliss
added a commit
to IntersectMBO/ouroboros-network
that referenced
this pull request
Oct 23, 2020
iohk-bors bot
added a commit
to IntersectMBO/ouroboros-network
that referenced
this pull request
Oct 23, 2020
2702: Update dependencies r=mrBliss a=mrBliss Highlights: * IntersectMBO/cardano-ledger#1915 * IntersectMBO/cardano-ledger#1922 * IntersectMBO/cardano-ledger#1902 * IntersectMBO/cardano-ledger#1923 * IntersectMBO/cardano-ledger#1927 * IntersectMBO/cardano-ledger#1929 Co-authored-by: Thomas Winant <thomas@well-typed.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the
Shelley.Spec.Ledger.API.*modules, define type classes for the mostimportant functionality consensus. These classes are parametric in the era.
Instances should be provided for each era (Shelley, Allegra, Mary, etc.). At the
moment, only instances for
ShelleyEraare provided.These classes list a bunch of super-class constraints that consensus relies on.
I have tried to be complete, but there's a good chance I forgot to add a few
constraints that are currently already satisfied.
Default implementations are provided for each method so that an empty instance
declaration should be enough for each class + era combination. The constraints
on these instances are what matters, they should be minimal, e.g., just
Crypto cryptoandDSignable ...Note: the super-class constraints of the classes are what consensus needs, not
what the default implementations of the methods need. If a default
implementation needs more constraints, add them to the default signature.