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

Some relayer improvments #2902

Merged
merged 6 commits into from
Apr 1, 2024
Merged

Conversation

svyatonik
Copy link
Contributor

I'll propagate changes to polkadot-sdk once paritytech/polkadot-sdk#3859 is merged.

This PR just adds some CLI args that we haven't supported so far. E.g. to connect to OnFinality Polkadot node, you need to use wss://polkadot.api.onfinality.io:443/public-ws. You had no chance to specify that path component before this PR. Now you can use either:

    --source-host polkadot.api.onfinality.io \
    --source-port 443 \
    --source-secure \
    --source-suffix "/public-ws" \

or

--source-uri wss://polkadot.api.onfinality.io:443/public-ws

We'll deprecate the former method later, when we'll start using *-uri in all our relayers.

Copy link
Collaborator

@serban300 serban300 left a comment

Choose a reason for hiding this comment

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

I'll propagate changes to polkadot-sdk once paritytech/polkadot-sdk#3859 is merged.

I can cherry-pick this commit into paritytech/polkadot-sdk#3859 after it's merged into parity bridges-common or I can redo paritytech/polkadot-sdk#3859 from scratch. I see that there are some more comments to address there anyway, and I'll probably fix at least part of them inside parity-bridges-common

/// Websocket server host name.
pub host: String,
/// Websocket server TCP port.
pub port: u16,
/// Websocket endpoint path at server.
pub path: Option<String>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we mark host, port, path and secure as deprecated in order to not forget about them ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO there's no much point in deprecating this in code - it'll only propagate to multiple #[allow(deprecated)] attributes across the code and end-users (those who run relays) won't see it anyway. I've just tried to find something deprecation-related in the clap/structopt to deprecate arguments instead, but there's nothing for that. And I don't want to remove arguments right now, because we have several relayers running && it'll take some time to propagate this change there. I could add a suffix to associated CLI arguments: "Deprecated: use uri instead" as a compromise. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, makes sense.

I could add a suffix to associated CLI arguments: "Deprecated: use uri instead" as a compromise. WDYT?

I don't know if it's necessary. We can leave it as it is. At most we can add a TODO comment, just to make sure we don't forget about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've filed #2909 to track it

@svyatonik svyatonik merged commit 34817d8 into polkadot-staging Apr 1, 2024
13 checks passed
@svyatonik svyatonik deleted the sv-some-relayer-improvements branch April 1, 2024 09:30
serban300 pushed a commit that referenced this pull request Apr 2, 2024
* added CLI arguments: full WS URI + separate for WS path URI component + additional log

* URI -> URL?

* added TODO

* fmt
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
* added CLI arguments: full WS URI + separate for WS path URI component + additional log

* URI -> URL?

* added TODO

* fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants