-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add Cardano.Api.Tx.Compatible #644
Conversation
b824b26
to
688ca33
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.
Looks good 👍
Just a couple of lints
txbody = | ||
conwayEraOnwardsConstraints conwayOnwards $ | ||
createCommonTxBody sbe ins outs txFee' | ||
& L.proposalProceduresTxBodyL | ||
.~ OSet.singleton updateProposalProcedure |
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.
txbody = | |
conwayEraOnwardsConstraints conwayOnwards $ | |
createCommonTxBody sbe ins outs txFee' | |
& L.proposalProceduresTxBodyL | |
.~ OSet.singleton updateProposalProcedure | |
bodyWithProtocolUpdate = | |
conwayEraOnwardsConstraints conwayOnwards $ | |
createCommonTxBody sbe ins outs txFee' | |
& L.proposalProceduresTxBodyL | |
.~ OSet.singleton updateProposalProcedure |
Best to use the same terminology as other branches. I would also recommend moving common bits to outside the case, but I've tried, and it seems the Haskell constraint solver doesn't like that at all
688ca33
to
0111746
Compare
This is still a WIP as it's being integrated in cardano-cli and needs to be tested by QA. |
-- | This module provides a way to construct a simple transaction over all eras. | ||
-- It is exposed for testing purposes only. | ||
module Cardano.Api.Tx.Compatible |
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.
The module name and the description is not quite clear for me. Is it supposed to be a module for support of transaction building for older eras once we remove them from API?
Maybe we should add Internal
to the module name e.g. Cardano.Api.Tx.Internal.Compatible
, to mark that it's not meant to be directly used?
I'm not sure about the name as well. Maybe we should name it something like Old
or Legacy
to put emphasis that it's meant just for backwards compatibility for tests.
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 meant to be directly used. It's supposed to house all functions that are expected to be backwards compatible across all shelley based eras. I'm not opposed to a rename but I'm not blocking the PR on this.
0111746
to
848d2ba
Compare
This module exposes createCompatibleSignedTx which is intended to be used in testing only. It allows creation of simple unbalanced transactions that can submit protocol updates in any era.
848d2ba
to
d6e71bd
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