Skip to content

Conversation

@Lucsanszky
Copy link
Contributor

@Lucsanszky Lucsanszky commented Jun 20, 2025

Description

Resolves #4963

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.
    NOTE: 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.
    NOTE: If bounds change 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).
  • CDDL files are up to date (use scripts/gen-cddl.sh)
  • hie.yaml updated (use scripts/gen-hie.sh).
  • Self-reviewed the diff.

@Lucsanszky Lucsanszky force-pushed the ldan/dijkstra-certs branch 3 times, most recently from 8e95681 to aaf4f14 Compare June 24, 2025 17:34
@Lucsanszky Lucsanszky force-pushed the ldan/dijkstra-certs branch from a4dc46d to f568416 Compare July 2, 2025 15:16
@Lucsanszky Lucsanszky force-pushed the ldan/dijkstra-certs branch from 44481c7 to 82c4848 Compare July 9, 2025 20:31
@Lucsanszky Lucsanszky force-pushed the ldan/dijkstra-certs branch 5 times, most recently from dcec4ee to b2300d1 Compare July 24, 2025 17:22
@Lucsanszky Lucsanszky force-pushed the ldan/dijkstra-certs branch 10 times, most recently from 44dc4d8 to 0ae255c Compare August 5, 2025 18:49
@Lucsanszky Lucsanszky force-pushed the ldan/dijkstra-certs branch 8 times, most recently from 8b93cb9 to dd8735f Compare August 13, 2025 14:05
@Lucsanszky Lucsanszky marked this pull request as ready for review August 13, 2025 14:37
@Lucsanszky Lucsanszky requested a review from a team as a code owner August 13, 2025 14:37
Lucsanszky and others added 20 commits September 3, 2025 19:52
It is not required by the `STS (ShelleyPOOL)` instance
* Decouple `ConwayEraTxCert` from `ShelleyEraTxCert`

* Remove `ShelleyEraTxCert` from `ShelleyEraImp` and `ConwayEraTxCert`
from `ConwayEraImp`
  These should not constrain `EraImp`s for the same
  reason they don't constrain `EraTest`s.
  See #4992

* Make some Conway `ImpTest` helpers era generic by only using
certificates with deposits
* Generalise compatible tests by using certificates with deposits
* Move tests that are reliant on certs w/out deposits and only run them pre-Dijkstra
* Move tests that are reliant on certs w/out deposits and only run them pre-Dijkstra
* Move tests that are reliant on certs w/out deposits and only run them pre-Dijkstra
Without this, users won't be able to change their
delegation without unregistration.

Also, re-organise some tests again and run them in each era
if they are general enough (i.e not dependent on Shelley style certs).

Co-authored-by: Alexey Kuleshevich <[email protected]>
Co-authored-by: Alexey Kuleshevich <[email protected]>
* Move tests that are reliant on certs w/out deposits and only run them pre-Dijkstra
Co-authored-by: Alexey Kuleshevich <[email protected]>
@Lucsanszky
Copy link
Contributor Author

@Lucsanszky Could you explain, for the benighted - what the deal is with EraSpecificSpec class ? I can see that each era accumulates the "era specific" ones from the previous ones, for alonzo: shelleyEraSpecificSpec >> alonzoEraSpecificSpec, for babbage: shelleyEraSpecificSpec >> alonzoEraSpecificSpec >> babbageEraSpecificSpec, for conway: shelleyEraSpecificSpe >> alonzoEraSpecificSpec >> babbageEraSpecificSpec >> conwayEraSpecificSpec

The raison d'etre of the EraSpecificSpec class is to allow us to conveniently select specs and tests to run per era. It is @lehins's brainchild and it was necessitated by deprecating certificates without deposits post-Conway. Pre-Conway, certificates didn't have deposits, Conway introduced certificates with deposits and post-Conway we will only allow certificates with deposits. This caused some calamity in our current Imp test setup, where in each era we also run the Imp tests from previous eras. My first iteration merely ripped out the parts that breaks post-Conway and made sure that we don't run certain tests in certain eras. However, this is not a well-organised solution and is prone to oversight in the long-run. This typeclass approach suggested by @lehins is clearer and it is also a well established pattern in our codebase.

Is the semantic of eraSpecificSpec: the tests in these functions should be used in future eras? So are they technically specific for all eras? How does this relate to how we are already including the Imp from previous eras when running Imp tests? (for example, Babbage Imp module calls AlonzoImp.spec @era when defining spec)

I must admit, EraSpecificSpec was a very poor choice of name on my part. Fortunately, @lehins is already cooking a PR which among other things will also address the unfortunate naming. To answer your question, the semantic is more along the lines of what I described above: you can select which tests (collected by the functions) to run for the given era, when providing an instance for that era. Essentially, we will be able to mix and match conveniently in an organised way from here on out. I think the best way to explain or demonstrate what this does is by comparing the EraSpecificSpec instance of ConwayEra and DijkstraEra. In case of the ConwayEra we run all the eraSpecificSpecs from previous eras, whereas in DijkstraEra we stop running those tests altogether, essentially saying: "Only run these tests up to Conway". However, it is worth noting that with the current setup, we could also say things like "Only run tests from Shelley and Alonzo". Surely enough, we could achieve this behaviour without creating a typeclass for it, but this typeclass solution yields better organisation. Not to mention, that the compiler will yell at us if we forget to provide an instance for an era.

And where is the eraSpecific method used? I can see it used in shelley Imp, but not anywhere else, but perhaps I'm just missing it.

Yes, because we only need to use it there. As you pointed out, each Imp spec calls the previous era's Imp spec. This, combined with the fact that we are providing EraSpecificSpec instances for all the eras yields that even when we are in Conway for example, eraSpecificSpec will be evaiuated but it will pick the implementation defined in the EraSpecificSpec ConwayEra instance, which in our case is:

  eraSpecificSpec =
    ShelleyImp.shelleyEraSpecificSpec
      >> AlonzoImp.alonzoEraSpecificSpec
      >> BabbageImp.babbageEraSpecificSpec
      >> conwayEraSpecificSpec

@Lucsanszky Lucsanszky merged commit c3d4f5d into master Sep 3, 2025
118 of 121 checks passed
@Lucsanszky Lucsanszky deleted the ldan/dijkstra-certs branch September 3, 2025 19:32
Lucsanszky added a commit that referenced this pull request Sep 29, 2025
@lehins lehins mentioned this pull request Oct 7, 2025
23 tasks
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.

Define Dijkstra certificates without the ones that have been deprecated

4 participants