-
Notifications
You must be signed in to change notification settings - Fork 1
fix(rust/catalyst-types): Convert UUID types to cbor #148
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
Conversation
|
✅ Test Report | |
stevenj
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.
I am not loving the existing and new coset types in the UUID types.
The reason being is that coset uses the ciborium library under the hood. This means we end up with two different ways of encoding the exactly same binary stream. Its a maintenance issue.
We settled a while ago on using minicbor for our go-to cbor encode/decode library.
I don't think we should be bringing in cborium or any other cbor encoder into our common libraries.
Its also unnecessary. minicbor can serialize/deserialize from bytes (Vec).
If we are forced to use ciborium then it should be 100% constrained to the crate that brings that requirement in (another topic).
We should be able to use the minicbor encode/decode functions for these types when converting too-from cbor, and just convert the upper type. All this requires is the ability to convert between a (in this case) coset value and vec which it should be under the hood anyway.
I want us to remove coset from the dependencies of this crate, use minicbor only for encoding/decoding to-from vec and the uuid types, so that there is a single source of truth for cbor encoding for these types. If the library we are using does not support minicbor then we should either:
- Use a library that does.
- (if 1 above can not work) Wrap the minicbor encode/decode in the crate which needs the non minicbor encoder and not in the types themselves.
829e1f6 to
60933ea
Compare
|
✅ Test Report | |
|
✅ Test Report | |
rust/catalyst-types/src/uuid/mod.rs
Outdated
| } | ||
|
|
||
| /// Encode `UUID` into `CBOR` | ||
| pub(crate) fn encode_cbor_uuid<W: minicbor::encode::Write>( |
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.
For generality, encoding with a Tag should be optional.
Suggest adding a context tag: bool.
- true = encode with a tag
- false = encode without a tag.
rust/catalyst-types/src/uuid/mod.rs
Outdated
| /// Decode from `CBOR` into `UUID` | ||
| pub(crate) fn decode_cbor_uuid( | ||
| d: &mut Decoder<'_>, (): &mut (), | ||
| ) -> Result<uuid::Uuid, minicbor::decode::Error> { |
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.
For generality, there are three possible options one would want on a decode:
- MUST have a TAG
- MUST not have a TAG
- Optionally has a TAG.
Suggest adding a tagged: Option<bool> as the context.
- None = Optionally has a Tag
- Some(true) = Must have a Tag
- Some(false) = Must NOT have a Tag
83fd117 to
f47590e
Compare
|
✅ Test Report | |
|
✅ Test Report | |
0dd8c98 to
5a376af
Compare
5a376af to
c6b0446
Compare
|
✅ Test Report | |
d7ca046 to
1a430ec
Compare
|
✅ Test Report | |
|
✅ Test Report | |
|
✅ Test Report | |
|
✅ Test Report | |
Mr-Leshiy
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.
Despite one comment LGTM
|
✅ Test Report | |
stevenj
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.
LGTM
stevenj
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.
LGTM
Description
Add conversion from UUID types into CBOR values.
Please confirm the following checks