Babbage: TxBody + TxOut #2560

goolord · 2021-11-23T20:18:05Z

this is the beginning of the babbage era work so this pr includes all of the necessary changes to build the new era + the TxBody module.

resolves #2465
resolves #2463

nc6

Looks reasonable, but I don't see where the burn address has been defined.

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs

lehins

@nc6 I have to disagree with you, it doesn't look reasonable, there is way too much duplicated functionality in this PR.

We need to try and figure out how we can reduce duplication as much as possible. Perfect example why this is important is the fact that definition of TxOut has recently changed as well as many of the helper functions that deal with it. Soon CBOR deserialization will change as well (due to sharing)

Maybe we can approach this problem from a different angle and instead of creating a new TxOut type for babbage we can create a new type family for datums, eg:

type family Datum era :: Type
...
  | TxOutCompactDH
      {-# UNPACK #-} !(CompactAddr (Crypto era))
      !(CompactForm (Core.Value era))
      !(Datum era)

... 
type instance Datum Alonzo = DataHash (Crypto Alonzo)
type instance Datum Babbage = Either (DataHash (Crypto Aonzo)) (Data Babbage)
-- ^ Note that `Either` is just an example, a custom type would be better

or something along those lines. This way we could avoid copying this whole monstrosity.
@goolord , @nc6 , @JaredCorduan Any thoughts on this?

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs

goolord · 2021-11-24T20:07:03Z

more code reuse would be nice. a type family is not like an open union though, don't just end up with the same problem?

we are already reasonably polymorphic over TxOut for each era when destructing using HasField, or in functions like decodeDataHash32 which just accept the parameters of the constructor as arguments. there are other techniques which don't change the semantics of TxOut

goolord · 2021-11-24T20:10:19Z

there is still a maintainership burden but it seems small, I don't really have any strong opinions either way

lehins

I think we need a test suite added in this PR that at least checks serialization rountripping. It would have caught most of my comments.

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs

lehins · 2021-11-30T12:47:45Z

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs

+data TxBodyRaw era = TxBodyRaw
+  { _inputs :: !(Set (TxIn (Crypto era))),
+    _collateral :: !(Set (TxIn (Crypto era))),
+    _collateralReturn :: !(StrictMaybe (TxOut era)),
+    _outputs :: !(StrictSeq (TxOut era)),
+    _certs :: !(StrictSeq (DCert (Crypto era))),
+    _wdrls :: !(Wdrl (Crypto era)),
+    _txfee :: !Coin,
+    _vldt :: !ValidityInterval,
+    _update :: !(StrictMaybe (Update era)),
+    _reqSignerHashes :: Set (KeyHash 'Witness (Crypto era)),
+    _mint :: !(Value (Crypto era)),
+    -- The spec makes it clear that the mint field is a
+    -- Cardano.Ledger.Mary.Value.Value, not a Core.Value.
+    -- Operations on the TxBody in the AlonzoEra depend upon this.
+    _scriptIntegrityHash :: !(StrictMaybe (ScriptIntegrityHash (Crypto era))),
+    _adHash :: !(StrictMaybe (AuxiliaryDataHash (Crypto era))),
+    _txnetworkid :: !(StrictMaybe Network)
+  }
+  deriving (Generic, Typeable)


@JaredCorduan or @nc6 can you comment on this? I think we could do something like this instead?

Suggested change

data TxBodyRaw era = TxBodyRaw

{ _inputs :: !(Set (TxIn (Crypto era))),

_collateral :: !(Set (TxIn (Crypto era))),

_collateralReturn :: !(StrictMaybe (TxOut era)),

_outputs :: !(StrictSeq (TxOut era)),

_certs :: !(StrictSeq (DCert (Crypto era))),

_wdrls :: !(Wdrl (Crypto era)),

_txfee :: !Coin,

_vldt :: !ValidityInterval,

_update :: !(StrictMaybe (Update era)),

_reqSignerHashes :: Set (KeyHash 'Witness (Crypto era)),

_mint :: !(Value (Crypto era)),

-- The spec makes it clear that the mint field is a

-- Cardano.Ledger.Mary.Value.Value, not a Core.Value.

-- Operations on the TxBody in the AlonzoEra depend upon this.

_scriptIntegrityHash :: !(StrictMaybe (ScriptIntegrityHash (Crypto era))),

_adHash :: !(StrictMaybe (AuxiliaryDataHash (Crypto era))),

_txnetworkid :: !(StrictMaybe Network)

}

deriving (Generic, Typeable)

data TxBodyRaw era = TxBodyRaw

{ txBodyAlonzo :: Alonzo.TxBodyRaw era

, txBodyCollateralReturn :: !(StrictMaybe (TxOut era))

}

This way we could reuse all the serialization/deserialization logic from alonzo. All this copy pasting is hurting my eyes. I will of course not stand in a way if that is how era parameterization was designed to be used.

hmm. I like the idea of removing code copy when we can, but it's also weird to switch to this nested format now.

We could have the datatype defined like this, and unpack in the patterns.

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs

goolord · 2021-11-30T16:02:50Z

yeah i neglected to introduce the test suite yet since it would be a lot of additional work to do so until the rest of the babbage era stuff is done, but i can add tests gradually

goolord · 2021-12-01T23:04:11Z

ok, adding tests at this point would actually be way more work than i thought. if it's alright i will fix what i can now and revisit the tests / correctness of this module later just to avoid adding way too much to this pr

JaredCorduan · 2021-12-02T18:03:31Z

ok, adding tests at this point would actually be way more work than i thought. if it's alright i will fix what i can now and revisit the tests / correctness of this module later just to avoid adding way too much to this pr

work for me

JaredCorduan · 2021-12-06T13:53:08Z

I think we need a test suite added in this PR that at least checks serialization rountripping. It would have caught most of my comments.

That's a good idea. @goolord I think it would be pretty easy to add a few round trip tests, you can reuse the existing tools, like here in Alonzo: https://github.com/input-output-hk/cardano-ledger/blob/e7b7773655f6f1acc600abd63a3de1c6599c6d98/eras/alonzo/test-suite/test/Test/Cardano/Ledger/Alonzo/Serialisation/Tripping.hs#L70-L104

I'm also fine with adding tests in a follow up PR.

goolord · 2021-12-22T15:54:55Z

ok, i got the whole era working on a branch and the tripping tests passing, i'll push the correct version of this module after I pr some of the related changes i needed to make

goolord · 2021-12-22T16:18:30Z

#2593

…ecs into zachc/babbage

goolord · 2021-12-28T17:44:49Z

ok, this is up to date with the latest version of the spec, and has changes from this branch https://github.com/input-output-hk/cardano-ledger/tree/zachc/babbage-big which has the test suite running (+ most of the other babbage changes which i'm goin to port over)

this pr #2593 is necessary for the module to actually be useful

goolord · 2021-12-28T18:59:43Z

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs

+  toCBOR (TxOutCompactDatum addr cv d) =
+    encodeListLen 4
+      <> toCBOR True
+      <> toCBOR addr
+      <> toCBOR cv
+      <> toCBOR d


this feels kinda hacky, but putting a break before the encodeListLen makes tests fail

JaredCorduan

Looks good to me! This has all the new features I was expecting.

…to zachc/babbage

lehins

LGTM. One minor suggestion.

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs

Co-authored-by: Alexey Kuleshevich <[email protected]>

implemented suggestions

* add babbage era, TxBody and TxOut * update collateralReturn * reuse Alonzo code, update TxOut * revert pattern synonym change * WIP * correct FromCBOR instance * Update eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs Co-authored-by: Alexey Kuleshevich <[email protected]> Co-authored-by: Alexey Kuleshevich <[email protected]>

goolord force-pushed the zachc/babbage branch from 1ef8d19 to e912f6b Compare November 23, 2021 20:19

nc6 previously requested changes Nov 24, 2021

View reviewed changes

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs Outdated Show resolved Hide resolved

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs Show resolved Hide resolved

lehins self-requested a review November 24, 2021 18:49

lehins requested changes Nov 24, 2021

View reviewed changes

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs Outdated Show resolved Hide resolved

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs Outdated Show resolved Hide resolved

goolord force-pushed the zachc/babbage branch 2 times, most recently from afc66c2 to 80e73e4 Compare November 28, 2021 03:23

lehins requested changes Nov 30, 2021

View reviewed changes

goolord added 4 commits December 1, 2021 17:40

add babbage era, TxBody and TxOut

2f7528c

update collateralReturn

f1a7738

reuse Alonzo code, update TxOut

b7b4b56

revert pattern synonym change

2e2cfdc

goolord force-pushed the zachc/babbage branch from 18d2239 to 615e9ce Compare December 2, 2021 00:17

WIP

066c16c

goolord force-pushed the zachc/babbage branch from 615e9ce to 066c16c Compare December 2, 2021 06:20

goolord mentioned this pull request Dec 22, 2021

weaken FromCBOR for TxOut to Annotator TxOut #2593

Merged

Merge branch 'master' of github.com:input-output-hk/cardano-ledger-sp…

fddb47e

…ecs into zachc/babbage

goolord force-pushed the zachc/babbage branch 3 times, most recently from a128e6f to 1a6b48b Compare December 28, 2021 18:00

goolord commented Dec 28, 2021

View reviewed changes

goolord force-pushed the zachc/babbage branch from 1a6b48b to 4d2e1d1 Compare December 28, 2021 21:47

correct FromCBOR instance

4d2e1d1

JaredCorduan approved these changes Jan 3, 2022

View reviewed changes

Merge branch 'master' of github.com:input-output-hk/cardano-ledger in…

e2367dd

…to zachc/babbage

lehins approved these changes Jan 4, 2022

View reviewed changes

eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs Outdated Show resolved Hide resolved

Update eras/babbage/impl/src/Cardano/Ledger/Babbage/TxBody.hs

e4264dd

Co-authored-by: Alexey Kuleshevich <[email protected]>

goolord merged commit b37412b into master Jan 4, 2022

iohk-bors bot deleted the zachc/babbage branch January 4, 2022 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Babbage: TxBody + TxOut #2560

Babbage: TxBody + TxOut #2560

goolord commented Nov 23, 2021

nc6 left a comment

lehins left a comment

goolord commented Nov 24, 2021

goolord commented Nov 24, 2021

lehins left a comment

lehins Nov 30, 2021

JaredCorduan Dec 6, 2021

nc6 Dec 6, 2021

goolord commented Nov 30, 2021

goolord commented Dec 1, 2021 •

edited

Loading

JaredCorduan commented Dec 2, 2021

JaredCorduan commented Dec 6, 2021 •

edited

Loading

goolord commented Dec 22, 2021

goolord commented Dec 22, 2021

goolord commented Dec 28, 2021

goolord Dec 28, 2021

JaredCorduan left a comment

lehins left a comment

Babbage: TxBody + TxOut #2560

Babbage: TxBody + TxOut #2560

Conversation

goolord commented Nov 23, 2021

nc6 left a comment

Choose a reason for hiding this comment

lehins left a comment

Choose a reason for hiding this comment

goolord commented Nov 24, 2021

goolord commented Nov 24, 2021

lehins left a comment

Choose a reason for hiding this comment

lehins Nov 30, 2021

Choose a reason for hiding this comment

JaredCorduan Dec 6, 2021

Choose a reason for hiding this comment

nc6 Dec 6, 2021

Choose a reason for hiding this comment

goolord commented Nov 30, 2021

goolord commented Dec 1, 2021 • edited Loading

JaredCorduan commented Dec 2, 2021

JaredCorduan commented Dec 6, 2021 • edited Loading

goolord commented Dec 22, 2021

goolord commented Dec 22, 2021

goolord commented Dec 28, 2021

goolord Dec 28, 2021

Choose a reason for hiding this comment

JaredCorduan left a comment

Choose a reason for hiding this comment

lehins left a comment

Choose a reason for hiding this comment

goolord commented Dec 1, 2021 •

edited

Loading

JaredCorduan commented Dec 6, 2021 •

edited

Loading