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: catch dot port missing early #4082

Merged
merged 5 commits into from
Oct 12, 2023
Merged

Conversation

j4m1ef0rd
Copy link
Contributor

Pull Request

Closes: PRO-881

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

Added a check to the validate settings code for the Dot settings that checks that the port number exist for all endpoints.
This catches the error early and gives a nice error message.

Error: Error reading settings

Caused by:
    Polkadot node endpoints must include a port number: No port found in url: http://loc****

@j4m1ef0rd j4m1ef0rd added the CFE label Oct 9, 2023
@j4m1ef0rd j4m1ef0rd requested a review from kylezs October 9, 2023 06:17
@j4m1ef0rd j4m1ef0rd self-assigned this Oct 9, 2023
@linear
Copy link

linear bot commented Oct 9, 2023

PRO-881 Error should notify that only Polkadot requires a port to be specified

From one of the mainnet validators:

if I omit port in the setting:

[dot]
ws_node_endpoint = "wss://my-testnet:443"
http_node_endpoint = "https://my-testnet:443"

it gives error: Error: Networking or low-level protocol error: Invalid Url: Port number is missing in the URL

Because the other chains don't require the port, only Polkadot does, it is confusing.

@j4m1ef0rd
Copy link
Contributor Author

I'm getting some unexpected behaviour from the URL crate, it seems to follow this logic. If wss or https is used with a non default port, it will return None as the port number.
This means that the example endpoints we have used in many places are invalid:

  • wss://my_fake_polkadot_rpc:443/<secret_key>, should be port 8080.
  • https://my_fake_polkadot_rpc:443/<secret_key>, should be port 80.

Can you confirm that we will not support wss or https endpoints on port 443?

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #4082 (3d94e74) into main (73f5d59) will decrease coverage by 0%.
Report is 21 commits behind head on main.
The diff coverage is 98%.

@@          Coverage Diff           @@
##            main   #4082    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        376     377     +1     
  Lines      59703   60078   +375     
  Branches   59703   60078   +375     
======================================
+ Hits       42599   42804   +205     
- Misses     14961   15025    +64     
- Partials    2143    2249   +106     
Files Coverage Δ
engine/src/settings.rs 86% <98%> (+1%) ⬆️

... and 42 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@kylezs kylezs left a comment

Choose a reason for hiding this comment

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

For visibility:
This won't handle the case we're trying to handle - Jamie to look into it tomorrow :)

validate_port_exists(&endpoints.http_endpoint)
};
validate_dot_endpoints(&self.nodes.primary)
.map_err(|e| ConfigError::Message(format!("{CONTEXT_MESSAGE}: {e}")))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

this map error should be able to be pulled out too? - thus removing the need for the const too

Copy link
Contributor

Choose a reason for hiding this comment

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

let validate_dot_endpoints = |endpoints: &WsHttpEndpoints| -> Result<(), ConfigError> {
			validate_port_exists(&endpoints.ws_endpoint)
				.and_then(|_| validate_port_exists(&endpoints.http_endpoint))
				.map_err(|e| {
					ConfigError::Message(format!(
						"Polkadot node endpoints must include a port number: {e}"
					))
				})
		};

@j4m1ef0rd j4m1ef0rd merged commit 6436088 into main Oct 12, 2023
@j4m1ef0rd j4m1ef0rd deleted the feat/dot-port-setting-error branch October 12, 2023 02:46
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