Conversation
berewt
left a comment
berewt
left a comment
There was a problem hiding this comment.
No blocker. The major concern I have is about the organisation of PSU and Ledger.Typed.Script but it could be discussed outside of thisPR.
| , MonadError MkTxError m | ||
| ) | ||
| => ChainIndexTxOut -> m (Maybe (Versioned Validator, Datum, Value)) | ||
| => ChainIndexTxOut -> m (Maybe (Versioned Validator, (Datum, Bool), Value)) |
There was a problem hiding this comment.
We can maybe change it to Maybe (Versioned Validator, Datum, Bool, Value)? It would improve readability when we need to consume the result of the function.
There was a problem hiding this comment.
I'm not sure, the Bool really belongs to the Datum. I think I should just make yet another specific datatype. (It's really annoying that als these datum-related types have to be slightly different!)
| unsafeMkTypedValidator :: Versioned Validator -> TypedValidator Any | ||
| unsafeMkTypedValidator vl = |
There was a problem hiding this comment.
Not really related to the PR but still: did we officially decide of a strategy regarding the content of Ledger.Typed.Scripts vs PSU.Vx.Typed.Scripts modules? The deployment of V2 scripts may require clarification. We have an epic about cleaning the code after the Babbage era release, maybe we need to add a review/clean up/harmonisation of these modules to it? (cc @koslambrou)
There was a problem hiding this comment.
I'd say at least keep the version specific things as much as possible in PSU.Vx.Typed.Scripts, and let the rest work with Versioned scripts. It's tempting to export "the latest version" code from Ledger.Typed.Scripts too, but I'm not sure we should do that, and at least it would silently break a lot of our tests at the moment. (But that's a bug really, tests that depend on something being version 1 should explicitly import from V1 modules.)
There was a problem hiding this comment.
I'd say at least keep the version specific things as much as possible in
PSU.Vx.Typed.Scripts, and let the rest work withVersionedscripts. It's tempting to export "the latest version" code fromLedger.Typed.Scriptstoo, but I'm not sure we should do that, and at least it would silently break a lot of our tests at the moment. (But that's a bug really, tests that depend on something being version 1 should explicitly import fromV1modules.)
Yes I was thinking of something alike, it would be quite aligned with Konstantinos' ADR about cardano API as well. We should probably stick to this behaviour in Ledger.Test as well (and maybe in other places, but I don't see where). At least it would give us a set of clearer rules to deal with plutus versions. This would probably imply moving Versioned to plutus-ledger as well?
koslambrou
left a comment
koslambrou
left a comment
There was a problem hiding this comment.
LGTM. Just documentation comments.
|
|
||
| mustSpendScriptOutputsInlineDatumHasNoDataInTx :: TestTree | ||
| mustSpendScriptOutputsInlineDatumHasNoDataInTx = | ||
| testGroup "mustSpendScriptOutput should not include datum in tx when spending a ref with inline datum" |
There was a problem hiding this comment.
The test group description is not accurate.
| ScriptAddress !(Either (Versioned Validator) (Versioned TxOutRef)) !Redeemer !(Maybe Datum) | ||
| -- ^ A transaction input that consumes (with a validator) or references (with a txOutRef) | ||
| -- a script address with the given the redeemer and datum. | ||
| -- Datum is optional in case the input references an inline datum. |
There was a problem hiding this comment.
Mmm... What do you mean here exactly?
Do you actually mean "Datum is optional if the input refers to a script output which contains an inline datum"?
| addScriptTxInput :: TxOutRef -> Versioned Validator -> Redeemer -> Datum -> Tx -> Tx | ||
| addScriptTxInput outRef vl rd dt tx@Tx{txInputs, txScripts, txData} = tx | ||
| {txInputs = TxInput outRef (TxScriptAddress rd (Left vlHash) dtHash) : txInputs, | ||
| -- Datum is optional when the reference points to an inline datum. |
| addReferenceTxInput outRef vref rd dt tx@Tx{txInputs, txData} = tx | ||
| {txInputs = TxInput outRef (TxScriptAddress rd (Right vref) dtHash) : txInputs, | ||
| txData = Map.insert dtHash dt txData} | ||
| -- Datum is optional when the reference points to an inline datum. |
sjoerdvisscher
force-pushed
the
PLT-658-Inline-Datum-Witnesses
branch
from
7d01822 to
578be1e
Compare
sjoerdvisscher
force-pushed
the
PLT-658-Inline-Datum-Witnesses
branch
from
7c52831 to
79eb64a
Compare
* Support empty witness for inline datum reference * Add test * Fix merge issues * Fix PureScript * Clean up and documentation * Replace (Datum, Bool) with DatumWithOrigin
3 participants
Pre-submit checklist: