Conversation
SeungheonOh
left a comment
SeungheonOh
left a comment
There was a problem hiding this comment.
I have one small comment. Rest seems good
kwxm
left a comment
kwxm
left a comment
There was a problem hiding this comment.
This looks OK to me. It reminded me of a few things that we should fix though.
There was a problem hiding this comment.
I don't think this code even works any more. It was used on the old script dump, which we don't have any more. I've made an issue to check it and remove it (although the results of the analysis may still be of some interest).
Your additions look correct though!
There was a problem hiding this comment.
This file can probably also go, but I'll look into that after this PR is merged.
| PlutusV1 -> alonzoPV | ||
| PlutusV2 -> vasilPV | ||
| PlutusV3 -> changPV | ||
| PlutusV4 -> dijkstraPV |
There was a problem hiding this comment.
Isn't Dijkstra the name of an era, not an HF? I think we've been using the names of the forks here, but we probably don't yet know the name of the fork that'll introduce Dijkstra. We might want to change this once we do.
There was a problem hiding this comment.
Also, do we know whether we'll be enabling the batch 7 builtins (multiIndexArray etc) at the Dijkstra HF, or just PlutusV4? If we are then some other things may need to be changed in this file.
| Map.fromList | ||
| [ | ||
| ( dijkstraPV | ||
| , Set.fromList (batch1 ++ batch2 ++ batch3 ++ batch4 ++ batch5 ++ batch6) |
There was a problem hiding this comment.
Do we know yet whether or not we'll be enabling the batch 7 builtins (multiIndexArray etc) at the Dijkstra HF, or just PlutusV4? If we are then this will have to be changed.
| V3.deserialiseScript changPV $ | ||
| errorScript <> "remdr3" | ||
| assertEqual "remdr4" (RemainderError "remdr4") $ | ||
| fromLeft (Prelude.error "Expected Reft, got Right") $ |
There was a problem hiding this comment.
| fromLeft (Prelude.error "Expected Reft, got Right") $ | |
| fromLeft (Prelude.error "Expected Left, got Right") $ |
| , mkTest changPV builtins1 | ||
| , mkTest plominPV builtins1 | ||
| , mkTest newestPV (allBuiltins \\ builtins7) | ||
| , mkTest vanRossemPV (allBuiltins \\ builtins7) |
There was a problem hiding this comment.
Good. We should probably have done this before, when van Rossem actually happened.
There was a problem hiding this comment.
This should all be simpler from now on, since all builtins are available for all ledger languages. I'll make an issue to check these tests and maybe simplify them.
| V3.deserialiseScript changPV $ | ||
| errorScript <> "remdr3" | ||
| assertEqual "remdr4" (RemainderError "remdr4") $ | ||
| fromLeft (Prelude.error "Expected Reft, got Right") $ |
There was a problem hiding this comment.
| fromLeft (Prelude.error "Expected Reft, got Right") $ | |
| fromLeft (Prelude.error "Expected Left, got Right") $ |
There was a problem hiding this comment.
We really should try to reduce the duplication between these tests and the Spec tests. There's an issue for that that's been open since February 2025, but there's always more important stuff to do.
| mCostModel :: MCostModel | ||
| mCostModel = | ||
| -- nothing to clear because v4 does not exist (yet). | ||
| -- nothing to clear yet because V4 currently has the same builtin as V3. |
There was a problem hiding this comment.
I think it always will, These tests need to be rethought: see #6089, open since May 2024.
3 participants
Most of the code mirrors the Plutus V3 counterpart.