[Peras 27] Split degenerate BlockSupportsPeras instance#2027
[Peras 27] Split degenerate BlockSupportsPeras instance#2027agustinmista wants to merge 12 commits into
Conversation
821b00a to
37fe870
Compare
f47be9f to
b084253
Compare
37fe870 to
7fbebad
Compare
b084253 to
67855ea
Compare
7fbebad to
35a01bc
Compare
8a2bab8 to
10f8c2e
Compare
f827bf2 to
c4b4797
Compare
10f8c2e to
e8a284d
Compare
d5d1fd0 to
f7ecea3
Compare
e8a284d to
00df1fa
Compare
tbagrel1
left a comment
There was a problem hiding this comment.
This was a long review ahah
General comments (when I didn't find an appropriate location to pin them):
-
We still have
PerasVoterIdclashing withSeatIndex. Same withPoint blkandPerasBoostedBlock -
ShelleyBlock and HFC block are using Mock Vote/Cert despite real vote and certs being available (
Peras.{Vote,Cert}.V1) -
It also could be worth generating a list of modified files with a mention for each saying if we just updated imports/replaced constraints
StandardHash -> BlockSupportsPeras, or if we did something meaninful in them. It could be useful for future reviewers
| , Typeable blk | ||
| , Typeable (PerasVote blk) | ||
| , Typeable (PerasCert blk) | ||
| , IsPerasVote (PerasVote blk) blk | ||
| , IsPerasCert (PerasCert blk) blk | ||
| , Show (PerasVote blk) | ||
| , Show (PerasCert blk) | ||
| , Eq (PerasVote blk) | ||
| , Eq (PerasCert blk) | ||
| , NoThunks (PerasVote blk) | ||
| , NoThunks (PerasCert blk) |
There was a problem hiding this comment.
I suggested we could move the vote-related and cert-related constraints to the IsPerasVote and IsPerasCert classes. If there is a reason why this isn't desirable, we should probably document it :)
There was a problem hiding this comment.
Ok, I took another stab at this, and I think I have a decent argument to avoid doing this for now:
-
Contrary to what I expected, we end up needing more constraints than before. E.g.
StandardHash blkbecomes necessary on every call site of a vote/cert projection, regardless of whether we need toShow,Eq, etc. a vote/cert'sPoint blk. -
We additionally need the same constraints for
PerasError blk, but we don't have a dedicated class to add these as superclasses to, creating an inconsistency.
| ) => | ||
| BlockSupportsPeras blk | ||
| where | ||
| type PerasCfg blk |
There was a problem hiding this comment.
Mention #wontfix-for-testnet in the PR description
There was a problem hiding this comment.
#needs-a-todo(blk-dependent Peras params)
There was a problem hiding this comment.
| -- | ||
| -- Returns 'Nothing' if the block does not contain a Peras certificate, or | ||
| -- if the block is from an era that does not support Peras certificates. | ||
| getPerasCertInBlock :: |
There was a problem hiding this comment.
Mention #wontfix-for-testnet
There was a problem hiding this comment.
| -------------------------------------------------------------------------------} | ||
|
|
||
| -- NOTE: this is a mocked up implementation without crypto! | ||
|
|
There was a problem hiding this comment.
Again, we need to replace this with concrete Peras.Vote.V1 for >= Djisktra eras, or at least add a TODO that says we need to do it :)
There was a problem hiding this comment.
I added a TODO, also:
#needs-a-todo(use real Peras types for Shelley and HFC)
There was a problem hiding this comment.
So far I updated the bullet list in tweag/cardano-peras#73
I'll let @qnikst decide if we should create subtask with their dedicated issue
There was a problem hiding this comment.
@tbagrel1 item is ok for now, this way we will not lose it. We can spli to a dedicated issue in case if it can be done in later milestone or by another person.
f7ecea3 to
d90bebf
Compare
d90bebf to
5008d65
Compare
00df1fa to
fa3893c
Compare
5008d65 to
a0c11ba
Compare
fa3893c to
2b8a77f
Compare
a0c11ba to
6f1359d
Compare
2b8a77f to
966acfc
Compare
4308107 to
4197f46
Compare
7c8ef80 to
51d4a31
Compare
966acfc to
f63c166
Compare
51d4a31 to
3392b04
Compare
f63c166 to
45f5251
Compare
3392b04 to
662ef8e
Compare
Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io>
Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io>
Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io>
Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io>
Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io>
Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io>
662ef8e to
1c7aa1a
Compare
45f5251 to
a7118cf
Compare
…ostedBlock`" This reverts commit e2f6e65.
This PR has the main goal of removing the existing degenerate
forall blk. BlockSupportsPeras blkinstance and replace it with separate, independent ones for the several block types in the codebase that need it.To aid with this process, we have made a couple of preliminary changes:
O.C.Peras.Typesmodule to keepO.C.Block.SupportsPerasas focused on theblk-dependent API as possible.HasPeras(Vote|Cert)Xclasses into two,IsPerasVoteandIsPerasCert, grouping all the necessary non-crypto-related projections needed to operate on Peras votes and certificates.PerasCfg blkassociated type, in favor of keeping things simple and using the monomorphicPerasParamsfor now. These will likely need a major overhaul in the future when on-chain governance of these parameters becomes necessary.MockPeras(Vote|Cert)s, containing the non-crypto mocked Peras votes and certificates that were previously hardcoded everywhere. These will still be used for certain (mostly test) block types.BlockSupportsPerasinstance defined in terms ofVoid, so Peras votes, certs, and errors are uninstantiable and can be discharged viaabsurd.With this in place, the remaining changes mostly encompass the plumbing needed after removing the existing
BlockSupportsPerasinstance and replacing it with dedicated ones. Concretely:ByronBlockuses the "emtpy" instanceShelleyBlock proto erauses a mocked instance (to be replaced with a concrete one using real type forera ~ Dijkstraand empty ones for every otherera.(@tbagrel1 is working on this on a separate PR)HardForkBlock xsuses a mocked instance, but will be replaced with a compositional one defined around the two previous ones (also WIP)On the testing side:
BlockAandBlockBboth use the "empty" instanceDualByronBlockuses the "empty" instanceO.C.Mock.Ledger.Block.SimpleBlock c extuses the "empty" instanceO.C.Storage.TestBlock.TestBlockuses a mocked instance (now with a properly instantiatedgetPerasCertInBlock)Test.Util.TestBlock.TestBlockWith ptypeuses a mocked instance (without any support for certificates in blocks, as it's not needed for any of the existing tests)Finally, some general notes (notice that some of these are already covered by new issues linked in the comments below):
BlockSupportsPerasis still missing aforgePerasVotemethod. This will become evident soon when trying to write ablk-generic voting thread (BlockSupportsPerascurrently misses aforgeVotemethod tweag/cardano-peras#248)BlockSupportsPerasinstances are defined via orphans in dedicated modules. This is because:SerialiseNodeToNodeConstraints blknow requiresSerialiseNodeToNode (PerasVote blk)andSerialiseNodeToNode (PerasCert blk)which are only in scope afterPerasVote blkandPerasCert blkare established when definingBlockSupportsPeras blkSerialiseNodeToNodeConstraintsinstances are already defined as orphans in their own dedicated modules, forcing us to create a new module or to defineBlockSupportsPeras blkin the same one asSerialiseNodeToNodeConstraints blk(confusing!). We might be able to rearrange constraints to avoid this, but that would probably mean addingSerialiseNodeToNode (Peras(Vote|Cert) blk)as superclasses to a type-class higher-up in the chain.MockPerasVotecarries its own stake. Please note this is not the case for real votes, as their stake is retrieved from the stake distribution after validation.Peras...types will become redundant and get removed in a later PR when we start integrating components.