Open
Conversation
Jimbo4350
force-pushed
the
jordan/tx-body-content-refactoring-fixes
branch
2 times, most recently
from
2f27bcb to
94d2bf5
Compare
Jimbo4350
commented
Jan 28, 2026
cardano-api/src/Cardano/Api/Experimental/Tx/Internal/BodyContent/New.hs
Outdated
Show resolved
Hide resolved
Jimbo4350
force-pushed
the
jordan/tx-body-content-refactoring-fixes
branch
from
a4bf7e5 to
b849b1e
Compare
Jimbo4350
requested review from
CarlosLopezDeLara,
carbolymer,
dcoutts,
disassembler,
erikd,
kevinhammond,
newhoggy and
palas
as code owners
Jimbo4350
force-pushed
the
jordan/tx-body-content-refactoring-fixes
branch
from
b849b1e to
5595edb
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Jimbo4350
force-pushed
the
jordan/tx-body-content-refactoring-fixes
branch
6 times, most recently
from
6f0764f to
294ecd9
Compare
Jimbo4350
force-pushed
the
jordan/tx-body-content-refactoring-fixes
branch
from
294ecd9 to
b210b64
Compare
palas
approved these changes
Feb 3, 2026
Contributor
palas
left a comment
palas
left a comment
There was a problem hiding this comment.
Looks good 👍. Just a typo and a suggestion
Comment on lines
+753
to
+758
| substitudeExecUnitsTxMint | ||
| :: ScriptWitnessIndex | ||
| -> AnyScriptWitness (LedgerEra era) | ||
| -> Either (TxBodyErrorAutoBalance (LedgerEra era)) (AnyScriptWitness (LedgerEra era)) | ||
| substitudeExecUnitsTxMint _ w@AnyScriptWitnessSimple{} = Right w | ||
| substitudeExecUnitsTxMint idx (AnyScriptWitnessPlutus psw) = |
Contributor
There was a problem hiding this comment.
Suggested change
| substitudeExecUnitsTxMint | |
| :: ScriptWitnessIndex | |
| -> AnyScriptWitness (LedgerEra era) | |
| -> Either (TxBodyErrorAutoBalance (LedgerEra era)) (AnyScriptWitness (LedgerEra era)) | |
| substitudeExecUnitsTxMint _ w@AnyScriptWitnessSimple{} = Right w | |
| substitudeExecUnitsTxMint idx (AnyScriptWitnessPlutus psw) = | |
| substituteExecUnitsTxMint | |
| :: ScriptWitnessIndex | |
| -> AnyScriptWitness (LedgerEra era) | |
| -> Either (TxBodyErrorAutoBalance (LedgerEra era)) (AnyScriptWitness (LedgerEra era)) | |
| substituteExecUnitsTxMint _ w@AnyScriptWitnessSimple{} = Right w | |
| substituteExecUnitsTxMint idx (AnyScriptWitnessPlutus psw) = |
Typo substitudeExecUnitsTxMint -> substituteExecUnitsTxMint
| [ (policyId, (assets,) <$> substitutedWitness) | ||
| | (ix, policyId, assets, wit) <- indexTxMintValue txMintValue' | ||
| , let substitutedWitness = substituteExecUnits ix wit | ||
| , let substitutedWitness = substitudeExecUnitsTxMint ix wit |
Contributor
There was a problem hiding this comment.
Suggested change
| , let substitutedWitness = substitudeExecUnitsTxMint ix wit | |
| , let substitutedWitness = substituteExecUnitsTxMint ix wit |
Also here
Comment on lines
+759
to
+765
| case Map.lookup idx exUnitsMap of | ||
| Nothing -> | ||
| Left $ TxBodyErrorScriptWitnessIndexMissingFromExecUnitsMap idx exUnitsMap | ||
| Just exunits -> | ||
| Right $ | ||
| AnyScriptWitnessPlutus $ | ||
| updatePlutusScriptWitnessExecutionUnits exunits psw |
Contributor
There was a problem hiding this comment.
This bit should probably be a separate function because it is identical to the bit in substituteExecUnits. Maybe substituteExecUnitsInPsw.
carbolymer
reviewed
Feb 3, 2026
| newDatum :: L.Datum (LedgerEra era) <- obtainCommonConstraints (useEra @era) $ toLedgerDatum d | ||
| return $ obtainCommonConstraints (useEra @era) $ TxOut $ o & L.datumTxOutL .~ newDatum | ||
|
|
||
| newtype DatumDecodingError = DataDecodingError String |
Contributor
There was a problem hiding this comment.
Why the type name and the constructor names are slightly different?
carbolymer
requested changes
Feb 3, 2026
| proxyToAsType _ = AsConwayEra | ||
|
|
||
| instance HasTypeProxy L.DijkstraEra where | ||
| data AsType L.DijkstraEra = AsDijkstraEra |
Contributor
There was a problem hiding this comment.
You have not exported the name constructors here. Additionally, there would be a name clash between definitions of our eras and ledger eras, since the constructors have the same name: https://github.com/IntersectMBO/cardano-api/blob/mgalazyn/proto-lens-ghc-9.12/cardano-api/src/Cardano/Api/Era/Internal/Core.hs#L108-L108
Perhaps it's the sign to switch to using ledger eras everywhere?
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
3 participants
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
Resolves:
TxOutdefinition can be refactored #1084UnsignedTx#1089Context
Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.
How to trust this PR
Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.
Checklist