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

raw token converters #207

Merged
merged 8 commits into from
Dec 20, 2024
Merged

raw token converters #207

merged 8 commits into from
Dec 20, 2024

Conversation

gandlafbtc
Copy link
Collaborator

Fixes: exposing functions for raw token format

Description

Some transport methods (like nfc) require the lowest amount of data overhead possible. This PR addresses that by externalizing the raw token format (CBOR)

Changes

  • Extracted template converter functions
  • added utility function to convert token to and from raw format
  • expose utility functions on index.ts

PR Tasks

  • Open PR
  • run npm run test --> no failing unit tests
  • run npm run format

Copy link
Collaborator

@Egge21M Egge21M left a comment

Choose a reason for hiding this comment

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

Awesome! I don't think this is specced yet, is it? Blazingly fast! I dont mind getting ahead of the spec, however we need to keep an eye out for any changes.

src/utils.ts Outdated
@@ -521,3 +531,16 @@ export function hasValidDleq(proof: Proof, keyset: MintKeys): boolean {

return true;
}

export function tokenToRawToken(token: string | Token): Uint8Array {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like bait :P
I think we should come up with a proper name for this. We call the the serialised version TokenV4 and the object notation TokenV4Template, so maybe this can be TokenV4Binary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree xD sorry for the bad naming. Yes TokenV4Binary is good

src/utils.ts Outdated
@@ -521,3 +531,16 @@ export function hasValidDleq(proof: Proof, keyset: MintKeys): boolean {

return true;
}

export function tokenToRawToken(token: string | Token): Uint8Array {
if (typeof token === 'string') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer this to only accept token and outsource conversion to the implementer. I think the readability improves if functions only do one (or close to one) thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that makes sense to me. I will change it accordingly

@gandlafbtc gandlafbtc requested a review from Egge21M November 22, 2024 10:29
@Egge21M
Copy link
Collaborator

Egge21M commented Nov 27, 2024

@gandlafbtc is there a spec for this? It seems to me like not having any versioning might be a mistake here.

@callebtc
Copy link
Contributor

@gandlafbtc is there a spec for this? It seems to me like not having any versioning might be a mistake here.

No spec for this afaik but it should be mentioned in NUT-00 that the CBOR structure can be exported without base64 when binary transport is possible. We do however need to come up with a magic byte prefix and a version like "cashuB" if we do that.

@gandlafbtc
Copy link
Collaborator Author

yeah, the structure is the "version 4" structure. but I agree with what @callebtc said. maybe just a version field in the CBOR structure?

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 4, 2024

yeah, the structure is the "version 4" structure. but I agree with what @callebtc said. maybe just a version field in the CBOR structure?

Take a look at this: cashubtc/nuts#199

@prusnak
Copy link
Contributor

prusnak commented Dec 8, 2024

ake a look at this: cashubtc/nuts#199

Yes, let's agree on binary format in described cashubtc/nuts#199 first, merge that PR and only afterwards merge this PR

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 17, 2024

I have adjusted this to match the newly merged binary token spec. Tests are still missing though

@prusnak
Copy link
Contributor

prusnak commented Dec 17, 2024

The code only supports V4 Binary tokens. Should we add the V3 binary tokens support to cashu-ts or should we remove V3 binary tokens from the spec? I am in favor for the latter, but no strong opinions here.

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 17, 2024

The code only supports V4 Binary tokens. Should we add the V3 binary tokens support to cashu-ts or should we remove V3 binary tokens from the spec? I am in favor for the latter, but no strong opinions here.

Good point... Adding them would be easy, but adding them to the spec in the first place was probably a mistake.

Edit: It would make sense to remove them to signal that this format requires modern keyset IDs, just like the regular v4 encoding.

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 18, 2024

I removed the reference to V4 completely now. I also adjusted the tests and exports. This is good to go i'd say

@Egge21M Egge21M merged commit 7bb2936 into development Dec 20, 2024
4 checks passed
@Egge21M Egge21M deleted the raw-token-utils branch December 20, 2024 05:01
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.

4 participants