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

Websockets - filtering options #1907

Merged

Conversation

guilhermelawless
Copy link
Contributor

@guilhermelawless guilhermelawless commented Apr 14, 2019

This adds per-subscription filtering options via an extra field in a subscribe action:

{
  "action": "subscribe",
  "topic": "confirmation",
  "options": {
    "all_local_accounts": "true",
    "accounts": [ "xrb_123...", "..." ]
  }
}

Filtering happens after the message is built and is sent for broadcasting, with a bunch of O(1) checks for existance of accounts. We would have to change the dynamics of the websocket server quite a bit to handle it differently. Open to suggestions as usually or just pickup from this PR and improve!

Can add something to core_test if this seems ok.

Related: #1901

@cryptocode
Copy link
Contributor

#1906 is merged so this one will need a rebase @guilhermelawless

@cryptocode
Copy link
Contributor

cryptocode commented Apr 15, 2019

Could we get a test for confirmations being filtered / not filtered for both query options?

I'm adding the documentation tag so we remember to update https://github.com/nanocurrency/nano-node/wiki/WebSockets once merged.

@cryptocode cryptocode added the documentation This item indicates the need for or supplies updated or expanded documentation label Apr 15, 2019
@cryptocode cryptocode requested a review from clemahieu April 15, 2019 12:09
@guilhermelawless
Copy link
Contributor Author

Some tests were failing due to the destination being in block.destination and not block.link_as_account for some blocks.

Other tests were incomplete or some things untested due to not using a true async read on the socket client test.

Everything should be ok now.

@zhyatt zhyatt requested review from SergiySW and removed request for clemahieu April 17, 2019 14:02
…dcasted, but with any filtering options they're not supported (always filtered)
@cryptocode cryptocode merged commit 9a9fbca into nanocurrency:master Apr 18, 2019
@cryptocode
Copy link
Contributor

@guilhermelawless Thanks! The Websocket Wiki page needs an update, feel free to attach markup here.

@guilhermelawless guilhermelawless deleted the websockets/filtering-accounts branch April 18, 2019 11:34
@guilhermelawless
Copy link
Contributor Author

@cryptocode md is not supported so i zipped it, here you go!

WebSockets.zip

@cryptocode
Copy link
Contributor

Docs merged into the Websockets wiki page

@zhyatt zhyatt removed the documentation This item indicates the need for or supplies updated or expanded documentation label Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants