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

Mint Websockets (NUT-17) #394

Merged
merged 10 commits into from
Nov 6, 2024
Merged

Conversation

crodas
Copy link
Contributor

@crodas crodas commented Oct 8, 2024

Very early stage nonfunctional prototype to add support for any subscriptions, aiming for NUT-17 support, #203 .

TODO:

  • Write tests for event generation inside the existing Mint tests (84b2a6a)
  • Write unit tests for the websocket client and external tests (if possible)
  • Does it worth to make it opt-in or possible to disable subscriptions?
  • Add Signaling Support via NUT-06 (6fafbfd)

@crodas crodas force-pushed the nut-17-ws-subscription branch 6 times, most recently from 88cdf9e to 672a4fd Compare October 9, 2024 23:08
@thesimplekid
Copy link
Collaborator

Hey, haven't done a very through review but it looks like a good approach so far, thank you for you work. If there is a specific part you would like me to look at let me know.

@crodas
Copy link
Contributor Author

crodas commented Oct 10, 2024

@thesimplekid I have no idea where the events are being generated, and any other feedback is more than welcome.

@crodas crodas force-pushed the nut-17-ws-subscription branch from 7163235 to 74b9ee4 Compare October 11, 2024 10:29
Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

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

Nice work, think were just missing sending updates in a few places for the proofs some I was able to note in line. This one i couldn't note inline so will note it here, when we swap we need to send an update for the state of the proofs that are spent as part of the swap. Think line 171 of mint/swap.rs

@crodas crodas force-pushed the nut-17-ws-subscription branch from e30f732 to 101b1c5 Compare October 28, 2024 16:08
crodas added a commit to crodas/cdk that referenced this pull request Oct 28, 2024
There are no listeners this early in the program stage[1]

[1] cashubtc#394 (comment)
crodas added a commit to crodas/cdk that referenced this pull request Oct 28, 2024
crodas added a commit to crodas/cdk that referenced this pull request Oct 28, 2024
@crodas crodas force-pushed the nut-17-ws-subscription branch from 78a8b5d to 3395cd1 Compare October 28, 2024 16:35
Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

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

In the verify_melt fn we need to broadcast the proof state and the quote state when they are changed to pending.

I think you have all the other state changes. What else do you feel is missing from this PR before you would like to see it merged?

Does it worth to make it opt-in or possible to disable subscriptions?

Do you mean like as a feature flag? I don't think it is and we should just have the mint always support it.

@crodas
Copy link
Contributor Author

crodas commented Oct 29, 2024

In the verify_melt fn we need to broadcast the proof state and the quote state when they are changed to pending.

I'll make it happen.

I think you have all the other state changes. What else do you feel is missing from this PR before you would like to see it merged?

I want to add an end-to-end test and an external web socket client with rust or typescript. If it is in Rust, it should connect to the WebSocket through TCP, even if using the same process. That's the last missing bit to have peace of mind that it works as expected.

@crodas crodas force-pushed the nut-17-ws-subscription branch from 615bc37 to 72daa0a Compare October 31, 2024 20:39
@crodas crodas changed the title WIP: Nut-17 Nut-17 Oct 31, 2024
…tion

Added a subscription (or pubsub) manager to let subscriber be listening for
events.

This PR also implements a Web Socket endpoint to let external clients to
subscribe to events
@crodas crodas force-pushed the nut-17-ws-subscription branch from 72daa0a to 7d05beb Compare October 31, 2024 20:42
@crodas crodas marked this pull request as ready for review October 31, 2024 20:42
Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

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

Overall really nice work. Just a few small things i noticed when testing vs Nutshell

Co-authored-by: thesimplekid <tsk@thesimplekid.com>
@thesimplekid thesimplekid changed the title Nut-17 Mint Websockets (NUT-17) Nov 4, 2024
* Added more logging
* Added helper to broadcast proof status
* Moved Into and From implementations to their own place
@crodas crodas requested a review from thesimplekid November 5, 2024 22:00
@crodas crodas requested a review from thesimplekid November 5, 2024 22:22
Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

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

Great work thanks for this.

@thesimplekid thesimplekid merged commit 6973e53 into cashubtc:main Nov 6, 2024
43 checks passed
@prusnak prusnak mentioned this pull request Nov 6, 2024
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.

2 participants