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

Add cbor round trip tests for Block for each era #4869

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

teodanciu
Copy link
Contributor

Description

While looking into roundtrip tests, I discovered we don't have any for Block.

I have added such tests for each era to cardano-protocol-tpraos.
Since Block is defined in core, the tests would be better suited in core, and then referenced and ran in each, but the problem is the reference to BHeader which is defined in cardano-protocol-tpraos.
Is there something else I can use instead of BHeader ? I can see there is BHeaderView, but it doesn't seem to be suitable for this kind of testing.

Another note: the cddlRoundTrip specs don't pass for Block, because of the checks we make in the TxSeq DecCBOR instances - the data we're generating from cddl is not internally consistent to pass these checks.

Checklist

  • Commits in meaningful sequence and with useful messages
  • Tests added or updated when needed
  • CHANGELOG.md files updated for packages with externally visible changes

    New section is never added with the code changes. (See RELEASING.md)
  • Versions updated in .cabal and CHANGELOG.md files when necessary, according to the
    versioning process.
  • Version bounds in .cabal files updated when necessary

    If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • Code formatted (use scripts/fourmolize.sh)
  • Cabal files formatted (use scripts/cabal-format.sh)
  • hie.yaml updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@teodanciu teodanciu requested a review from a team as a code owner February 3, 2025 17:38
@teodanciu teodanciu changed the title Add cbor RoundTrip tests for blocks for each era Add cbor round trip tests for Block` for each era Feb 3, 2025
@teodanciu teodanciu force-pushed the td/add-missing-block-roundtrip-tests branch from 23d176c to af18f12 Compare February 3, 2025 17:41
Comment on lines +37 to +46
roundTripBlock ::
forall era.
( EraSegWits era
, Arbitrary (Tx era)
) =>
Spec
roundTripBlock =
prop (show (typeRep $ Proxy @(Block (BHeader StandardCrypto) era))) $
withMaxSuccess 3 $
roundTripAnnEraExpectation @era @(Block (BHeader StandardCrypto) era)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to add this funciton to the testlib, so it can be used in consensus as well. For this it needs to be parameterized by block header

Suggested change
roundTripBlock ::
forall era.
( EraSegWits era
, Arbitrary (Tx era)
) =>
Spec
roundTripBlock =
prop (show (typeRep $ Proxy @(Block (BHeader StandardCrypto) era))) $
withMaxSuccess 3 $
roundTripAnnEraExpectation @era @(Block (BHeader StandardCrypto) era)
roundTripBlock ::
forall era bh.
( EraSegWits era
, Arbitrary (Tx era)
) =>
Spec
roundTripBlock =
prop (show (typeRep $ Proxy @(Block bh era))) $
withMaxSuccess 3 $
roundTripAnnEraExpectation @era @(Block bh era)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! good point, thanks.

Comment on lines +34 to +35
roundTripBlock @BabbageEra
roundTripBlock @ConwayEra
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These do not make sense, because block header as defined in this package is never used with Babbage onwards

Suggested change
roundTripBlock @BabbageEra
roundTripBlock @ConwayEra

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wondered about whether it brings any value at all like this, but I agree, no point testing unrealistic stuff.
I will try adding for TxSeq instead!

@lehins lehins changed the title Add cbor round trip tests for Block` for each era Add cbor round trip tests for Block for each era Feb 6, 2025
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.

2 participants