-
Notifications
You must be signed in to change notification settings - Fork 166
Remove listLenBound from EncCBORGroup class #5383
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
libs/cardano-ledger-binary/src/Cardano/Ledger/Binary/Decoding/Coders.hs
Outdated
Show resolved
Hide resolved
cb5da85 to
bb92847
Compare
|
CI seems happy so I think this is good for another look |
lehins
left a comment
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.
There is really only one suggestion that I have, which I repeated multiple times. Other than that it looks great!
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/Annotator.hs
Outdated
Show resolved
Hide resolved
libs/cardano-protocol-tpraos/src/Cardano/Protocol/TPraos/OCert.hs
Outdated
Show resolved
Hide resolved
libs/cardano-protocol-tpraos/src/Cardano/Protocol/TPraos/OCert.hs
Outdated
Show resolved
Hide resolved
libs/cardano-protocol-tpraos/src/Cardano/Protocol/TPraos/BHeader.hs
Outdated
Show resolved
Hide resolved
3a4f5c1 to
193bf38
Compare
| fromStrictMaybeL = lens strictMaybeToMaybe (const maybeToStrictMaybe) | ||
|
|
||
| toProxy :: forall a. a -> Proxy a | ||
| toProxy _ = Proxy |
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.
@lehins asProxyTypeOf was not what we needed, so I have defined this toProxy here. I'm pretty sure this is not the right place, so let's figure out where it needs to move or if it should be defined in every module where we need it
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.
Yes, I totally agree with the fact that this is a better function for us than asProxyTypeOf. I tend to avoid defining things that are already available in some form in base, even if they are slightly worse then what I need.
The reason why I avoid it is because I am too lazy. 😅
There are two very hard problems that needs to be solved when one creates a generally reusable function like this 🤣
- Finding a good name
- Finding where to put it
The irony about this function, is that I have defined a function just like that in the past toProxy, which doesn't really help us here, but still funny 🥲
In order to stay consistent with the name in base, I think it would be better to call this one asProxy instead. And, as of recent developments, there is a really good place where we can put it, we created a package cardano-base.
With all this in mind, I think it would be a prefect time for you to make this small contribution in a place other than ledger. So, please:
- add a module
Cardano.Base.Proxyincardano-basepackage. - add this function there and re-export all of the stuff from
Data.Proxy. - make a release of that package to CHaP and tag me on the RP for approval
- Update index-state and nix flake in cardano-ledger, once release has been merged and start using this
asProxyfunction in this PR.
FTR. We don't normally go through all these troubles just to get one function form some place. I am only suggesting you do this to get familiar with the process of making release to CHaP and pulling in newer dependencies in
| groupRecord :: forall a s. (EncCBORGroup a, DecCBORGroup a) => Decoder s a | ||
| groupRecord = decodeRecordNamed "CBORGroup" (fromIntegral . toInteger . listLen) decCBORGroup | ||
| groupRecord = | ||
| decodeRecordNamed "CBORGroup" (fromIntegral . toInteger . const (listLen $ Proxy @a)) decCBORGroup |
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.
Looks like I missed one more suggestion. Using Proxy @a would have been fine, it is the fromIntegral . toInteger that is totally unnecessary here, which was there before this PR.
| decodeRecordNamed "CBORGroup" (fromIntegral . toInteger . const (listLen $ Proxy @a)) decCBORGroup | |
| decodeRecordNamed "CBORGroup" (listLenInt . (`asProxyTypeOf` Proxy)) decCBORGroup |
| fromStrictMaybeL = lens strictMaybeToMaybe (const maybeToStrictMaybe) | ||
|
|
||
| toProxy :: forall a. a -> Proxy a | ||
| toProxy _ = Proxy |
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.
Yes, I totally agree with the fact that this is a better function for us than asProxyTypeOf. I tend to avoid defining things that are already available in some form in base, even if they are slightly worse then what I need.
The reason why I avoid it is because I am too lazy. 😅
There are two very hard problems that needs to be solved when one creates a generally reusable function like this 🤣
- Finding a good name
- Finding where to put it
The irony about this function, is that I have defined a function just like that in the past toProxy, which doesn't really help us here, but still funny 🥲
In order to stay consistent with the name in base, I think it would be better to call this one asProxy instead. And, as of recent developments, there is a really good place where we can put it, we created a package cardano-base.
With all this in mind, I think it would be a prefect time for you to make this small contribution in a place other than ledger. So, please:
- add a module
Cardano.Base.Proxyincardano-basepackage. - add this function there and re-export all of the stuff from
Data.Proxy. - make a release of that package to CHaP and tag me on the RP for approval
- Update index-state and nix flake in cardano-ledger, once release has been merged and start using this
asProxyfunction in this PR.
FTR. We don't normally go through all these troubles just to get one function form some place. I am only suggesting you do this to get familiar with the process of making release to CHaP and pulling in newer dependencies in
|
|
||
| listLenInt :: EncCBORGroup a => a -> Int | ||
| listLenInt x = fromIntegral (listLen x) | ||
| listLenInt :: forall a. EncCBORGroup a => a -> Int |
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.
Why not a Proxy here too, instead of a value a? Isn't it misleading like this, as if the value is taken into consideration?
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.
Good point - though I find that not having to pass the proxy is half of the convenience of this function: it then becomes fromIntegral . listLen, and I think I'd rather just remove it at that point?
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 both of you. That's why I think the best approach here will be to make it:
| listLenInt :: forall a. EncCBORGroup a => a -> Int | |
| listLenInt :: forall proxy a. EncCBORGroup a => proxy a -> Int |
This will allow using it with concrete types instead of just Proxy, e.g.
Plain.decodeRecordNamed "OCert" (listLenInt . Just) decodeOCertFieldsor Identity a, if Maybe a feels too hacky:
Plain.decodeRecordNamed "OCert" (listLenInt . Identity) decodeOCertFieldsThis will keep it consistent, while still allow for convenience to use the value for inferring the type.
Description
Fix #5372
Checklist
CHANGELOG.mdfiles updated for packages with externally visible changes.NOTE: New section is never added with the code changes. (See RELEASING.md).
.cabalandCHANGELOG.mdfiles when necessary, according to theversioning process.
.cabalfiles updated when necessary.NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
scripts/fourmolize.sh).scripts/cabal-format.sh).scripts/gen-cddl.sh)hie.yamlupdated (usescripts/gen-hie.sh).