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

chore: test serde stability in types #295

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

boxdot
Copy link
Collaborator

@boxdot boxdot commented Jan 20, 2025

Structures that are marked with WARNING are tested for stability when serialized to JSON and CBOR formats.

Structures that are marked with WARNING are tested for stability when
serialized to JSON and CBOR formats.
]
}
},
"_phantom": null
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like we have now this in our format. Removing it breaks both JSON and CBOR.

Copy link
Member

@kkohbrok kkohbrok left a comment

Choose a reason for hiding this comment

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

Looks good! This would also be great to do for protocol messages s.t. we know when we need to push a new protocol version.

@boxdot
Copy link
Collaborator Author

boxdot commented Jan 20, 2025

Looks good! This would also be great to do for protocol messages s.t. we know when we need to push a new protocol version.

The question whether we want to switch to protobuf there. In that case, this would be lost effort. For types stored in the database though this is important.

Copy link
Member

@kkohbrok kkohbrok left a comment

Choose a reason for hiding this comment

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

Thanks!

@boxdot boxdot marked this pull request as draft January 20, 2025 15:08
@boxdot
Copy link
Collaborator Author

boxdot commented Jan 20, 2025

Converted to draft because I want to cover all types that implement serde in types.

@boxdot
Copy link
Collaborator Author

boxdot commented Feb 5, 2025

Looks good! This would also be great to do for protocol messages s.t. we know when we need to push a new protocol version.

The question whether we want to switch to protobuf there. In that case, this would be lost effort. For types stored in the database though this is important.

We decided not to switch to protobuf. So, this concern is obsolete now.

@joshsmith
Copy link
Contributor

Looks good! This would also be great to do for protocol messages s.t. we know when we need to push a new protocol version.

The question whether we want to switch to protobuf there. In that case, this would be lost effort. For types stored in the database though this is important.

We decided not to switch to protobuf. So, this concern is obsolete now.

@boxdot Do you mind if I ask why you decided not to switch to protobuf there?

@boxdot
Copy link
Collaborator Author

boxdot commented Feb 11, 2025

@boxdot Do you mind if I ask why you decided not to switch to protobuf there?

The main reason is that many types originate from the OpenMLS implementation and are serialized using the TLS codec. So, if we were to use Protobuf, it would essentially be a thin wrapper containing binary blobs of MLS types. Mixing TLS encoding (TLS codec) with Protobuf in this way feels odd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants