Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Adds validate_node_url() and refactors boot node check (#6907) #6970

Merged
merged 3 commits into from
Nov 10, 2017

Conversation

0x7CFE
Copy link
Contributor

@0x7CFE 0x7CFE commented Nov 2, 2017

This PR fixes #6907 by propagating error value instead of a bool flag.

IMO, full solution should return the actual reason of failure including underlying error message. However, there are some issues with it:

  • Impl Display for NetworkError should handle error details
  • Impl FromStr for Node and NodeEndpoint return NetworkError::AddressResolve even on incorrectly formatted addresses, like foo, enode://foo@bar and even on an empty string (Node::from_str() reports errors incorrectly #6972).

@5chdn 5chdn added this to the 1.9 milestone Nov 2, 2017
@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 2, 2017
@@ -368,6 +368,15 @@ pub fn is_valid_node_url(url: &str) -> bool {
Node::from_str(url).is_ok()
}

/// Same as `is_valid_node_url` but returns detailed `NetworkError`
pub fn validate_node_url(url: &str) -> Option<NetworkError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why introduce another method then?
Wouldn't it be better to just do is_valid_node_url().is_ok() in places that don't care about the error?

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 thought about it and it was my first intention, however, in that case, method name would conflict with it's semantics.

Also I'm a bit confused why we validate nodes by effectively doing all the work needed to construct Node just to throw it away and use the raw string value. Wouldn't it be better to return Reusult<Vec<Node>, NetworkError> from init_reserved_nodes? Anyway, I thought, that would cause substantial refactoring and is out of the scope.

For now I think it would be better to stick with the new function and refactor existing calls to is_valid_node_url.

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 3, 2017
@0x7CFE
Copy link
Contributor Author

0x7CFE commented Nov 6, 2017

Removed obsolete variant is_valid_node_url().

@tomusdrw, currently validate_node_url() returns Option<NetworkError>. Maybe it would be better to return Result<(), NetworkError>? That would allow check with ? and other chaining.

Actually, we may go even further, and return Result<Node, NetworkError> but that would require re-exporting of the Node struct. Don't sure, if it's viable in this case.

@0x7CFE 0x7CFE added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Nov 6, 2017
@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 10, 2017
@debris debris merged commit e13204c into master Nov 10, 2017
@debris debris deleted the validate_node_url branch November 10, 2017 14:39
@debris
Copy link
Collaborator

debris commented Nov 10, 2017

Impl Display for NetworkError should handle error details

this one is probably addressed by #6979

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid enode address format on failed name resolution
4 participants