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

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

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

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

Show resolved Hide resolved nano/node/websocket.hpp Outdated
Show resolved Hide resolved nano/node/websocket.cpp Outdated
Show resolved Hide resolved nano/node/websocket.cpp Outdated

@cryptocode cryptocode added this to the V19.0 milestone Apr 15, 2019

@cryptocode cryptocode added this to CP3 (2019-04-10) in V19 Apr 15, 2019

@cryptocode

This comment has been minimized.

Copy link
Collaborator

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 requested a review from clemahieu Apr 15, 2019

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

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 Apr 17, 2019

guilhermelawless added some commits Apr 17, 2019

Show resolved Hide resolved nano/node/websocket.cpp Outdated
Show resolved Hide resolved nano/node/websocket.cpp Outdated
Show resolved Hide resolved nano/node/websocket.cpp Outdated
Add tests for legacy blocks - without filtering options they are broa…
…dcasted, but with any filtering options they're not supported (always filtered)

@cryptocode cryptocode merged commit 9a9fbca into nanocurrency:master Apr 18, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

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

@guilhermelawless guilhermelawless deleted the guilhermelawless:websockets/filtering-accounts branch Apr 18, 2019

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

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

WebSockets.zip

@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

Docs merged into the Websockets wiki page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.