Runtime network selection #1482
Conversation
…est into elmattic/network_selection
…est into elmattic/network_selection
* use NetworkVersion and implement de; scaffold ser * test for serde functionality * error on invalid network version; default to V1 * add missing test cases * remove redundant type conversion * fix bug in serialization; requested changes * add bad test case * use expect over unwrap
|
I can still sync to mainnet with this branch. Is it ready for review? |
Yes, please do. |
tests/conformance_tests/Cargo.toml
Outdated
| @@ -0,0 +1,87 @@ | |||
| [package] | |||
There was a problem hiding this comment.
This file should be deleted.
types/networks/src/lib.rs
Outdated
| } else { | ||
| mainnet::DRAND_SCHEDULE.iter() | ||
| }; | ||
| let mut points = BeaconSchedule(Vec::with_capacity(ds_iter.len())); |
There was a problem hiding this comment.
Would it make sense to make it a creation method for BeaconSchedule? Messing with its internals in another module is a bit shady.
types/networks/src/lib.rs
Outdated
| .iter() | ||
| .find(|info| height == info.height) | ||
| .map(|info| info.epoch) | ||
| .unwrap() |
There was a problem hiding this comment.
perhaps an expect with a nice description would be more gentle in a terrible case where it may fail (or perhaps it can't?)
There was a problem hiding this comment.
Done. It would be nice to prove that it cannot happen.
There was a problem hiding this comment.
Thanks @lemmih ! Hard to prove because it can depend of an input config file. But we could validate those calls upfront, ie at client init. This would avoid for unexpected panics later.
There was a problem hiding this comment.
@elmattic how about adding a shiny, low-priority issue with a nice description to have this validation so someone could take a look in a separate PR?
Ideally we would prove that it cannot happen.
vm/address/src/network.rs
Outdated
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| match s { | ||
| "mainnet" => Ok(Network::Mainnet), | ||
| _ => Ok(Network::Testnet), |
There was a problem hiding this comment.
Not sure about this, "cthulhu" shouldn't default to testnet - I'd expect an unrecognized network panic. Let's be pickier!
There was a problem hiding this comment.
I think it should, all networks that don't are mainnet are testnet.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #1402
Other information and links