search

interledger / interledger-rs

An easy-to-use, high-performance Interledger implementation written in Rust
http://interledger.rs
Other
201 stars 70 forks source link

Allow multple consumers per accountID in websocket payment subcription #657

Closed pradovic closed 4 years ago

pradovic commented 4 years ago

In the current code only the last consumer that subscribes to /accounts/:username/payments/incoming will get notifications. The quick fix is store a vec of UnboundedSender per accountID instead of a single one.

The goal was to support a several clients per accountID. If we expect much more clients per accountID in the future, more scalable solution could be considered.

aphelionz commented 4 years ago

Waiting 24 hours for @kincaidoneil @emschwartz et al

pradovic commented 4 years ago

Fix for the CircleCI problem: https://github.com/interledger-rs/interledger-rs/pull/658

aphelionz commented 4 years ago

@kincaidoneil @emschwartz @matdehaast My understanding is that this may fix #40 and #636, is that correct?

kincaidoneil commented 4 years ago

@aphelionz I don't think so. I believe there's a separate WS server for BTP (sending/receiving incoming ILP packets) vs this payment notification API.

Here's some of the relevant code that might need to be changed to support multiple BTP clients, per account, and/or fix other issues: https://github.com/interledger-rs/interledger-rs/blob/96a699f6d042608198ac476663634a1e02b9a515/crates/interledger-btp/src/server.rs#L33-L51 https://github.com/interledger-rs/interledger-rs/blob/96a699f6d042608198ac476663634a1e02b9a515/crates/interledger-btp/src/service.rs#L49

kincaidoneil commented 4 years ago

(Don't think that's necessary to merge this, just wanted to point it out in case you also wanted to support multiple BTP clients per account)

aphelionz commented 4 years ago

Agree, we can tackle that in another PR or potentially upstream in warp / tungstenite