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

Add spec for binary token serialization #199

Merged
merged 8 commits into from
Dec 12, 2024
Merged

Conversation

Egge21M
Copy link
Contributor

@Egge21M Egge21M commented Nov 28, 2024

This adds some standardization for upcoming raw / binary token serialization, simply prepending the token with a identifier, as well as a version "byte" and encoding everything using cbor.

@prusnak
Copy link
Collaborator

prusnak commented Nov 28, 2024

I am not a big fan of this proposal because it requires cbor dependency for future versions which might not be based on cbor.

IMO much better encoding is just simple:

CRAW <uint8_t version> <binary data>

(where binary data is the data just before base64 encoding)

That way an app can very simply check whether it understands the payload by checking the first 5 bytes.

Also I think that we should use binary version 03 for v3 of token and 04 for v4 token.

@Egge21M
Copy link
Contributor Author

Egge21M commented Nov 28, 2024

I am not a big fan of this proposal because it requires cbor dependency for future versions which might not be based on cbor.

IMO much better encoding is just simple:

CRAW <uint8_t version> <binary data>

That way an app can very simply check whether it understands the payload by checking the first 5 bytes.

Very good point, however this is meant to be for transports that are binary only. So we should probably do something like

utf8(CRAW) <uint8_t version> <binary data>

00.md Outdated Show resolved Hide resolved
00.md Outdated Show resolved Hide resolved
00.md Outdated Show resolved Hide resolved
00.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

ACK

00.md Outdated Show resolved Hide resolved
00.md Outdated Show resolved Hide resolved
Copy link
Contributor

@callebtc callebtc left a comment

Choose a reason for hiding this comment

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

LGTM

@callebtc callebtc merged commit 77924b6 into cashubtc:main Dec 12, 2024
1 check passed
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