Parameterize Value parser on role of the Value being parsed: transaction output or minting policy

[smelc]

Changelog

- description: |
    Parameterize Value parser on role of the Value being parsed: transaction output or minting policy
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

• API part of fixing Negative values are not checked when building tx and ledger exception is returned cardano-cli#904 in the CLI
• CLI PR using this: Negative values are not checked when building tx and ledger exception is returned cardano-cli#904

How to trust this PR

1. This PR adds more checks
2. Existing tests still pass (and there are a number of them!)

Checklist

• Commit sequence broadly makes sense and commits have useful messages
• Self-reviewed the diff

[carbolymer]

I'm wondering about this usage site: https://github.com/IntersectMBO/cardano-cli/blob/447fa87001663f942a8f79252876b355fa1f0a04/cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs#L2046

It's a parser of a whole utxo value, so it's not a Tx out value. It should work with RoleTxOut case of the parser, but the name should be different I think.

[smelc]

@carbolymer> What do you prefer:

1. Rename RoleTxOut into RoleTxOutOrUtxo
2. Add a RoleUtxo

I'm tempted by option 2., in which case we could check that the quantity is positive and the only AssetId is ada?

[carbolymer]

I'm liking the second option.

the only AssetId is ada?

UTXO can contain also non-ada, right?

[smelc]

@carbolymer> yes I checked and UTxO can contain non-ada. So I added the new RoleUTxO but keps the same checks as RoleTxOut.

[carbolymer]

I'm not sure that duplicated role makes sense here. You will always be able to put things which are in UTXO into TX output, so the format for the value will be the same.

I think I would just stick with RoleUTxO, since all tx outputs will be "unspent".

[smelc]

@carbolymer> Only kept RoleUTxO 👍

[carbolymer]

In line 67, there's a case match:

ValueExprMultiAsset polId aName quant -> [(AssetId polId aName, quant)]

could you provide a parser

Parser (PolicyId, AssetName, Quantity)

?

(It could be also Parser [(PolicyId, AssetName, Quantity)] - whatever is more convenient here).

That would be very useful for parsing values for minting.

This can be done in a follow up PR.

[carbolymer]

If you don't plan to work on it, I can implement that after you finish your work.

[smelc]

@carbolymer> I want to have @Jimbo4350's feedback before amending this PR.

[Jimbo4350]

I was thinking of a change like this: master...jordan/delete-me-value-parser-test

ghci> parse parseMintingMultiAssetValue "" "-5 51936f3c98a04b6609aa9b5c832ba1182cf43a58e534fcc05db09d69.4D696C6C6172436F696E"
Right (valueFromList [(AssetId "51936f3c98a04b6609aa9b5c832ba1182cf43a58e534fcc05db09d69" "MillarCoin",-5)])
ghci> parse  parseTxOutMultiAssetValue "" "-5 51936f3c98a04b6609aa9b5c832ba1182cf43a58e534fcc05db09d69.4D696C6C6172436F696E"
Left (line 1, column 1):
unexpected "-"
expecting multi-asset value expression

We could customize the error message if need be.

[smelc]

@Jimbo4350> the problem with the approach consisting of restricting the operators is that it forbids sums whose total is positive, but that contain negative operands (as tested in this test: https://github.com/IntersectMBO/cardano-cli/blob/43137957a2e943bf9873718f1c99591e8b54afd0/cardano-cli/test/cardano-cli-test/Test/Cli/Shelley/Transaction/Build.hs#L93).

I checked with Modus Create's smart contracts auditing team and they reported that they see such uses.

So I kept my approach but added the new functions parseTxOutMultiAssetValue and parseMintingMultiAssetValue that you had shown.

[carbolymer]

@Jimbo4350

parseMintingMultiAssetValue :: Parser Value

My main problem with the parser is it's return type. Value is too broad for minting - you shouldn't be able to keep ADA there.

[carbolymer]

Regarding negative values in minting, I think we still need them for burning tokens. https://developers.cardano.org/docs/native-tokens/minting/#burning-token

[Jimbo4350]

I would return the value as well in the error messages.

[smelc]

Showed the value 👍

@@ -52,12 +52,12 @@ parseValue role = do
   case role of
     RoleUTxO -> do
       unless (allPositive value) $
-        fail "Value must be positive in an UTxO (or transaction output)"
+        fail $ "Value must be positive in UTxO (or transaction output): " <> show value
       return value
     RoleMint -> do
       let (Coin lovelace) = selectLovelace value
       when (lovelace /= 0) $
-        fail "Lovelace must be zero in a minting value"
+        fail $ "Lovelace must be zero in minting value: " <> show value
       return value