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

PLT-713: must mint value with reference #758

Merged
merged 2 commits into from
Oct 28, 2022
Merged

Conversation

ak3n
Copy link
Contributor

@ak3n ak3n commented Oct 14, 2022

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
    • Reference the ADR in the PR and reference the PR in the ADR (if revelant)
    • Reviewer requested

plutus-contract/test/Spec/TxConstraints/MustMint.hs Outdated Show resolved Hide resolved
@@ -111,7 +111,7 @@ checkTxConstraint ctx@ScriptContext{scriptContextTxInfo} = \case
-- gives us the redeemer's hash, but 'MustSpendScriptOutput' gives
-- us the full redeemer
$ isJust (V.findTxInByTxOutRef txOutRef scriptContextTxInfo)
MustMintValue mps _ tn v ->
MustMintValue mps _ tn v _ ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it fail if a reference script is given?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok to assume it'll only get this far without a reference script in the tx. The same is done for MustSpendScriptOutput.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @james-iohk that we don't have to check because when it is not Nothing construction of the transaction would have already failed.

Of course it is possible to construct the transaction yourself and use this constraint only in the script, and then a "cheating" transaction could store the minting script in the body instead of using the reference script. Perhaps this could theoretically be a security risk in some way? I think it's fine to just document that MustMintValue does not check on-chain that a reference script is used, instead of wasting valuable script resources to do the check.

Copy link
Contributor

@catch-21 catch-21 Oct 25, 2022

Choose a reason for hiding this comment

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

store the minting script in the body instead of using the reference script

This wouldn't pass phase-1 because reference inputs are prohibited when V1 scripts are involved. Forced to remove the reference script altogether, you're back to valid legacy behaviour.
Even if using V2, you cannot have the same script in the txbody and as reference.

@ak3n ak3n marked this pull request as ready for review October 25, 2022 10:49
@@ -127,6 +127,16 @@ toCardanoScriptWitness datum redeemer (P.Versioned script lang) = (case lang of
) <*> pure datum
<*> pure (C.fromPlutusData $ PV1.toData redeemer)
<*> pure zeroExecutionUnits
toCardanoScriptWitness datum redeemer (Right (P.Versioned ref lang)) = (case lang of
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be possible to merge this code with the code for toCardanoTxInWitness.

fix

Merge two cases of toCardanoScriptWitness into one

Rename txMintingScripts to txMintingWitnesses. Use it for references.

nits

fix

wip

Apply James's patch

Update plutus-use-cases golden files
@ak3n ak3n merged commit 2b81178 into main Oct 28, 2022
@ak3n ak3n deleted the must-mint-value-reference branch October 28, 2022 19:35
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.

None yet

5 participants