Mint Websockets (NUT-17) #394

2024-10-08T13:57:09Z

crodas commented

2024-10-08 13:57:09 +00:00

(Migrated from github.com)

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 (6fafbfdd70158f2c5f40a65e5505d7719379fa60)

Very early stage nonfunctional prototype to add support for any subscriptions, aiming for NUT-17 support, #203 . TODO: * [x] Write tests for event generation inside the existing Mint tests (84b2a6a) * [x] Write unit tests for the websocket client and external tests (if possible) * [x] Does it worth to make it opt-in or possible to disable subscriptions? * [x] Add [Signaling Support via NUT-06](https://github.com/cashubtc/nuts/blob/main/17.md#signaling-support-via-nut-06) (6fafbfdd70158f2c5f40a65e5505d7719379fa60)

thesimplekid commented

2024-10-10 06:48:56 +00:00

(Migrated from github.com)

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 commented

2024-10-10 15:33:12 +00:00

(Migrated from github.com)

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

thesimplekid (Migrated from github.com) requested changes

2024-10-23 15:01:27 +00:00

thesimplekid (Migrated from github.com) left a comment

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

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`

crates/cdk-axum/src/ws/mod.rs

					
				@ -0,0 +1,123 @@

				use crate::MintState;

    loop {
        select! {
            Some((sub_id, payload)) = subscriber.recv() => {
                if !context.subscriptions.contains_key(&sub_id) {
                    continue;
                }
                let notification: WsNotification<Notification> = (sub_id, payload).into();
                let message = match serde_json::to_string(&notification) {
                    Ok(message) => message,
                    Err(_) => {
                        tracing::error!("Could not serialize notification");
                        continue;
                    }
                };
                if socket.send(Message::Text(message)).await.is_err() {
                    break;
                }
            }
            Some(Ok(msg)) = socket.next() => {
                match msg {
                    Message::Text(text) => {
                        let request = match serde_json::from_str::<WsRequest>(&text) {
                            Ok(request) => request,
                            Err(err) => {
                                tracing::error!("Could not parse request: {}", err);
                                continue;
                            }
                        };
                        match request.method.process(request.id, &mut context).await {
                            Ok(result) => {
                                if socket.send(Message::Text(result.to_string())).await.is_err() {
                                    break;
                                }
                            }
                            Err(err) => {
                                tracing::error!("Error serializing response: {}", err);
                                break;
                            }
                        }
                    }
                    Message::Close(_) => {
                        tracing::info!("WebSocket closed");
                        break;
                    }
                    _ => {}
                }
            }
            else => {
                tracing::info!("WebSocket closed");
                break;
            }
        }
    }

Think I would prefer select usage here

```suggestion loop { select! { Some((sub_id, payload)) = subscriber.recv() => { if !context.subscriptions.contains_key(&sub_id) { continue; } let notification: WsNotification<Notification> = (sub_id, payload).into(); let message = match serde_json::to_string(&notification) { Ok(message) => message, Err(_) => { tracing::error!("Could not serialize notification"); continue; } }; if socket.send(Message::Text(message)).await.is_err() { break; } } Some(Ok(msg)) = socket.next() => { match msg { Message::Text(text) => { let request = match serde_json::from_str::<WsRequest>(&text) { Ok(request) => request, Err(err) => { tracing::error!("Could not parse request: {}", err); continue; } }; match request.method.process(request.id, &mut context).await { Ok(result) => { if socket.send(Message::Text(result.to_string())).await.is_err() { break; } } Err(err) => { tracing::error!("Error serializing response: {}", err); break; } } } Message::Close(_) => { tracing::info!("WebSocket closed"); break; } _ => {} } } else => { tracing::info!("WebSocket closed"); break; } } } ``` Think I would prefer select usage here

crates/cdk-mintd/src/main.rs

This is only run at mint start up so there will be no ws subs at this point so we dont need to send notifications here.

crates/cdk/src/mint/melt.rs

Since only the mint knows the quote id at this point and it hasn't been returned, its not possible for anyone to have subscribed to updates for it so we don't need to seed ws updates here.

```suggestion ``` Since only the mint knows the quote id at this point and it hasn't been returned, its not possible for anyone to have subscribed to updates for it so we don't need to seed ws updates here.

crates/cdk/src/mint/melt.rs

In addition to sending an update when it is paid as is done here, we also need to send one when the quote is pending, think this is best done in the verify_melt_request fn above. In both cases we also need to send updates for the proof states when those are moved to pending and then spent.

In addition to sending an update when it is paid as is done here, we also need to send one when the quote is pending, think this is best done in the `verify_melt_request` fn above. In both cases we also need to send updates for the proof states when those are moved to pending and then spent.

crodas (Migrated from github.com) reviewed

2024-10-23 15:06:34 +00:00

crates/cdk-axum/src/ws/mod.rs

					
				@ -0,0 +1,123 @@

				use crate::MintState;

I'm also familiar with tokio::select! but there was a clippy warning, so I learned how it was done in the earlier days. I'll gladly switch to tokio::select!.

I'm also familiar with `tokio::select!` but there was a clippy warning, so I learned how it was done in the earlier days. I'll gladly switch to `tokio::select!`.

crodas (Migrated from github.com) reviewed

2024-10-23 15:20:11 +00:00

crates/cdk-axum/src/ws/mod.rs

					
				@ -0,0 +1,123 @@

				use crate::MintState;

Fixed in e30f732025f88ae9fb7a22ed704453523aa042fa

thesimplekid (Migrated from github.com) reviewed

2024-10-23 16:11:02 +00:00

crates/cdk-axum/src/ws/mod.rs

					
				@ -0,0 +1,123 @@

				use crate::MintState;

Yeah for some reason clippy thinks it breaks msrv when it doesn't, so we can use #[allow(clippy::incompatible_msrv)] to suppress the warning

Yeah for some reason clippy thinks it breaks msrv when it doesn't, so we can use ` #[allow(clippy::incompatible_msrv)] ` to suppress the warning

