-
Notifications
You must be signed in to change notification settings - Fork 158
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
Conversation
My plan is to re-export the exact same types for everything except |
53507a0
to
da6389f
Compare
Okay Jared, you should be able to try this again with plutus master. |
da6389f
to
aa62640
Compare
aa62640
to
107b664
Compare
4ae834b
to
43f4e3f
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.
Few suggested changes, otherwise looks good!
(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) |
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.
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.
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.
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.
getLanguageView pp lang@PlutusV2 = | ||
LangDepView | ||
(serialize' lang) | ||
(serializeEncoding' $ maybe encodeNull toCBOR $ Map.lookup lang (_costmdls pp)) |
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.
Do we expect this implementation to change?
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.
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] |
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.
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.
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.
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?
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.
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 (..)) |
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.
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 |
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.
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).
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.
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.
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.
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 |
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.
huh, I guess I would have expected this to use the ToCBOR
instance for Language...
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.
I know, it's a bit awkward. See https://github.com/input-output-hk/cardano-ledger-specs/pull/2485/files#r717306756
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.
I have no idea what I'm talking about, feel free to ignore me.
43f4e3f
to
2946ff8
Compare
2946ff8
to
781e2c0
Compare
} | ||
deriving (Eq, Generic, Show) | ||
-- It is deliberate that there is no Ord instance, use `pointWiseExUnits` instead. | ||
deriving | ||
(BoundedMeasure, Measure) |
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.
@JaredCorduan May I ask why was this removed? AlonzoMeasure
in ouroboros-network
wants it:
https://github.com/input-output-hk/ouroboros-network/blob/e0ccbb73296027a5e5fd224a78374321974758ce/ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Mempool.hs#L314
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.
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
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.
Awesome, thank you for the reference!
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:Language
enum and follow the GHC warnings.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 forPlutusV2
results in a phase-one error.