-
Notifications
You must be signed in to change notification settings - Fork 25
Implement experimental estimateBalancedTxBody
#887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement experimental estimateBalancedTxBody
#887
Conversation
8a27422
to
d5f6785
Compare
d5f6785
to
0d7851a
Compare
4699948
to
2afafe7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite the fact that it's needed only for experimental API, we should keep orphans in as small number of places as possible. I think this should go to one of the already existing orphans module, so it'll be easier to propagate orphans through cardano-api.
cardano-api/src/Cardano/Api/Experimental/Tx/Internal/AnyWitness.hs
Outdated
Show resolved
Hide resolved
2afafe7
to
e112e36
Compare
-> Either | ||
CBOR.DecoderError | ||
[AnyIndexedPlutusScriptWitness (LedgerEra era)] | ||
extractAllIndexedPlutusScriptWitnesses era b = do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extractAllIndexedPlutusScriptWitnesses era b = do | |
extractAllIndexedPlutusScriptWitnesses era b = obtainEraCommonConstraints era $ do |
shelleyLedgerEraToLedgerEra
can be then removed. We shouldn't add more era matching functions unless they're really needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I forgot the transformation ShelleyLedgerEra era ~ LedgerEra era
is provided with obtainCommonConstraints
.
4e45c1b
to
676b334
Compare
676b334
to
7b92a5e
Compare
7b92a5e
to
d567415
Compare
cardano-api/test/cardano-api-test/Test/Cardano/Api/Transaction/Body/Plutus/Scripts.hs
Outdated
Show resolved
Hide resolved
cardano-api/test/cardano-api-test/Test/Cardano/Api/Transaction/Body/Plutus/Scripts.hs
Outdated
Show resolved
Hide resolved
& setTxProposalProcedures Nothing | ||
& setTxMintValue mempty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels kind of redundant, because those fields are set again in L173-177
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not. If I don't set the fields to empty the test will fail because there will be a different number of script witnesses.
I was measuring all script witnesses earlier, so yes it's redundant 👍
) | ||
] | ||
fromLegacyMintWitness aeon = do | ||
legacyWitnessConversion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that's outside of the scope of this PR, but legacyWitnessConversion
should be a verb, not a noun.
d567415
to
079de21
Compare
Changelog
Context
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