Reduce overhead of inline datums #2629
Conversation
| -- to the memory overhead of the ledger state. Constructor is intentionally not | ||
| -- exported, in order to prevent invalid creation of data from arbitrary binary | ||
| -- data. Use `makeInlineData` for smart construction. | ||
| newtype InlineData era = InlineData ShortByteString |
There was a problem hiding this comment.
Should this not live in the Babbage codebase? We don't have inline datums in Alonzo
There was a problem hiding this comment.
In order to guarantee type safety Data and InlineData have to live in the same module (note both of their constructors aren't exported). I think it is more important than placing types into eras they were introduced in.
However, if it is the name that bothers you, I can rename it to BinaryData and then it will not be tight into Babbage in any way.
There was a problem hiding this comment.
I sometimes wish Haskell had a concept of "friends", a la C++. But yeah, I wonder whether BinaryData might be a better name, just to avoid confusion.
There was a problem hiding this comment.
Isn't the usual trick to make Cardano.Ledger.Alonzo.Data.Internal, and re-export in Cardano.Ledger.Alonzo.Data? I'm happy either way.
There was a problem hiding this comment.
Using Internal modules is one of the common ways to deal with this, but that doesn't give you the same guarantees as the approach taken in this PR, because Internal modules simply relay responsibility to all programmers, sort of: "if you want to shoot yourself in the foot this is how you do it". While not exporting constructors at all, and instead providing smart constructors (a.k.a Data and makeInlineData in this case) will give hard guarantees that undeserializable InlineDatum cannot be created outside the Data module.
I've renamed it to BinaryData. This will avoid any future confusion and will provide a good insight into what that type means.
There was a problem hiding this comment.
I guess the nice part about "friends" is that you can pick exactly who all gets to shoot themselves in the foot. :)
| @@ -204,12 +211,11 @@ decodeAddress28 stakeRef (Addr28Extra a b c d) = do | |||
| addrHash = | |||
| hashFromPackedBytes $ | |||
| PackedBytes28 a b c (fromIntegral (d `shiftR` 32)) | |||
| pure $ Addr network paymentCred (StakeRefBase stakeRef) | |||
| pure $! Addr network paymentCred (StakeRefBase stakeRef) | |||
There was a problem hiding this comment.
We think this bang could be anything to do with the 1.31 regression?
There was a problem hiding this comment.
I doubt, but I was thinking this too. So, who knows, maybe.
lehins
force-pushed
the
lehins/reduce-overhead-of-inline-datums
branch
from
ca595d7
to
13cc280
Compare
Main purpose of this PR is to introduce
InlineData, which is aShortByteStringof serialized plutusData. It is designed with type safety in mind and by hiding type constructor it is impossible to constructInlineDatafrom binary that doesn't deserialize into valid plutus data.This whole thing is necessary to avoid carrying around both serialized and deserialized version of Data in memory for every inline datum in the utxo.
It also cleans up the rest of usage of
HasField "address", which was started in #2616