Introduce subscription support in the Wallet crate. #473

2024-11-25T20:13:34Z

crodas commented

2024-11-25 20:13:34 +00:00

(Migrated from github.com)

Description

The last bit missing to wrap up #442

The main goal is to add a subscription to CDK Mint updates into the wallet. This feature will be particularly useful for improving the code whenever loops hit the mint server to check status changes.

The goal is to add an easy-to-use interface that will hide the fact that we're connecting to WebSocket and subscribing to events. This will also hide the fact that the CDK-mint server may not support WebSocket updates.

To be fully backward compatible, the HttpClientMethods traits have a new method, subscribe, which will return an object that implements ActiveSubscription.

In the primary implementation, there is a SubscriptionClient that will attempt to connect through WebSocket and will fall to the HTTP-status pull and sleep approach (the current approach), but upper stream code will receive updates as if they come from a stream of updates through WebSocket. This SubscriptionClient struct will also manage reconnections to WebSockets (with automatic resubscriptions) and all the low-level stuff, providing an easy-to-use interface and leaving the upper-level code with a nice interface that is hard to misuse. When ActiveSubscription is dropped, it will automatically unsubscribe.

Notes to the reviewers

Suggested CHANGELOG Updates

Added Websocket subscription to the Wallet.

CHANGED

ADDED

REMOVED

FIXED

Checklist

I followed the code style guidelines
I ran just final-check before committing

### Description The last bit missing to wrap up #442 The main goal is to add a subscription to CDK Mint updates into the wallet. This feature will be particularly useful for improving the code whenever loops hit the mint server to check status changes. The goal is to add an easy-to-use interface that will hide the fact that we're connecting to WebSocket and subscribing to events. This will also hide the fact that the CDK-mint server may not support WebSocket updates. To be fully backward compatible, the HttpClientMethods traits have a new method, `subscribe,` which will return an object that implements `ActiveSubscription.` In the primary implementation, there is a `SubscriptionClient` that will attempt to connect through WebSocket and will fall to the HTTP-status pull and sleep approach (the current approach), but upper stream code will receive updates as if they come from a stream of updates through WebSocket. This `SubscriptionClient` struct will also manage reconnections to WebSockets (with automatic resubscriptions) and all the low-level stuff, providing an easy-to-use interface and leaving the upper-level code with a nice interface that is hard to misuse. When `ActiveSubscription` is dropped, it will automatically unsubscribe. ----- ### Notes to the reviewers  ----- ### Suggested [CHANGELOG](https://github.com/cashubtc/cdk/blob/main/CHANGELOG.md) Updates Added Websocket subscription to the Wallet. #### CHANGED #### ADDED #### REMOVED #### FIXED ---- ### Checklist * [x] I followed the [code style guidelines](https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md) * [x] I ran `just final-check` before committing

thesimplekid (Migrated from github.com) requested changes

2024-12-01 11:34:16 +00:00

thesimplekid (Migrated from github.com) left a comment

Great work just a few comments.

crates/cdk/src/wallet/subscription/mod.rs

					
				@ -0,0 +1,322 @@

				//! Client for subscriptions

If we call this on a mint that does not include NUT17 in the info response which is possible because it is an optional nut is_ws_support is set to true when we need to assume false. This leads to an infinite loop of the wallet trying to connect to the ws endpoint that does not exist on the mint.

This happens since we have the deafult of NUT17 ` Supported settings to support all the endpoints.
github.com/cashubtc/cdk@7d15587e3f/crates/cdk/src/nuts/nut17/mod.rs (L38-L65)

I think we should change the default here to not support any endpoints and then for out mint we won't use the Default but will explicitly set that we support them.

It may also be worth adding a time out to the ws connection attempt so the loop does not happen.

to reproduce this you can start a cdk-mintd from the v0.4.0 tag and then use this branch as the wallet with cdk-cli.

If we call this on a mint that does not include NUT17 in the info response which is possible because it is an optional nut `is_ws_support` is set to true when we need to assume false. This leads to an infinite loop of the wallet trying to connect to the ws endpoint that does not exist on the mint. This happens since we have the deafult of NUT17 ` Supported settings to support all the endpoints. https://github.com/cashubtc/cdk/blob/7d15587e3f3b5d7e992db4b12755caf289c2e31d/crates/cdk/src/nuts/nut17/mod.rs#L38-L65 I think we should change the default here to not support any endpoints and then for out mint we won't use the Default but will explicitly set that we support them. It may also be worth adding a time out to the ws connection attempt so the loop does not happen. to reproduce this you can start a cdk-mintd from the v0.4.0 tag and then use this branch as the wallet with cdk-cli.

Would it be better to put the mint specific code behind the mint feature instead of using unreachable?

This whole file in in a wallet mod so really there should be no mint relevant code here.

Would it be better to put the mint specific code behind the mint feature instead of using unreachable? This whole file in in a wallet mod so really there should be no mint relevant code here.

crates/cdk/src/wallet/subscription/ws.rs

					
				@ -0,0 +1,214 @@

				use std::collections::{HashMap, HashSet};

As mentioned above this creates an infinite loop if the wallet cannot connect to the ws endpoint, I think we should add a timeout.

NIT: Code style we don't import tracing and use the full path when we use it.

https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md#full-paths-for-logging

NIT: Code style we don't import tracing and use the full path when we use it. https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md#full-paths-for-logging

