-
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 new types to CDDL list and test forward compatibility of deserialiseTxLedgerCddl
#634
Conversation
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.
LGTM! We need to remove all the type strings that are not defined in a HasTextEnvelope
instance.
@@ -32,6 +33,18 @@ import Test.Tasty.Hedgehog (testProperty) | |||
-- TODO: Need to add PaymentExtendedKey roundtrip tests however | |||
-- we can't derive an Eq instance for Crypto.HD.XPrv | |||
|
|||
prop_forward_compatibility_txbody_CBOR :: Property |
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 test does the same as the one below, however prop_roundtrip_txbody_CBOR
uses a deprecated function. I guess it would be nice to add a comment with that information.
@Jimbo4350 when should we remove already deprecated API functions?
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.
Added comment here: b33f0af
9fcb2ba
to
e7a7066
Compare
Changelog
Context
There was an issue with transactions created using the non-deprecated function
serialiseToTextEnvelope
as alternative toserialiseTxLedgerCddl
. Because the implementation incardano-cli
still doesn't accept it.This PR adds a test ensuring the
deserialiseTxLedgerCddl
function is compatible withserialiseToTextEnvelope
.It also adds the new types to the list of
cddlTypeToEra
conversion function, this is necessary for the corresponding fix incardano-cli
: IntersectMBO/cardano-cli#892How to trust this PR
The change is small. Probably just make sure you agree with the direction of the change.
Checklist