Skip to content

Improve ImpTest helpers #5220

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Improve ImpTest helpers #5220

wants to merge 11 commits into from

Conversation

Lucsanszky
Copy link
Contributor

@Lucsanszky Lucsanszky commented Aug 11, 2025 •
edited
Loading

Description

Follow-up to #5137

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 changed the base branch from master to ldan/dijkstra-certs August 11, 2025 17:14
@Lucsanszky Lucsanszky changed the base branch from ldan/dijkstra-certs to master August 11, 2025 17:15
@Lucsanszky Lucsanszky mentioned this pull request Aug 27, 2025
Merged
10 tasks
@Lucsanszky Lucsanszky force-pushed the ldan/imptest-improvements branch 4 times, most recently from 2997716 to a5d8109 Compare September 5, 2025 10:17
@Lucsanszky Lucsanszky force-pushed the ldan/imptest-improvements branch from c7f0287 to 8155e7c Compare September 9, 2025 16:48
@Lucsanszky Lucsanszky force-pushed the ldan/imptest-improvements branch 3 times, most recently from 4dffd56 to 4bbc441 Compare September 17, 2025 22:51
@Lucsanszky Lucsanszky marked this pull request as ready for review September 17, 2025 22:52
@Lucsanszky Lucsanszky requested a review from a team as a code owner September 17, 2025 22:52
@Lucsanszky Lucsanszky force-pushed the ldan/imptest-improvements branch from 4bbc441 to ca9c96d Compare September 17, 2025 22:56
@Lucsanszky
Make genRegTxCert a method of ShelleyEraImp
8f4845e
@Lucsanszky
Make genUnRegTxCert a method of ShelleyEraImp
e4cff67
@Lucsanszky
Remove registerStakeCredentialWithDeposit
1a49345
@Lucsanszky
Remove registerPoolWithDeposit
c766965
@Lucsanszky
Remove registerRewardAccountWithDeposit
41a7c8a
@Lucsanszky
Use certs w/out deposits in some script validation tests
d052c57 
For some reason, these tests fail if we use certificates with deposits,
so as a temporary measure we will avoid using deposits in these cases.

Related: #4571
@Lucsanszky
Remove shelleyEraSpecificSpec
2519b08 
No longer necessary since the introduction of `genRegTxCert`,
`genUnRegTxCert` `ShelleyEraImp` typeclass methods.
@Lucsanszky
Remove regDelegToDRep helper
ad6852d 
This reverts commit 9a06817
and related changes.
@Lucsanszky
Move some alonzoEraSpecificSpec tests
eac5e2c
@Lucsanszky
Remove babbageEraSpecificSpec
3e31688 
No longer necessary since the introduction of `genRegTxCert`,
`genUnRegTxCert` `ShelleyEraImp` typeclass methods.
@Lucsanszky
Move a conwayEraSpecificSpec testcase
318a7dc
@Lucsanszky Lucsanszky force-pushed the ldan/imptest-improvements branch from ca9c96d to 318a7dc Compare September 19, 2025 12:26
@Lucsanszky Lucsanszky mentioned this pull request Sep 19, 2025
Open
10 tasks
teodanciu
teodanciu approved these changes Sep 23, 2025
View reviewed changes
Copy link
Contributor

@teodanciu teodanciu left a comment

Choose a reason for hiding this comment

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

A few comments from me.
I love it, it's so cool how introducing these two methods genRegTxCert and genUnRegTxCert made it possible to remove all these variations of functions

Looks good to me, barring some version changes and changelog suggestions.

eras/shelley/impl/CHANGELOG.md
* Added `shelleyGenUnRegTxCert`
* Added `genUnRegTxCert` to `ShelleyEraImp`
* Added `shelleyGenRegTxCert`
* Added `genRegTxCert` to `ShelleyEraImp`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should increase the version in cabal for this to 1.17.1.0 probably, and also move the changelong in the corresponding section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, nice catch because it seems like there was a release for 1.17.0.0, however the changelog was not bumped for some reason as you pointed out here: #5286
@lehins, did you use the bump-changelogs.sh script? Because if so, then it seems like it's buggy as it missed some bumps.

Copy link
Contributor

@teodanciu teodanciu Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In that PR that you mean, I added a commit containing some, but not all fixes , so - in hindsight - that might have been misleading.
I made now a PR to correct all problems, since this is an issue now across several PRs: #5307

eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Comment on lines +511 to +513
genRegTxCert :: HasCallStack => Credential 'Staking -> ImpTestM era (TxCert era)

genUnRegTxCert :: HasCallStack => Credential 'Staking -> ImpTestM era (TxCert era)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why HasCallStack here? Can these fail?

eras/dijkstra/impl/testlib/Test/Cardano/Ledger/Dijkstra/ImpTest.hs
module Test.Cardano.Ledger.Conway.ImpTest,
exampleDijkstraGenesis,
DijkstraEraImp,
dijkstraGenRegTxCert,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe expose also dijkstraGenUnRegTxCert, for consistency? otherwise, don't expose either - as they're not yet needed?
If you expose them, you should update the version to 0.2.1.0 and add changelog to the corresponding section.

eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Imp/LedgerSpec.hs
let cred = ScriptHashObj scriptHash
void $ regDelegToDRep cred (Coin 1_000_000) DRepAlwaysAbstain
ra <- getRewardAccountFor cred
ra <- registerStakeCredential cred
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also call delegateToDRep here, in order to preserve the logic of the test?

Suggested change
ra <- registerStakeCredential cred
ra <- registerStakeCredential cred
kh <- freshKeyHash
_ <- delegateToDRep (KeyHashObj kh) (Coin 1_000_000) DRepAlwaysAbstain

eras/shelley/impl/CHANGELOG.md

### `testlib`

* Added `shelleyGenUnRegTxCert`
Copy link
Contributor

Choose a reason for hiding this comment

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

You could mention removing shelleyEraSpecificSpec

Suggested change
* Added `shelleyGenUnRegTxCert`
* Remove shelleyEraSpecificSpec
* Added `shelleyGenUnRegTxCert`

eras/babbage/impl/testlib/Test/Cardano/Ledger/Babbage/Imp.hs
{-# OPTIONS_GHC -Wno-orphans #-}

module Test.Cardano.Ledger.Babbage.Imp (spec, babbageEraSpecificSpec) where
module Test.Cardano.Ledger.Babbage.Imp (spec) where
Copy link
Contributor

Choose a reason for hiding this comment

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

You could mention this change in CHANGELOG, and probably increment the version of babbage package to 1.12.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teodanciu teodanciu teodanciu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Lucsanszky @teodanciu