-
Notifications
You must be signed in to change notification settings - Fork 25
Add SerialiseAsRawBytes
instance to UnsignedTx ConwayEra
#880
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
base: master
Are you sure you want to change the base?
Conversation
9933152
to
4c1e0b0
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.
I agree with @Jimbo4350
Co-authored-by: Mateusz Galazyn <[email protected]>
cd55c85
to
75b3755
Compare
, Ledger.EncCBOR (Ledger.Tx (LedgerEra era)) | ||
, Ledger.DecCBOR (Ledger.Annotator (Ledger.Tx (LedgerEra era))) |
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 think it's better to have those constraints in UnsignedTx
. I mean to write:
data UnsignedTx
= L.EraTx (LedgerEra era) => UnsignedTx (Ledger.Tx (LedgerEra era))
instead of adding them to instances.
Rationale: we'll always be dealing with L.EraTx
instances for transactions. Having L.EraTx
is easier to understand and satisfy, rather than dealing with more specialised constraints like EncCBOR
.
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.
Still, that would work for serialiseToRawBytes
, but I don't think it is possible to implement deserialiseFromRawBytes
without the assumption that era
is a valid era
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.
But I think we can simplify constraints further:
diff --git c/cardano-api/src/Cardano/Api/Experimental/Tx.hs i/cardano-api/src/Cardano/Api/Experimental/Tx.hs
index 208a65777..1978d1efd 100644
--- c/cardano-api/src/Cardano/Api/Experimental/Tx.hs
+++ i/cardano-api/src/Cardano/Api/Experimental/Tx.hs
@@ -165,7 +165,6 @@ import Cardano.Crypto.Hash qualified as Hash
import Cardano.Ledger.Alonzo.TxBody qualified as L
import Cardano.Ledger.Api qualified as L
import Cardano.Ledger.Binary qualified as Ledger
-import Cardano.Ledger.Conway qualified as Ledger
import Cardano.Ledger.Conway.TxBody qualified as L
import Cardano.Ledger.Core qualified as Ledger
import Cardano.Ledger.Hashes qualified as L hiding (Hash)
@@ -180,8 +179,8 @@ import Lens.Micro
-- | A transaction that can contain everything
-- except key witnesses.
-newtype UnsignedTx era
- = UnsignedTx (Ledger.Tx (LedgerEra era))
+data UnsignedTx era
+ = L.EraTx (LedgerEra era) => UnsignedTx (Ledger.Tx (LedgerEra era))
instance HasTypeProxy era => HasTypeProxy (UnsignedTx era) where
data AsType (UnsignedTx era) = AsUnsignedTx (AsType era)
@@ -189,10 +188,8 @@ instance HasTypeProxy era => HasTypeProxy (UnsignedTx era) where
proxyToAsType _ = AsUnsignedTx (asType @era)
instance
- ( L.Era (LedgerEra era)
- , HasTypeProxy era
- , Ledger.EncCBOR (Ledger.Tx (LedgerEra era))
- , Ledger.DecCBOR (Ledger.Annotator (Ledger.Tx (LedgerEra era)))
+ ( HasTypeProxy era
+ , L.EraTx (LedgerEra era)
)
=> SerialiseAsRawBytes (UnsignedTx era)
where
@@ -210,9 +207,8 @@ instance
:: Ledger.DecoderError -> SerialiseAsRawBytesError
wrapError = SerialiseAsRawBytesError . displayException
-instance IsEra era => Show (UnsignedTx era) where
- showsPrec p (UnsignedTx tx) = case useEra @era of
- ConwayEra -> showsPrec p (tx :: Ledger.Tx Ledger.ConwayEra)
+instance Show (UnsignedTx era) where
+ showsPrec p (UnsignedTx tx) = showsPrec p tx
newtype UnsignedTxError
= UnsignedTxError TxBodyError
@@ -366,5 +362,5 @@ convertTxBodyToUnsignedTx sbe txbody =
(error $ "convertTxBodyToUnsignedTx: Error - unsupported era " <> docToString (pretty sbe))
( \w -> do
let ShelleyTx _ unsignedLedgerTx = makeSignedTransaction [] txbody
- UnsignedTx $ obtainCommonConstraints w unsignedLedgerTx
+ obtainCommonConstraints w $ UnsignedTx unsignedLedgerTx
)
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.
Huh, so these were not necessary at all:
Ledger.EncCBOR (Ledger.Tx (LedgerEra era))
Ledger.DecCBOR (Ledger.Annotator (Ledger.Tx (LedgerEra era)))
But adding L.EraTx (LedgerEra era) =>
does simplify the Show
instance.
I've applied your patch in: 4bf61df
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. Squash all the commits before merging.
Co-authored-by: Mateusz Galazyn <[email protected]>
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.
Nice work, thanks!
Changelog
Context
The standard way of serialising transactions is using CBOR, following the CDDL, and in the case of
CIP-30
, encoding it as base16.SerialiseAsRawBytes
makes this easy. This PR addsSerialiseAsRawBytes
instance toUnsignedTx
.Idea comes from this review in a different PR: #852 (comment)
How to trust this PR
It is mainly format changes. Make sure the new instances are implemented correctly and code design is good.
Checklist