-
Notifications
You must be signed in to change notification settings - Fork 25
Use raw bytes representation instead of CBOR for CIP-129 identifier serialisation #937
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
Use raw bytes representation instead of CBOR for CIP-129 identifier serialisation #937
Conversation
8208b60
to
925d98f
Compare
4913e52
to
cdc3dcd
Compare
instance Cip129 (Credential L.ColdCommitteeRole) where | ||
cip129Bech32PrefixFor _ = unsafeHumanReadablePartFromText "cc_cold" | ||
cip129Bech32PrefixesPermitted AsColdCommitteeCredential = ["cc_cold"] | ||
|
||
cip129HeaderHexByte = |
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 should remain. It's explicit and it can be re-used instead of inlining 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.
It's not a very useful class field (?). You still have to duplicate the constants when serialising - there's no easy way to have a two-way mapping between headers to type constructors.
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 prefer explicit over inlining here.
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've pushed a commit reusing headers instead of inlining them.
|
||
-- | CIP-129 decoding errors | ||
data Cip129EncodingError | ||
= CeeTypeDecodingError TypeRep BS.ByteString |
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.
What does Cee stand for?
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.
"Cip Encoding Error". I guess I could use C129ee
but it felt too long and not bringing any additional information over Cee
.
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 just use Cip129
as the prefix? Cee
is ambiguous.
0339def
to
7da9b03
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.
LGTM but read my comment. I think your prior changes were significantly more elegant and we can make the inlining more palatable by defining constants for the header bytes.
-- no decoded response, try codec | ||
(_, Nothing) -> | ||
\case | ||
RawCodec Cip129RawCodec{decodeRaw} -> |
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 is a departure from the simplicity of the changes you made in the previous commit. It looks like you were trying to include the ability to define the Cip129 GovActionId
instance and that's where the trouble begins because it doesn't have a header byte like the other items in Cip129.
In that case, I would stick with your prior commit's changes and just implement values (with haddocks) for the header bytes and reuse those values in the same inlining fashion.
a3e0b4a
to
3b484d7
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.
Nice 👍
3b484d7
to
0a36317
Compare
Changelog
Context
Fixes:
Checklist