Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Ensure the listen addresses are consistent with the transport #6436

Merged
gavofyork merged 12 commits into from
Jun 23, 2020
Merged

Ensure the listen addresses are consistent with the transport #6436

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
09dab31
Initial commit
cecton Jun 19, 2020
8eba6b9
Ensure the listen addresses are consistent with the transport
cecton Jun 19, 2020
e29916f
Update client/network/src/error.rs
cecton Jun 19, 2020
bffba27
Update client/network/src/service.rs
cecton Jun 19, 2020
0e01458
Better implementation
cecton Jun 19, 2020
6721e00
Fix bad previous impl
cecton Jun 19, 2020
b856c82
add boot_nodes
cecton Jun 19, 2020
37068c7
reserved nodes
cecton Jun 19, 2020
486875f
test boot nodes
cecton Jun 19, 2020
e201b7a
reserved nodes tests
cecton Jun 19, 2020
75f40be
add public_addresses and make specific error type
cecton Jun 23, 2020
e97d85c
Update client/network/src/error.rs
cecton Jun 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update client/network/src/error.rs
  • Loading branch information
@cecton
cecton authored Jun 19, 2020
commit e29916fe3ddef51a28ae52ce4bf62c6ed88b0496
2 changes: 1 addition & 1 deletion client/network/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ pub enum Error {
},
/// Prometheus metrics error.
Prometheus(prometheus_endpoint::PrometheusError),
/// Invalid configuration
/// Invalid configuration.
Copy link
Contributor

@tomaka tomaka Jun 23, 2020

Choose a reason for hiding this comment

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

I know that this pattern of using Strings for error messages is common in Substrate, but to me it is a bad programming practice nonetheless.
Please change to an enum, or at least mention what the string is.

Suggested change
/// Invalid configuration.
/// Invalid configuration. Contains a human-readable error message.

Copy link
Contributor

@tomaka tomaka Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Also, Error::DuplicateBootnode is normally part of InvalidConfiguration as well.
I know I'm probably being annoying, but I'm trying to remove quick hacks from sc-network as much as possible.

Copy link
Contributor Author

@cecton cecton Jun 23, 2020

Choose a reason for hiding this comment

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

Not at all!! This is fine, I will improve it! Thanks for the feedback

Copy link
Contributor Author

@cecton cecton Jun 23, 2020

Choose a reason for hiding this comment

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

@tomaka voila. I made the error more "generic". It's a bit less descriptive but I think it's still good. I think it would have been weird to have 2 different errors to make the distinction in the human readable text.

InvalidConfiguration(String),
}

Expand Down