Allow providing of actual datum for reference inputs

[carbolymer]

Changelog

- description: |
    Allow providing of actual datum for reference inputs in `TxInsReference`.
# 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

This PR adds a method of adding actual datum for the datum hashes present in reference inputs.

Tested in:

• cardano-testnet | Add test submitting transaction with supplemental datums using cardano-api cardano-node#6174

Integrated in CLI:

• Add posibility to supply supplemental datum hash preimage cardano-cli#1169

Closes #803

Checklist

• Commit sequence broadly makes sense and commits have useful messages
• New tests are added if needed and existing tests are updated. See Running tests for more details
• Self-reviewed the diff

[carbolymer]

@mmontin This should implement #803 . What do you think about the design here?

[Jimbo4350]

LGTM 👍 . Get input from the ledger team regarding naming and we can finalize that aspect.

[Jimbo4350]

TxInsReferenceAdditionalDatums? Can you run this by the ledger team. We should also get their input, it would be useful.

[mmontin]

How about TxInsReferenceResolvedDatums?

[mmontin]

Thanks a lot for implementing this ! I believe this feature, while somewhat a corner case, is very important to have.

In terms of the actual implementation, I have two general comments / remarks on top of the small comments I left here and there

1. I wonder why these additional datums are given as a set directly inside TxInsReference instead of input by input. This breaks the link as to which input is supposed to bear which datum hash. In other words, why not couple each TxIn with a Maybe HashableScriptData in a structure (let's call it RefInputWithDatum) and have TxInsReference take a list of those?

2. About the need of providing a UTxO here and there. I understand the need for this structure. It allows to ensure that the provided datums do indeed correspond to the resolved variants of hashed datums in reference inputs. However, this will be caught later on in the pipeline anyway as the ledger with throw an error when they do not. So is this necessary to ensure this here? Admittedly, I don't have a full picture as to which properties need to be satisfied by createTransactionBody and are expected to hold by the caller. However, it makes the implementation more involved, and also forces the caller to build this UTxO and understand that it corresponds to the corner case of providing resolved datums to reference inputs. Additionally, since the current implementation seems to just ignore datums that do not correspond to hashed datums in ref inputs, there is no clear way to bypass this requirement for testing purposes. And I believe this is dangerous that this process is silent. This makes me wonder if there are other places in the code base where silent discards are performed.

[mmontin]

How about TxInsReferenceResolvedDatums?

[mmontin]

It seems weird that addTxInsReference allows to provide the full datums for the given list of TxIn while addTxInReference does not for its own TxIn. I would expect to have an additional argument of type Maybe HashableScriptData.

[carbolymer]

Good point, added, thanks.

[mmontin]

It seems that this will silently ignore datums that do not correspond to hashed datums in the reference inputs. Is there any way to make this visible? Are there other places where such elements are ignored under the hood?

[carbolymer]

@mmontin Good remarks thanks!

ad 2. I agree. Passing UTXO around mostly gets in the way, without any real benefit here. I think we could skip it, ditch the filtering and do the validation at transaction submission step (by ledger).

ad 1. Assuming we remove the necessity of passing UTXO: tying TxIns with datum values would only make sense if we have UTXOs of reference inputs in the scope, and we would validate that respective datum is at the right input. I agree that there's a link there, but I'm not sure there's a value in that information: this link gets "lost" when the transaction gets submitted to the ledger. So the constructor TxInsReference would potentially look similar to this:

 TxInsReference
    :: BabbageEraOnwards era
    -> [( TxIn
        , BuildTx build (Maybe HashableScriptData)
        )
       ]
    -> TxInsReference build era

which would complicate handling of the type a bit: the values would have to be zipped and unziped when constructing and deconstructing the type.
Do you have an example, where this form of TxInsReference would be beneficial?

[mmontin]

About location of datums in the body. Your point makes sense, it would be unwise to complicate the type for no valid reason. And to be honest, the current version where the datum are grouped into a set would be suitable enough for my use case and overall satisfactory, in my opinion. However, I see 2 reasons why the other option also makes sense, based on my current understanding of cardano-api and my current biases as to how I use it in cooked-validators:

1. Consistency: Currently, the second use case for supplemental datums (when paying outputs with hashed datums) is handled locally, within the TxOut itself. Technically, TxOutSupplementalDatum is the same as TxOutDatumHash with the exception that the full datum will be given to the transaction. This means that TxOutSupplementalDatum could technically be removed and, instead, the txOuts field of the TxBodyContent could be typed:

TxOuts
   :: BabbageEraOnwards era -- not sure about Babbage, but not so relevant for the example
   -> [TxOut ctx era]
   -> Set (BuildTx build HashableScriptData)
   -> TxOuts build era

I am definitely not advocating for such a change, just pointing out that the choice was made to have these supplemental datums be handled within each TxOut instead of grouped outside. Admittedly, this makes a lot more sense here, because the TxOuts naturally contain a datum, as opposed to TxIn which are merely ids. That being said, handling supplemental datums similarly for the second possible use case would make sense and likely be more familiar to future users relying on this new feature.

2. Locality: This one is more related to my personal bias, but I like when the information is as close as possible to where it belongs. Granted, this will be lost later on in the pipeline, but I see cardano-api transaction bodies as an opportunity to layout pieces of information in a more local way as the transaction produced by the ledger and given to the scripts. Having these supplemental datums defined locally would feel just right (for me at least) and this is the place where I would naturally search for this information. That being said, having the datum a bit higher up does not hurt and they could also be found quite easily there.

I feel like this second point is not quite as strong as I thought when I decided to write it, because it's mostly just based on a feeling, and I leave it to you to take it into account or not. Regardless, I hope these points are of help in making the final decision.

[Jimbo4350]

1. I wonder why these additional datums are given as a set directly inside TxInsReference instead of input by input. This breaks the link as to which input is supposed to bear which datum hash. In other words, why not couple each TxIn with a Maybe HashableScriptData in a structure (let's call it RefInputWithDatum) and have TxInsReference take a list of those?

What would be the purpose? You would lose the connection as soon as you construct a valid transaction.

However, this will be caught later on in the pipeline anyway as the ledger with throw an error when they do not.

I agree with this regarding removing the UTxO era parameter. cc: @carbolymer

[Jimbo4350]

just pointing out that the choice was made to have these supplemental datums be handled within each TxOut instead of grouped outside.

I see your point but the TxOut type was designed this way so that consumers of cardano-api could easily differentiate between the different datums that exist at outputs.

I wouldn't include the "resolved datums' in the TxOut definition because currently it concerns itself with the creation of datums/datum hashes (or not) at outputs. The "resolved datums" feature is about including already existing datums in the datum map which to me means it should live with the reference inputs definition.