-
Notifications
You must be signed in to change notification settings - Fork 156
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 Conformance.Imp: imptests with conformance #4748
Conversation
64aa141
to
a2c1460
Compare
6f49a26
to
f51847a
Compare
d72129e
to
d7c9778
Compare
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
libs/cardano-ledger-conformance/test/Test/Cardano/Ledger/Conformance/Imp.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-conformance/test/Test/Cardano/Ledger/Conformance/Imp.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but a few minor changes are still needed before we can merge it
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-conformance/test/Test/Cardano/Ledger/Conformance/Imp.hs
Outdated
Show resolved
Hide resolved
c8d3e8b
to
5065734
Compare
With a new `iteExpectLedgerRuleConformance` field added to `ImpTestEnv`, we get an overridable function that can be executed within `trySubmitTx` for every submitted transaction. To override this hook we have `modifyImpInitExpectLedgerRuleConformance` the function. We add a new `Conformance.Imp` module to `cardano-ledger-conformance` package, to import `Conway.Imp` and run those tests with a modified "hook" that runs the `LEDGER` rule from Agda on the Tx and `checkConformance` on the results. Additions: - `iteExpectLedgerRuleConformance` field to `ImpTestEnv`. - `modifyImpInitExpectLedgerRuleConformance` function to override the hook. - Add lenses for all fields of `LedgerEnv`. Changes: - Change `trySubmitTx` to run the `iteExpectLedgerRuleConformance` for every submitted Tx.
5065734
to
ec616f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. I've made couple suggestions, but neither one is a blocking suggestion, so in order not to block the follow up work I am gonna resolve them and merge the PR.
Description
With a new
iteExpectLedgerRuleConformance
field added toImpTestEnv
,we get an overridable function that can be executed within
trySubmitTx
for every submitted transaction. To override this hook we have
modifyImpInitExpectLedgerRuleConformance
the function. We add a newConformance.Imp
module tocardano-ledger-conformance
package, toimport
Conway.Imp
and run those tests with a modified "hook" thatruns the
LEDGER
rule from Agda on the Tx andcheckConformance
onthe results.
Additions:
iteExpectLedgerRuleConformance
field toImpTestEnv
.modifyImpInitExpectLedgerRuleConformance
function to override thehook.
LedgerEnv
.Changes:
trySubmitTx
to run theiteExpectLedgerRuleConformance
forevery submitted Tx.
The Problem
We get a failure from the Agda side, telling us that the
witsVKeyNeeded utxo txb
is not a subset ofwitsKeyHashes
as it should be. We have isolated the issue to be with the translation ofspendableInputs
, since it is most likely that those aren't being handled in a manner that fits into the problem we are trying to solve with this conformance.This PR can be merged as it is, since @Soupstraw and I want to work in parallel on related issues.
Closes #4725
Checklist
CHANGELOG.md
for the affected packages.New section is never added with the code changes. (See RELEASING.md)
.cabal
andCHANGELOG.md
files according to theversioning process.
.cabal
files for all affected packages are updated.If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
fourmolu
(usescripts/fourmolize.sh
)scripts/cabal-format.sh
)hie.yaml
has been updated (usescripts/gen-hie.sh
)