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

GADT for TransactionScriptFailure and PlutusDebug #3167

Merged
merged 2 commits into from
Dec 15, 2022

Conversation

aniketd
Copy link
Contributor

@aniketd aniketd commented Nov 23, 2022

NOTE:

  • The first commit only renames all Plutus.* imports in a standard form of either PV1 (PlutusLedgerApi.V1), PV2 (PlutusLedgerApi.V2) or PCD (PlutusCore.Data)
  • Only the second commit implements all the changes related to making PlutusDebug a GADT.

TODO:

  • Get confirmation about CBOR codecs being correct
  • Add CHANGELOG entry

@aniketd aniketd force-pushed the aniketd/gadt-transactionscriptfailure-plutusdebug branch 6 times, most recently from af8429b to 3ea4e01 Compare December 2, 2022 08:47
@aniketd aniketd force-pushed the aniketd/gadt-transactionscriptfailure-plutusdebug branch from 3ea4e01 to 5f0e101 Compare December 2, 2022 08:58
@aniketd aniketd marked this pull request as ready for review December 2, 2022 08:59
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Few things need adjusting, but overall this is great stuff!!!

After this PR will probably be able to adopt this approach in other places as well, specific type that comes to mind is CostParams.

libs/cardano-ledger-core/src/Cardano/Ledger/Language.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Tools.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

This looks really great @aniketd , thanks for improving the type safety of our code!

@aniketd aniketd requested review from bezirg and lehins and removed request for bezirg December 7, 2022 18:02
@aniketd
Copy link
Contributor Author

aniketd commented Dec 9, 2022

NOTE: My current standing on this PR is that I'm uncertain only about the CBOR instances that have changed, and would like for someone to take a closer look at them. Besides those, the rest of the changes seem to be okay, and the failing tests are likely unrelated. The unexpected failure caused in an earlier CI run is also reproducible otherwise, so investigating that could be considered separately from this PR.

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

So it does look good, but the changes to CBOR encoding/decoding got broken.

It would be nice if you could add some roundtrip tests for the PlutusDebug serialization. At least it would give us some confidence that it is ok. You'll need to add a few Arbitrary instances here: Test.Cardano.Ledger.Alonzo.Serialisation.Generators for it to work.

libs/cardano-ledger-core/src/Cardano/Ledger/Language.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
@aniketd aniketd requested review from lehins and removed request for bezirg December 12, 2022 22:29
@aniketd
Copy link
Contributor Author

aniketd commented Dec 12, 2022

I'm going to clean up the commit history in preparation for the merge.

@aniketd aniketd force-pushed the aniketd/gadt-transactionscriptfailure-plutusdebug branch 3 times, most recently from fb997ca to 090ee6d Compare December 13, 2022 11:38
@aniketd
Copy link
Contributor Author

aniketd commented Dec 13, 2022

I have cleaned up the commit history and rebased on master. I think this is ready to be merged now. cc. @lehins 🙇 😅

@aniketd aniketd force-pushed the aniketd/gadt-transactionscriptfailure-plutusdebug branch from 090ee6d to cd2e285 Compare December 14, 2022 10:40
@aniketd aniketd force-pushed the aniketd/gadt-transactionscriptfailure-plutusdebug branch from cd2e285 to da7e3bb Compare December 15, 2022 08:42
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

This looks awesome!

eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxInfo.hs Outdated Show resolved Hide resolved
@aniketd aniketd force-pushed the aniketd/gadt-transactionscriptfailure-plutusdebug branch from da7e3bb to f050446 Compare December 15, 2022 13:03
@aniketd aniketd force-pushed the aniketd/gadt-transactionscriptfailure-plutusdebug branch from f050446 to d56253c Compare December 15, 2022 14:25
@aniketd aniketd merged commit a87ffef into master Dec 15, 2022
@iohk-bors iohk-bors bot deleted the aniketd/gadt-transactionscriptfailure-plutusdebug branch December 15, 2022 16:48
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.

5 participants