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

Reduce constraint usage with eons #299

Merged

newhoggy merged 10 commits into main from newhoggy/reduce-constraint-usage-with-eons

Oct 9, 2023

Collaborator

newhoggy commented Oct 5, 2023 •

edited

Changelog

- description: |
    Reduce constraint usage with eons
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  - improvement    # QoL changes e.g. refactoring
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

By using eons to summon constraints our functions can have fewer or no constraints in their type signatures which simplifies code here as well as later in cardano-cli.

Checklist

Commit sequence broadly makes sense and commits have useful messages
The change log section in the PR description has been filled in
New tests are added if needed and existing tests are updated. These may include:
- golden tests
- property tests
- round trip tests
- integration tests
  See Running tests for more details
The version bounds in .cabal files are updated
CI passes. See note on CI. The following CI checks are required:
- Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
- Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
- Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
Self-reviewed the diff

newhoggy marked this pull request as ready for review

October 5, 2023 10:05

newhoggy requested review from dcoutts, Jimbo4350, erikd, carbolymer, CarlosLopezDeLara and disassembler as code owners

October 5, 2023 10:05

Jimbo4350 reviewed

View reviewed changes

cardano-api/internal/Cardano/Api/Fees.hs

+                          (toLedgerUTxO sbe utxo)
+                          txbody
+                  evalAdaOnly :: ShelleyToAllegraEra era -> TxOutValue era

Contributor

Jimbo4350 Oct 5, 2023

This has changed from ByronToAllegraEra. Was that intentional?

Collaborator Author

newhoggy Oct 5, 2023

Yes. The original code had ShelleyLedgerEra era in the constraints so you could not call it in the Byron era anyway.

newhoggy requested a review from Jimbo4350

October 5, 2023 22:11

newhoggy force-pushed the newhoggy/reduce-constraint-usage-with-eons branch from 9fe07d7 to fcae06e Compare

October 7, 2023 03:18

newhoggy commented

View reviewed changes

cardano-api/internal/Cardano/Api/TxBody.hs

+                  case txProtocolParams of
+                    BuildTxWith Nothing -> SNothing
+                    BuildTxWith (Just (LedgerProtocolParameters pp)) ->
+                      Alonzo.hashScriptIntegrity (Set.map (L.getLanguageView pp) languages) redeemers datums

Collaborator Author

newhoggy Oct 7, 2023

convPParamsToScriptIntegrityHash simplified a lot for three reasons:

The function was returning and Either when Left is never returned. This imposed on callers to handle the Left case as well, which is unnecessary.
The function is called with ShelleyBasedEra but the constraint makes it not possible for eras before alonzo anyway. Changing this to AlonzoEraOnwards means we don't have to case on those earlier eras that never happen.
alonzoEraOnwardsConstraints removes the need to handle the remaining cases.

newhoggy commented

View reviewed changes

cardano-api/internal/Cardano/Api/TxBody.hs

-                                 IsShelleyBasedEra era
-                              => L.AlonzoEraTxOut ledgerera
-                              => Ledger.EraCrypto ledgerera ~ StandardCrypto
-                              => Ledger.Value ledgerera ~ MaryValue StandardCrypto

Collaborator Author

newhoggy Oct 7, 2023

Removing constraints like these means the caller no longer needs to supply them.

newhoggy commented

View reviewed changes

cardano-api/internal/Cardano/Api/TxBody.hs

-                       <> CBOR.encCBOR (txScriptValidityToScriptValidity scriptValidity)
-                       <> CBOR.encodeNullMaybe CBOR.encCBOR txmetadata
-                    ShelleyBasedEraConway ->
-                      CBOR.serialize' (L.eraProtVerLow @L.Babbage)

Collaborator Author

newhoggy Oct 7, 2023

This looks like a bug. We say Babbage when doing Conway stuff.

Contributor

Jimbo4350 Oct 9, 2023

I think this was implemented when there was no Conway era available.

newhoggy commented

View reviewed changes

cardano-api/internal/Cardano/Api/TxBody.hs