crodas (Migrated from github.com) reviewed

2024-12-01 14:44:16 +00:00

crates/cdk/src/wallet/subscription/ws.rs

					
				@ -0,0 +1,214 @@

				use std::collections::{HashMap, HashSet};

My idea is to try a few loops and then fall back to the HTTP worker otherwise. But it's better to understand the INFO response and attempt to connect to WebSocket instead of treating it as a boolean.

The HTTP subscription worker should be spawned if any subscription method is not supported over WebSocket or the WebSocket worker is exhausted of failures. The HTTP subscription method supports all methods, but it is less efficient.

My idea is to try a few loops and then fall back to the HTTP worker otherwise. But it's better to understand the INFO response and attempt to connect to WebSocket instead of treating it as a boolean. The HTTP subscription worker should be spawned if any subscription method is not supported over WebSocket or the WebSocket worker is exhausted of failures. The HTTP subscription method supports all methods, but it is less efficient.

thesimplekid (Migrated from github.com) reviewed

2024-12-02 17:10:04 +00:00

crates/cdk/src/wallet/subscription/ws.rs

					
				@ -0,0 +1,214 @@

				use std::collections::{HashMap, HashSet};

Yeah I think that makes sense. I noticed it because of the above noted issue with a mint that does not signal anything for nut17 and bc of the default we try anyway. In the case of a mint signaling explicitly that it does not support ws we should not try ws at all, if it does signal support we can try and then fall back on failure.

crodas (Migrated from github.com) reviewed

2024-12-04 22:26:09 +00:00

crates/cdk/src/wallet/subscription/mod.rs

					
				@ -0,0 +1,322 @@

				//! Client for subscriptions

If we call this on a mint that does not include NUT17 in the info response which is possible because it is an optional nut is_ws_support is set to true when we need to assume false. This leads to an infinite loop of the wallet trying to connect to the ws endpoint that does not exist on the mint.

If the response is not part of the info, it will be false (unwrap_or_default is set to false). Keep in mind that is_empty is negated. Anyway, I think we can safely assume it exists _now since the WebSocket worker will fall back to HTTP (after 246e583).

What do you think @thesimplekid?

> If we call this on a mint that does not include NUT17 in the info response which is possible because it is an optional nut is_ws_support is set to true when we need to assume false. This leads to an infinite loop of the wallet trying to connect to the ws endpoint that does not exist on the mint. If the response is not part of the info, it will be false (unwrap_or_default is set to false). Keep in mind that `is_empty` is negated. Anyway, I think we can safely assume it exists _now since the WebSocket worker will fall back to HTTP (after 246e583). What do you think @thesimplekid?

thesimplekid (Migrated from github.com) reviewed

2024-12-05 09:54:30 +00:00

crates/cdk/src/wallet/subscription/mod.rs

					
				@ -0,0 +1,322 @@

				//! Client for subscriptions

It will not be false if NUT17 is not included in the info response since we use the default here github.com/cashubtc/cdk@5ad4328a4e/crates/cdk/src/nuts/nut06.rs (L242-L245) and the default currently is that they are all supported github.com/cashubtc/cdk@34bd97b262/crates/cdk/src/nuts/nut17/mod.rs (L40-L87). This is why i suggest we change the default to not supported.

I think we should avoid the fallback in cases where we know it is not supported.

It will not be false if NUT17 is not included in the info response since we use the default here https://github.com/cashubtc/cdk/blob/5ad4328a4eda7ecd7ae869db0f98799f33ab2945/crates/cdk/src/nuts/nut06.rs#L242-L245 and the default currently is that they are all supported https://github.com/cashubtc/cdk/blob/34bd97b26263b322ecfef5c92dc5d7e84c2e087c/crates/cdk/src/nuts/nut17/mod.rs#L40-L87. This is why i suggest we change the default to not supported. I think we should avoid the fallback in cases where we know it is not supported.

thesimplekid (Migrated from github.com) reviewed

2024-12-05 09:57:22 +00:00

crates/cdk/src/wallet/subscription/ws.rs

					
				@ -0,0 +1,214 @@

				use std::collections::{HashMap, HashSet};

Do we want to log each error and what the error is or too much logging?

thesimplekid commented

2024-12-05 10:06:12 +00:00

(Migrated from github.com)

https://github.com/cashubtc/cdk/issues/493

- [x] https://github.com/cashubtc/cdk/issues/493

crodas commented

2024-12-05 20:28:44 +00:00

(Migrated from github.com)

I fixed #493 in 8b3ff74

crodas (Migrated from github.com) reviewed

2024-12-05 23:40:10 +00:00

crates/cdk/src/wallet/subscription/ws.rs

					
				@ -0,0 +1,214 @@

				use std::collections::{HashMap, HashSet};

Fixed in 392136e

crodas (Migrated from github.com) reviewed

2024-12-06 15:57:54 +00:00

crates/cdk/src/wallet/subscription/mod.rs

					
				@ -0,0 +1,322 @@

				//! Client for subscriptions

Fixed in 641c301

thesimplekid (Migrated from github.com) approved these changes

2024-12-08 16:52:59 +00:00

thesimplekid (Migrated from github.com) left a comment

ACK 2462168ec9

ACK 2462168ec99c77c61d15b8e5b63a63fd918d4473

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!473

				`@ -0,0 +1,214 @@`
				`use std::collections::{HashMap, HashSet};`

Rows
Columns