-
Notifications
You must be signed in to change notification settings - Fork 157
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
weaken FromCBOR
for TxOut
to Annotator TxOut
#2593
Conversation
9147c3d
to
6095478
Compare
6095478
to
086baff
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 highly recommend avoiding all the copy pasting. Most of the code added in this PR can be avoided. For example:
instance ... => FromCBOR (Annotator (EpochState era)) where
fromCBOR = do
es@EpochState {eslState} <- fromCBOR
pure $ Annotator $ \fbs -> es {esLState = runAnnotator esLState fbs}
It is especially dangerous to copy serialization/deserialziation code like that, because it s possible that someone in the future will change one fromCBOR
, but not the other other one.
Also it is very bad practice to copy paste code in general, it is almost always better to create an abstraction. A professor of mine said once: "If you are finding yourself copying a piece of code, it is most likely that you are doing something wrong."
So please, keep your code as DRY as possible.
I don't think the code in your example would typecheck. since the constraints are different the instances have to be duplicated, or we can remove the non-Annotator FromCBOR instances, but not both |
actually, I just realized that if |
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.
Adding those annotators do tend to infect a lot of code, thanks for plowing through this! LGTM
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 looks great. @goolord thank you for reducing duplication!
There are just a couple of minor suggestions that I have in this review.
co-authored-by: Alexey Kuleshevich <[email protected]>
8b680a7
to
4f3d300
Compare
``` Case Max MaxOS Live Allocated GCs Wall Time EpochState (FromCBOR) 1,739,105,216 5,082,447,872 1,724,424,656 103,862,408,816 93,015 99.967s Benchmark memory: FINISH ```
required for #2560
babbage's
TxOut
will only be an instance ofFromCBOR (Annotator TxOut)
, so this pr adds instances forFromCBOR (Annotator..
as it bubbles up fromTxOut
andFromCBOR
constraints where necessary in code that's reused in babbage