+                    , CBOR.encodeNullMaybe CBOR.encCBOR txmetadata
+                    ]
+                  )
+                  (const $ CBOR.serialize' (L.eraProtVerLow @(ShelleyLedgerEra era)) $ mconcat

Collaborator Author

newhoggy Oct 7, 2023

Using the era parameter means we can't get this wrong:

https://github.com/input-output-hk/cardano-api/pull/299/files#r1349452875

Contributor

Jimbo4350 Oct 9, 2023

I think we can actually delete this function (or we should) because we no longer use the intermediate tx body format.

newhoggy commented

View reviewed changes

cardano-api/internal/Cardano/Api/TxBody.hs

+                            case sData of
+                              TxBodyNoScriptData -> SNothing
+                              TxBodyScriptData w datums redeemers ->
+                                convPParamsToScriptIntegrityHash w apiProtocolParameters redeemers datums languages

Collaborator Author

newhoggy Oct 7, 2023

Now that convPParamsToScriptIntegrityHash takes the more appropriate AlonzoEraOnwards witness (as opposed to ShelleyBasedEra we can make use of the witness supplied by TxBodyScriptData.

newhoggy commented

View reviewed changes

cardano-api/internal/Cardano/Api/TxBody.hs

-                => IsShelleyBasedEra era
-                => ShelleyLedgerEra era ~ ledgerera
-                => Ledger.EraCrypto ledgerera ~ StandardCrypto
-                => Ledger.Value ledgerera ~ MaryValue StandardCrypto

Collaborator Author

newhoggy Oct 7, 2023

Removing constraints relieves the caller of the burden of providing them.

newhoggy commented

View reviewed changes

cardano-api/internal/Cardano/Api/TxBody.hs

-                => Ledger.Value ledgerera ~ MaryValue StandardCrypto
-                => MaryEraOnwards era
-                -> AlonzoEraOnwards era
-                -> BabbageEraOnwards era

Collaborator Author

newhoggy Oct 7, 2023

We were previously demanding callers provide three witnesses. We only need the BabbageEraOnwards because that's the most strict and the others are redundant. Demanding fewer witness relieves the called of the burden of providing so many.

newhoggy force-pushed the newhoggy/reduce-constraint-usage-with-eons branch from fcae06e to 47847fb Compare

October 9, 2023 11:38

Jimbo4350 approved these changes

View reviewed changes

Contributor

Jimbo4350 left a comment

Nice 👍

cardano-api/internal/Cardano/Api/TxBody.hs

-                       <> CBOR.encCBOR (txScriptValidityToScriptValidity scriptValidity)
-                       <> CBOR.encodeNullMaybe CBOR.encCBOR txmetadata
-                    ShelleyBasedEraConway ->
-                      CBOR.serialize' (L.eraProtVerLow @L.Babbage)

Contributor

Jimbo4350 Oct 9, 2023

I think this was implemented when there was no Conway era available.

cardano-api/internal/Cardano/Api/TxBody.hs

+                    , CBOR.encodeNullMaybe CBOR.encCBOR txmetadata
+                    ]
+                  )
+                  (const $ CBOR.serialize' (L.eraProtVerLow @(ShelleyLedgerEra era)) $ mconcat

Contributor

Jimbo4350 Oct 9, 2023

I think we can actually delete this function (or we should) because we no longer use the intermediate tx body format.

newhoggy added 10 commits

October 10, 2023 00:33


          Reduce constraint usage with eons

b422319


          Remove constraints from serialiseShelleyBasedTxBody

9bda1c7


          Remove constraints from deserialiseShelleyBasedTxBody

034c4ca


          Remove constraints from fromAlonzoTxOut

7b68f18


          Remove constraints from fromAlonzoTxOut

857f155


          Remove constraints from fromBabbageTxOut

2ca4da7


          Remove constraints from convScriptData

5f46669


          Remove constraints from convPParamsToScriptIntegrityHash

2014b06


          Remove Either from return type of convPParamsToScriptIntegrityHash


          Remove constraints from mkCommonTxBody

11efd28

newhoggy force-pushed the newhoggy/reduce-constraint-usage-with-eons branch from 47847fb to 11efd28 Compare

October 9, 2023 13:47

newhoggy enabled auto-merge

October 9, 2023 13:49

newhoggy added this pull request to the merge queue

Merged via the queue into main with commit 9184dff

20 checks passed

newhoggy deleted the newhoggy/reduce-constraint-usage-with-eons branch

October 9, 2023 14:18

newhoggy pushed a commit that referenced this pull request


          #299 Fix 'MissingRedeemers' error when deregistering Plutus stake add…

99e3ae7

…ress

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

Reviewers

Jimbo4350 Jimbo4350 approved these changes

dcoutts Awaiting requested review from dcoutts dcoutts is a code owner

erikd Awaiting requested review from erikd erikd is a code owner

carbolymer Awaiting requested review from carbolymer carbolymer is a code owner

CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara CarlosLopezDeLara is a code owner

disassembler Awaiting requested review from disassembler disassembler is a code owner