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.

Sign up for GitHub

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

Jump to bottom

Reading V4 Token #144

Merged
merged 8 commits into from
Jul 18, 2024
Merged

Reading V4 Token #144

merged 8 commits into from
Jul 18, 2024

Conversation

Egge21M
Copy link
Collaborator

@Egge21M Egge21M commented Jun 28, 2024 •
edited
Loading

Fixes: None

Description

This PR changes the way handleTokens works. It also adds reading the newly proposed v4 Token format. V4 token are parsed and mapped to output the exact same structure as v3 Token to make handling easier. Consumers of getDecodedToken receive a stable result.

Changes

  • Removed deprecated token format support (pre v3)
  • Removed test cases for deprecated token formats
  • handleTokens now parses the version and decodes accordingly.

PR Tasks

  • Open PR
  • Refactoring and remove logs
  • run npm run test --> no failing unit tests
  • run npm run format

@Egge21M Egge21M force-pushed the tokenV4 branch 2 times, most recently from 0269245 to 19b9b0e Compare June 30, 2024 10:42
@Egge21M Egge21M marked this pull request as ready for review July 1, 2024 13:37
gudnuf
gudnuf reviewed Jul 1, 2024
View reviewed changes
src/utils.ts Outdated Show resolved Hide resolved
gudnuf
gudnuf approved these changes Jul 1, 2024
View reviewed changes
Copy link
Collaborator

@gudnuf gudnuf left a comment

Choose a reason for hiding this comment

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

lgtm! 233b8ba
Thanks for adding cbor directly to this lib, I've had issues in the past with cbor and server-side rendering, but just tested in a next.js app and works great!

@Egge21M
Copy link
Collaborator Author

Egge21M commented Jul 1, 2024

lgtm! 233b8ba Thanks for adding cbor directly to this lib, I've had issues in the past with cbor and server-side rendering, but just tested in a next.js app and works great!

Great to hear that! Please be aware that this implementation is very limited in scope and was only tested against token v4 specific cases. There might be edge cases that are not covered

@Egge21M Egge21M requested a review from gandlafbtc July 1, 2024 22:05
@Egge21M
Copy link
Collaborator Author

Egge21M commented Jul 4, 2024

FYI this is rebased to the new dev branch and ready for a release in 1.X

@callebtc
Copy link
Contributor

Amazing work, especially implementing the cbor de/serializer.

@minibits-cash minibits-cash mentioned this pull request Jul 17, 2024
Closed
@Egge21M Egge21M requested a review from callebtc July 18, 2024 11:41
callebtc
callebtc approved these changes Jul 18, 2024
View reviewed changes
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.

ut-LGTM

@Egge21M Egge21M merged commit c68de1a into cashubtc:development Jul 18, 2024
1 check passed
@prusnak prusnak mentioned this pull request Aug 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gudnuf gudnuf gudnuf approved these changes

@callebtc callebtc callebtc approved these changes

@gandlafbtc gandlafbtc Awaiting requested review from gandlafbtc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Egge21M @callebtc @gudnuf