Unify cert indexing via indexCertificatesWith#1140
Open
Conversation
…itnessed certs Introduce indexCertificatesWith, a shared helper that associates each certificate with its position-based ScriptWitnessIndex while preserving unwitnessed certificates (those stored with a Nothing witness). Previously indexTxCertificates filtered out Nothing-witness certs, silently dropping them during fee balancing. The return type now includes Maybe so callers must handle both cases. Fixes #1138
Update indexTxCertificates in the experimental API and Compatible/Tx to use the shared indexCertificatesWith helper. Fix mapScriptWitnessesCertificates in both the legacy and experimental APIs to handle Nothing-witness certs instead of silently dropping them. Also fix scriptWitnessesCertificates in collectTxBodyScriptWitnesses to collect all script witnesses (simple and Plutus), not just Plutus.
- Cover ConwayRegCert (unwitnessed), ConwayUnRegCert (key-witnessed), ConwayDelegCert (simple-script-witnessed), and ConwayRegDelegCert (key-witnessed) in the substituteExecutionUnits property. - Randomize cert ordering via Gen.shuffle for meaningful variation. - Add property: collectTxBodyScriptWitnesses returns exactly the script-witnessed certs and ignores unwitnessed/key-witnessed ones. - Add property: createCompatibleTx preserves all certs (witnessed and unwitnessed) in the resulting ledger transaction body. - Extract shared genShuffledCertsWithCount generator.
Jimbo4350
force-pushed
the
unify-cert-indexing-1138
branch
from
6f39374 to
e33e202
Compare
Jimbo4350
requested review from
CarlosLopezDeLara,
carbolymer,
dcoutts,
disassembler,
erikd,
kevinhammond,
newhoggy and
palas
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes certificate indexing across the legacy, experimental, and compatible transaction-building paths to ensure certificate ordering/indexing matches ledger semantics and to prevent silently dropping certificates that have no witness (Nothing).
Changes:
- Introduces a shared
indexCertificatesWithhelper and updates allindexTxCertificatesimplementations to preserve unwitnessed certs viaMaybewitnesses. - Fixes fee-balancing and script-witness collection to handle
Nothing-witness certs correctly and to collect simple-script cert witnesses (not only Plutus). - Expands/extends property tests to cover certificate preservation and witness collection across the affected code paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
cardano-api/src/Cardano/Api/Tx/Internal/Body.hs |
Adds indexCertificatesWith, updates indexTxCertificates to include unwitnessed certs, and adjusts cert script-witness collection accordingly. |
cardano-api/src/Cardano/Api/Tx/Internal/Fee.hs |
Updates substituteExecutionUnits certificate handling to preserve certs with Nothing witnesses when rebuilding TxCertificates. |
cardano-api/src/Cardano/Api/Experimental/Tx/Internal/Fee.hs |
Reuses the unified indexing approach; fixes cert mapping and witness collection to include simple scripts and preserve unwitnessed certs. |
cardano-api/src/Cardano/Api/Compatible/Tx.hs |
Updates compatible transaction construction to use the new Maybe witness form while preserving certs and correct indices. |
cardano-api/test/cardano-api-test/Test/Cardano/Api/Experimental/Fee.hs |
Adds/updates regression properties for cert preservation, script witness collection from certs, and compatible tx cert preservation. |
| -- store as @Nothing@ in the OMap (no witness required) and some as @Just@ (witness | ||
| -- required) — then verify that all survive 'substituteExecutionUnits' unchanged. | ||
| prop_substituteExecutionUnits_preserves_certs :: Property | ||
| prop_substituteExecutionUnits_preserves_certs = H.property $ do |
Contributor
Author
There was a problem hiding this comment.
I'm still leaning towards property.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog
Context
Follow-up to #1136 based on review feedback from @carbolymer.
indexCertificatesWithso cert-indexing logic lives in one placeindexTxCertificatesin all three locations to returnMaybewitnesses instead of filtering outNothingmapScriptWitnessesCertificatesin both legacy and experimental APIs — previously silently dropped unwitnessed certs during fee balancingscriptWitnessesCertificatesincollectTxBodyScriptWitnessesto collect all script witnesses (simple + Plutus), not just PlutuscollectTxBodyScriptWitnessesandcreateCompatibleTxcert preservationCloses #1138
Checklist