Skip to content

Conversation

@yaslama
Copy link
Member

@yaslama yaslama commented Mar 11, 2025

No description provided.

Copy link
Contributor

@danielgranhao danielgranhao left a comment

Choose a reason for hiding this comment

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

Looking good!

upstreamURL string
clients map[*websocket.Conn]map[string]bool // Tracks swap IDs for each client
subscribers map[string]map[*websocket.Conn]bool // Tracks clients for each swap ID
mu sync.Mutex
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't say we need to do it now, but we should keep in mind the possibility of changing to an RWMutex or even per-swap or per-conn locking in case we see contention issues.

Copy link
Contributor

@dangeross dangeross left a comment

Choose a reason for hiding this comment

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

LGTM. Agree with @danielgranhao the "ping" message is also needed

Copy link

@JssDWt JssDWt left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@danielgranhao danielgranhao left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Looks good! a few small comments.

p.mu.Unlock()

if len(swapIDs) > 0 {
resubscribeMsg := map[string]any{
Copy link
Member

Choose a reason for hiding this comment

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

I think just as a safety mechanism we should check with boltz if they have any limitation on the message size or the number of swaps can be sent in one message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Boltz devs confirmed that there is no limit on the number of swap ids

// Notify clients
for client, updates := range clientUpdates {
// Create a single notification message for the client
notification := map[string]any{
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider taking the message as is from the upstream and sent it to the subscribers in stead of creating the notification object?


// Optionally, send an unsubscribe message to the upstream server
if p.upstream != nil {
unsubscribeMsg := map[string]any{
Copy link
Member

Choose a reason for hiding this comment

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

unsubscribe supports multiple swaps like the code does in subscribe. It seems better to aggregate the swap ids and call once at the end of the loop to unsubscribe.

@yaslama yaslama merged commit c652392 into main Apr 3, 2025
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.

6 participants