Fix supplemental datum handling in experimental API#1112
Conversation
Jimbo4350
requested review from
CarlosLopezDeLara,
carbolymer,
dcoutts,
disassembler,
erikd,
kevinhammond,
newhoggy and
palas
as code owners
There was a problem hiding this comment.
Pull request overview
This PR fixes critical bugs in the handling of supplemental datums in the experimental transaction API. Supplemental datums are now correctly converted to DatumHash references (instead of inline Datum representations) and explicitly tracked in TxBodyContent, ensuring proper separation from inline datums in the witness set.
Changes:
- Fixed
toLedgerDatumto convert supplemental datums toDatumHashinstead of inlineDatum - Added
txSupplementalDatumsfield toTxBodyContentfor explicit tracking of supplemental datum data - Refactored
getDatumsto use the new explicit supplemental datums field instead of extracting all datums from outputs
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cardano-api/src/Cardano/Api/Experimental/Tx/Internal/BodyContent/New.hs | Core fix: updated datum conversion logic, added supplemental datums tracking to TxBodyContent, and refactored datum collection |
| cardano-api/src/Cardano/Api/Experimental/Tx.hs | Exported new setTxSupplementalDatums and supplementalDatumFromLegacy functions |
| cardano-api/src/Cardano/Api/Experimental/Tx/Internal/Fee.hs | Added wildcard pattern to match new txSupplementalDatums field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| :: forall era | ||
| . IsEra era | ||
| => OldApi.TxOut CtxTx era | ||
| -> Either DatumDecodingError (TxOut (LedgerEra era), Map L.DataHash (L.Data (LedgerEra era))) |
There was a problem hiding this comment.
The function signature change to return a tuple including supplemental datums is significant but lacks documentation explaining the new return value. Add a docstring explaining that the second element of the tuple contains supplemental datum data extracted from the output, or an empty map for non-supplemental datums.
| supplementalDatumFromLegacy | ||
| :: L.Era (LedgerEra era) | ||
| => OldApi.TxOutDatum CtxTx era | ||
| -> Map L.DataHash (L.Data (LedgerEra era)) |
There was a problem hiding this comment.
The function has a comment header but the signature lacks a proper docstring. Consider adding a Haddock docstring that explains the function's purpose, parameters, and return value to complement the existing comment at line 379-380.
| L.TxDats $ | ||
| fromList $ | ||
| refInDatums <> txOutsDats | ||
| L.TxDats $ fromList refInDatums <> supplementalDats |
There was a problem hiding this comment.
The expression fromList refInDatums <> supplementalDats combines a list-to-map conversion with map concatenation. This could be clearer as fromList (refInDatums <> toList supplementalDats) or by converting both to maps first, making the data type transformations more explicit.
| L.TxDats $ fromList refInDatums <> supplementalDats | |
| L.TxDats $ fromList (refInDatums <> toList supplementalDats) |
|
This PR description is not following the template. |
Jimbo4350
force-pushed
the
jordan/supplemental-datum-fix
branch
from
3642778 to
97e322e
Compare
getDatums previously extracted datums from transaction outputs using L.dataTxOutL, which could not distinguish between inline datums and supplemental datums, causing both to be added to TxDats. Per the Alonzo ledger spec, supplemental datums are only those whose hashes correspond to output DatumHash fields. Inline datums should never appear in TxDats. This commit introduces a txSupplementalDatums field on TxBodyContent so supplemental datums can be provided explicitly, and refactors getDatums to use this field instead of extracting from outputs.
toLedgerDatum was incorrectly converting TxOutSupplementalDatum to L.Datum (inline representation). Per the Alonzo ledger spec, supplemental datums should produce L.DatumHash on the output, with the full datum data going into TxDats separately. Also updates fromLegacyTxOut to return supplemental datum data alongside the TxOut, so callers can populate txSupplementalDatums. Adds supplementalDatumFromLegacy helper for this purpose.
Jimbo4350
force-pushed
the
jordan/supplemental-datum-fix
branch
from
97e322e to
59e5004
Compare
Changelog
Context
The previous implementation had two bugs:
toLedgerDatumconverted supplemental datums to inline datum representation (L.Datum), but per the Alonzo ledger spec they should only set aDatumHashon the output with the actual data going into the witness set (TxDats).getDatumsextracted datums from all transaction outputs usingL.dataTxOutL, which couldn't distinguish inline datums from supplemental datums, causing inline datums to also be added toTxDats.This PR introduces a
txSupplementalDatumsfield onTxBodyContentso supplemental datums can be provided explicitly, and fixestoLedgerDatumto produceL.DatumHashfor supplemental datums. It also adds asupplementalDatumFromLegacyhelper and updatesfromLegacyTxOutto return supplemental datum data alongside theTxOut.How to trust this PR
getDatums, the second fixestoLedgerDatumand updatesfromLegacyTxOutCardano.Api.Experimental.TxDatumHashin outputs (not inlineDatum)TxDatscabal build allandcabal test cardano-api:cardano-api-testshould passChecklist