crodas (Migrated from github.com) reviewed

2024-10-28 16:09:01 +00:00

crates/cdk/src/mint/melt.rs

Fixed in 101b1c5

crodas (Migrated from github.com) reviewed

2024-10-28 16:10:35 +00:00

crates/cdk/src/mint/melt.rs

Fixed in d7f5791

thesimplekid (Migrated from github.com) requested changes

2024-10-29 08:58:57 +00:00

thesimplekid (Migrated from github.com) left a comment

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.

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 commented

2024-10-29 12:31:17 +00:00

(Migrated from github.com)

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.

> 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.

thesimplekid (Migrated from github.com) requested changes

2024-11-04 16:34:56 +00:00

thesimplekid (Migrated from github.com) left a comment

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

crates/cdk-axum/src/ws/mod.rs

					
				@ -0,0 +1,123 @@

				use crate::MintState;

          if let Err(err)= socket.send(Message::Text(message)).await {
                   tracing::error!("Could not send websocket message: {}", err);
                     break;
          }

What do you think of using an if let to log the error here?

```suggestion if let Err(err)= socket.send(Message::Text(message)).await { tracing::error!("Could not send websocket message: {}", err); break; } ``` What do you think of using an if let to log the error here?

Same as above use an If let to log the error?

crates/cdk-axum/src/ws/subscribe.rs

					
				@ -0,0 +14,4 @@

				pub struct Response {

				    status: String,

				    #[serde(rename = "subId")]

				    sub_id: SubId,

    #[serde(rename = "subId")]  
    sub_id: SubId,

```suggestion #[serde(rename = "subId")] sub_id: SubId, ```

crates/cdk-axum/src/ws/unsubscribe.rs

					
				@ -0,0 +1,30 @@

				use super::{handler::WsHandle, WsContext, WsError};

pub struct Method {
    #[serde(rename = "subId")]
    pub sub_id: SubId,
}

sub id should be snake case

```suggestion pub struct Method { #[serde(rename = "subId")] pub sub_id: SubId, } ``` sub id should be snake case

crates/cdk-integration-tests/tests/regtest.rs

        r#"{"jsonrpc":"2.0","result":{"status":"OK","subId":"test-sub"},"id":2}"#

```suggestion r#"{"jsonrpc":"2.0","result":{"status":"OK","subId":"test-sub"},"id":2}"# ```

crates/cdk/src/nuts/nut17.rs

					
				@ -0,0 +1,333 @@

				//! Specific Subscription for the cdk crate

Should we also have a proof state helper here for constancy?

crates/cdk/src/pub_sub/mod.rs

				`@ -0,0 +1,311 @@`
				`//! Publish–subscribe pattern.`

    /// This task will run in the background (and will be dropped when the [`Manager`]
    /// is) and will remove subscriptions from the storage struct it is dropped.

```suggestion /// This task will run in the background (and will be dropped when the [`Manager`] /// is) and will remove subscriptions from the storage struct it is dropped. ```

thesimplekid (Migrated from github.com) reviewed

2024-11-04 16:43:06 +00:00

crates/cdk/src/nuts/nut17.rs

					
				@ -0,0 +1,333 @@

				//! Specific Subscription for the cdk crate

We have from MeltQuote in nut05, lets move this to NUT05.

github.com/cashubtc/cdk@d9fb5f814a/crates/cdk/src/nuts/nut05.rs (L198-L213)

We have from MeltQuote in nut05, lets move this to NUT05. https://github.com/cashubtc/cdk/blob/d9fb5f814a745098c8a3c4ac8784ce2d28f52139/crates/cdk/src/nuts/nut05.rs#L198-L213

thesimplekid (Migrated from github.com) reviewed

2024-11-04 16:43:48 +00:00

crates/cdk/src/nuts/nut17.rs

					
				@ -0,0 +1,333 @@

				//! Specific Subscription for the cdk crate

We have this in NUT04 lets move this there.

github.com/cashubtc/cdk@d9fb5f814a/crates/cdk/src/nuts/nut04.rs (L95-L105)

We have this in NUT04 lets move this there. https://github.com/cashubtc/cdk/blob/d9fb5f814a745098c8a3c4ac8784ce2d28f52139/crates/cdk/src/nuts/nut04.rs#L95-L105

crodas (Migrated from github.com) reviewed

2024-11-04 22:33:23 +00:00

crates/cdk/src/pub_sub/mod.rs

				`@ -0,0 +1,311 @@`
				`//! Publish–subscribe pattern.`

Thank you for fixing my broken English

Thank you for fixing my broken English ![borat-borat-very-nice](https://github.com/user-attachments/assets/3d99acbb-28b3-4545-a754-24edf4f28cdc)

crodas (Migrated from github.com) reviewed

2024-11-05 21:11:17 +00:00

crates/cdk/src/nuts/nut17.rs

					
				@ -0,0 +1,333 @@

				//! Specific Subscription for the cdk crate

Fixed in fcae992

Fixed in fcae992

crodas (Migrated from github.com) reviewed

2024-11-05 21:11:37 +00:00

crates/cdk/src/nuts/nut17.rs

					
				@ -0,0 +1,333 @@

				//! Specific Subscription for the cdk crate

Fixed in fcae992

Fixed in fcae992

crodas (Migrated from github.com) reviewed

2024-11-05 21:11:40 +00:00

crates/cdk/src/nuts/nut17.rs

					
				@ -0,0 +1,333 @@

				//! Specific Subscription for the cdk crate

Fixed in fcae992

Fixed in fcae992

thesimplekid (Migrated from github.com) reviewed

2024-11-05 22:06:29 +00:00

crates/cdk-axum/src/lib.rs

Think the merge got messed up here this route shouldn't be deleted

crodas (Migrated from github.com) reviewed

2024-11-05 22:22:44 +00:00

crates/cdk-axum/src/lib.rs

Fixed in 9d99d6d

Fixed in [9d99d6d](https://github.com/cashubtc/cdk/pull/394/commits/9d99d6daa007cdc32a2fc579b8527597a15eb97b)

thesimplekid (Migrated from github.com) reviewed

2024-11-05 22:40:13 +00:00

crates/cdk/src/mint/mint_nut04.rs

					
				@ -295,6 +300,7 @@ impl Mint {

				                .update_mint_quote_state(&mint_request.quote, MintQuoteState::Paid)

I'm not sure that we should send the state here, to the wallet this is not a change in state as the quote was already paid as there is not pending state for the mint quote on the wallet. So they would have already received the paid quote update above in the pay_mint_quote_for_request_id. The reason we need to update it in the database is we set it to pending to avoid a race condition where a wallet attempts to mint twice at the same time but this pending state isnt known to the wallet. See how minting in nutshell gets a duplicate message.

I'm not sure that we should send the state here, to the wallet this is not a change in state as the quote was already paid as there is not `pending` state for the mint quote on the wallet. So they would have already received the paid quote update above in the `pay_mint_quote_for_request_id`. The reason we need to update it in the database is we set it to pending to avoid a race condition where a wallet attempts to mint twice at the same time but this pending state isnt known to the wallet. See how minting in nutshell gets a duplicate message. ![image](https://github.com/user-attachments/assets/dfc0dbd3-f3d9-4e9b-8b0d-fba1b249f392)

crodas (Migrated from github.com) reviewed

2024-11-06 13:21:53 +00:00

crates/cdk/src/mint/mint_nut04.rs

					
				@ -295,6 +300,7 @@ impl Mint {

				                .update_mint_quote_state(&mint_request.quote, MintQuoteState::Paid)

Fixed in 54f5ab9

Fixed in 54f5ab9

thesimplekid (Migrated from github.com) approved these changes

2024-11-06 14:35:44 +00:00

thesimplekid (Migrated from github.com) left a comment

Great work thanks for this.

Sign in to join this conversation.

No reviewers

thesimplekid

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

cashubtc/cdk!394

				`@ -0,0 +1,30 @@`
				`use super::{handler::WsHandle, WsContext, WsError};`

				`@ -0,0 +1,333 @@`
				`//! Specific Subscription for the cdk crate`

				`@ -295,6 +300,7 @@ impl Mint {`
				`.update_mint_quote_state(&mint_request.quote, MintQuoteState::Paid)`

Rows
Columns