Skip to content
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

feat(rust/signed-doc): Add deserializer for Catalyst Signed Documents #101

Merged
merged 82 commits into from
Jan 16, 2025

Conversation

saibatizoku
Copy link
Contributor

@saibatizoku saibatizoku commented Dec 10, 2024

Description

Thanks for contributing to the project!
Please fill out this template to help us review your changes.

Related Issue(s)

Closes input-output-hk/catalyst-voices#1364

Description of Changes

Provide a clear and concise description of what the pull request changes.

Breaking Changes

Describe any breaking changes and the impact.

Screenshots

If applicable, add screenshots to help explain your changes.

Related Pull Requests

If applicable, list any related pull requests.

e.g., #123, #456

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@saibatizoku saibatizoku self-assigned this Dec 10, 2024
Copy link
Contributor

github-actions bot commented Dec 10, 2024

Test Report | ${\color{lightgreen}Pass: 228/228}$ | ${\color{red}Fail: 0/228}$ |

@saibatizoku saibatizoku added enhancement New feature or request do not merge yet PR is not ready to merge yet labels Dec 11, 2024
@saibatizoku saibatizoku changed the title fix(rust/signed_doc): Replace ULID with UUIDv7 fix(rust/signed-doc): Replace ULID with UUIDv7 Dec 11, 2024
@saibatizoku saibatizoku force-pushed the fix/replace-ulid-with-uuidv7 branch 3 times, most recently from 1cd0243 to 96846f0 Compare December 16, 2024 03:03
@saibatizoku saibatizoku changed the title fix(rust/signed-doc): Replace ULID with UUIDv7 feat(rust/signed-doc): Add deserializer for Catalyst Signed Documents Dec 16, 2024
@saibatizoku saibatizoku force-pushed the fix/replace-ulid-with-uuidv7 branch from 8e07f3e to b0aa90a Compare December 17, 2024 15:11
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we are using coset as a library here.
I am not asking that we change this as it would be too much work and we do not have the time, but the problem is, it relies on cborium which we decided earlier that we would not use.
As this crate now has a hard dependency on cborium, any and all decoding issues caused by that need to be 100% constrained to this crate.

Ultimately I would like us to not have any cborium dependency anywhere in our tree, which means we need an issue to stop using coset and either write our own cose encode/decode using minicbor or find another library. I don't think encoding/deciding cose is very challenging, especially as we only have a requirement for COSE_SIGN and none of the other COSE formats.

In future if any crate brings in a CBOR dependency other than minicbor we need to have a discussion before brining it in, as it creates a maintenance issue having multiple ways to encode/decode the same data to/from ultimately the exact same binary stream.

However this does mean that no types we have defined elsewhere can have either a cborium or coset dependency. That needs to be 100% constrained to this crate.

Also note for clarity, we should NOT add Type Encoding here to cbor for any type, we should be encdoing/decoding to/from vec and using minicbor for that, and then just using the appropriate higher level type.
I do not want to see multiple ways to encode/decode any type to the same binary formats because of differing dependent library choices made by upstream crates.

@Mr-Leshiy Mr-Leshiy marked this pull request as ready for review January 16, 2025 10:31
@Mr-Leshiy Mr-Leshiy force-pushed the fix/replace-ulid-with-uuidv7 branch from 98bb6d7 to cd817a4 Compare January 16, 2025 10:34
Copy link
Contributor

Test Report | ${\color{lightgreen}Pass: 260/260}$ | ${\color{red}Fail: 0/260}$ |

Copy link
Contributor

@Mr-Leshiy Mr-Leshiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Mr-Leshiy Mr-Leshiy requested a review from stevenj January 16, 2025 11:18
Copy link
Contributor

Test Report | ${\color{lightgreen}Pass: 260/260}$ | ${\color{red}Fail: 0/260}$ |

Copy link
Contributor

Test Report | ${\color{lightgreen}Pass: 260/260}$ | ${\color{red}Fail: 0/260}$ |

@Mr-Leshiy Mr-Leshiy enabled auto-merge (squash) January 16, 2025 12:06
@Mr-Leshiy Mr-Leshiy merged commit c8810d6 into main Jan 16, 2025
20 of 22 checks passed
@Mr-Leshiy Mr-Leshiy deleted the fix/replace-ulid-with-uuidv7 branch January 16, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet PR is not ready to merge yet enhancement New feature or request F14 review me PR is ready for review
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

🛠️ [TASK] : Make Signed documents library have a basic deserializer
3 participants