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

feat(ws): add methods taking in a tungstenite config #2373

Merged
merged 5 commits into from
May 15, 2023

Conversation

fulminmaxi
Copy link
Contributor

@fulminmaxi fulminmaxi commented Apr 23, 2023

Motivation

Currently the websocket implementation does not allow to pass in a custom Tungestenite config. For example, its not possible to increase the default msg size over the Tungestenite default, which can lead to Message Too Long errors if fetching a lot of events with a lot of fields in them, one case where I've seen this failure in practice is when fetching around 8k events like this one:

    event TradeRequested(
        address indexed tokenIn,
        address tokenOut,
        uint256 amountIn,
        uint256 minAmountOut,
        address recipient,
        uint256 indexed tradeIndex,
        address indexed trader
    );

Solution

This PR adds 2 new methods to the WsClient and the WsManager:

  • connect_with_config which is the same as connect, but allows taking in a Tungstenite config object
  • connect_with_config_and_reconnects which is the same as connect_with_reconnects, but allows taking in a Tungstenite config object.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this mimics tungsenite's API and is an additional function so no breaking changes.

lgtm, pending nit

@@ -50,6 +50,34 @@ impl WsClient {
Ok(this)
}

/// Establishes a new websocket connection. This method allows specifying a custom websocket
/// configuration.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we link to tungsenite's connect functions here in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// custom websocket configuration.
pub async fn connect_with_reconnects_and_config(
conn: impl Into<ConnectionDetails>,
reconnects: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:
make last argument so arg 1+2 are identical to connect_with_config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, done in last commit, I've also changed the name so its connect_with_config_and_reconnects which will make it pop in IDE hints once you already decided you want to use a config

@fulminmaxi fulminmaxi requested a review from mattsse April 24, 2023 07:55
Copy link
Collaborator

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

The config is discarded, so will not apply to successive reconnects. Need to modify the struct to store the Option, and the need to modify the reconnect function to use it if available

Signed-off-by: Francesco <francesco@fulminlabs.org>
Signed-off-by: Francesco <francesco@fulminlabs.org>
Signed-off-by: Francesco <francesco@fulminlabs.org>
@prestwich
Copy link
Collaborator

wasm compilation is broken. I believe you'll need to add a few cfg blocks to the new reconnect logic and related imports

Signed-off-by: Francesco <francesco@fulminlabs.org>
@@ -234,6 +240,70 @@ impl RequestManager {
))
}

#[cfg(not(target_arch = "wasm32"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we DRY these? this and reconnect() are largely duplicated code. take the part that needs to be conditionally compiled and move it into a smaller function instead of duplicating the entire function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be better now

Signed-off-by: Francesco <francesco@fulminlabs.org>
@fulminmaxi fulminmaxi requested a review from prestwich May 12, 2023 11:56
@prestwich prestwich merged commit 3e7606d into gakonst:master May 15, 2023
11 of 15 checks passed
@fulminmaxi fulminmaxi deleted the add-ws-config branch May 15, 2023 15:17
@DaniPopes DaniPopes mentioned this pull request May 18, 2023
3 tasks
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.

None yet

3 participants