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

Support for Plutus V2 (ledger API) #2485

Merged
merged 1 commit into from
Oct 7, 2021
Merged

Conversation

JaredCorduan
Copy link
Contributor

@JaredCorduan JaredCorduan commented Sep 16, 2021

These changes add support for the second version of the Plutus ledger API (which now includes all the redeemers for a transaction inside the Plutus context). Using V2 Plutus scripts will result in a phase-one validation error, however, so long as there is no associated cost model.

Subsequent versions (V3, etc) should be simpler to add:

  • extend the Language enum and follow the GHC warnings.
  • for serialization, add a new key in the witness set (see the cddl).

The trace generation and the direct LEDGER property tests are now both using PlutusV2. There is also a unit test to ensure that not having a cost model for PlutusV2 results in a phase-one error.

@michaelpj
Copy link
Contributor

Notably missing from this PR is support for per language TxInfo, the information that is passed the Plutus. @michaelpj do you think that all the built in types will change (things like TxOutRef), or just TxInfo?

My plan is to re-export the exact same types for everything except TxInfo.

@JaredCorduan JaredCorduan force-pushed the jc/multiple-plutus-versions branch 3 times, most recently from 53507a0 to da6389f Compare September 17, 2021 17:15
@michaelpj
Copy link
Contributor

Okay Jared, you should be able to try this again with plutus master.

@JaredCorduan JaredCorduan marked this pull request as ready for review September 27, 2021 20:30
@JaredCorduan JaredCorduan changed the title Draft: Support for more than one version of Plutus Support for more than one version of Plutus Sep 27, 2021
@JaredCorduan JaredCorduan changed the title Support for more than one version of Plutus Support for Plutus V2 (ledger API) Sep 27, 2021
@JaredCorduan JaredCorduan force-pushed the jc/multiple-plutus-versions branch 2 times, most recently from 4ae834b to 43f4e3f Compare September 27, 2021 23:14
nc6
nc6 previously requested changes Sep 28, 2021
Copy link
Contributor

@nc6 nc6 left a comment

Choose a reason for hiding this comment

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

Few suggested changes, otherwise looks good!

eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Data.hs Outdated Show resolved Hide resolved
(Foldable.toList allScripts)
sortScripts (ts, v1, v2) s@(TimelockScript _) = (s : ts, v1, v2)
sortScripts (ts, v1, v2) s@(PlutusScript PlutusV1 _) = (ts, s : v1, v2)
sortScripts (ts, v1, v2) s@(PlutusScript PlutusV2 _) = (ts, v1, s : v2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a strong reason to serialise plutus scripts in segregated blocks in this way? I just imagine this will get tiresome when we add even more versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this was the original plan all along, but indeed this code is a bit messy, it wasn't pleasant to write...

I'm open to other ideas, but this change does need to be backwards compatible, and we do not currently have the language version in the script serialization.

eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Language.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Language.hs Outdated Show resolved Hide resolved
getLanguageView pp lang@PlutusV2 =
LangDepView
(serialize' lang)
(serializeEncoding' $ maybe encodeNull toCBOR $ Map.lookup lang (_costmdls pp))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect this implementation to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not think we could assume that every language view would contain the same data. And I wanted to make sure that GHC would at least make us think about each new language.

@@ -47,7 +56,7 @@ data ScriptFailure c
| -- | Missing datum.
MissingDatum (DataHash c)
| -- | Plutus evaluation error.
ValidationFailed P.EvaluationError
ValidationFailed PV1.EvaluationError [Text]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is rather confusing - what happens to Plutus V2 evaluation errors? I guess the key is that presently these types are the same, but it would be good to clarify this at least in the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to make it very mechanical, then I think we should have different errors here. Or an error GADT indexed by the language enum? :trollface:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

@@ -12,6 +12,7 @@
-- cd into the plutus-preprocessor directory and type 'cabal run'
module Main where

import Cardano.Ledger.Alonzo.Language (Language (..))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether this should live elsewhere. But seems reasonable for now!

@@ -112,12 +113,12 @@ instance NoThunks Tag
-- | Scripts in the Alonzo Era, Either a Timelock script or a Plutus script.
data Script era
= TimelockScript (Timelock (Crypto era))
| PlutusScript ShortByteString -- A Plutus.V1.Ledger.Scripts(Script) that has been 'CBOR' encoded
| PlutusScript Language ShortByteString
Copy link
Contributor

Choose a reason for hiding this comment

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

Hum. I guess I would have expected either:

data Script = TimelockScript ... | PlutusV1Script ... | PlutusV2Script ...

or

data Script = AScript Language ....

or maybe even

data Script = TimeLockScript ... | NonNativeScript Language ...

IDK, there seems like there's maybe still a bit too much "plutus" specificity (if we're trying to be a bit more agnostic about what languages there are).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to cross that bridge when we got there, I don't really know how to anticipate what shape other languages will have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, I'm open to suggestions if you have a favorite!

@@ -294,7 +303,8 @@ instance forall era. (Typeable (Crypto era), Typeable era) => ToCBOR (Script era

encodeScript :: (Typeable (Crypto era)) => Script era -> Encode 'Open (Script era)
encodeScript (TimelockScript i) = Sum TimelockScript 0 !> To i
encodeScript (PlutusScript s) = Sum PlutusScript 1 !> To s -- Use the ToCBOR instance of ShortByteString
encodeScript (PlutusScript PlutusV1 s) = Sum (PlutusScript PlutusV1) 1 !> To s -- Use the ToCBOR instance of ShortByteString
Copy link
Contributor

Choose a reason for hiding this comment

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

huh, I guess I would have expected this to use the ToCBOR instance for Language...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

I have no idea what I'm talking about, feel free to ignore me.

@nc6 nc6 dismissed their stale review September 30, 2021 14:13

Comments addressed!

@JaredCorduan JaredCorduan merged commit 8ef01f2 into master Oct 7, 2021
@iohk-bors iohk-bors bot deleted the jc/multiple-plutus-versions branch October 7, 2021 21:18
}
deriving (Eq, Generic, Show)
-- It is deliberate that there is no Ord instance, use `pointWiseExUnits` instead.
deriving
(BoundedMeasure, Measure)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@JaredCorduan JaredCorduan Oct 9, 2021

Choose a reason for hiding this comment

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

We were waffling about how best to make this instance, so to move this PR along I removed the instance. This PR adds it back in, with one proposed solution: #2515

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thank you for the reference!

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.

4 participants