-
Notifications
You must be signed in to change notification settings - Fork 6
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
Creating new OrderbookYaml and DotrainYaml structs #1043
Conversation
chain_id: require_string( | ||
&value, | ||
Some("chain-id"), | ||
Some(format!("chain-id string missing in network: {key}")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think saying string here is a bit confusing, because it's also an error in the next step if it's a string (because it can't be parsed to a u64). from the user perspective if they put "one", that's not valid, but this error indicates it would be
let key = key.as_str().unwrap_or_default().to_string(); | ||
let network = Network { | ||
document: document.clone(), | ||
name: key.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i know it's not part of this PR, but i think the name field should be renamed to "key" as it's more generic and can be used across all the structs, whereas "name" collides with Token name for example
@@ -45,9 +47,81 @@ impl Network { | |||
} | |||
} | |||
|
|||
pub fn parse_networks_from_yaml( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we make this generic like parse_all_from_yaml
could it be trait?
.map(|(key, value)| { | ||
let key = key.as_str().unwrap_or_default().to_string(); | ||
let network = Network { | ||
document: document.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you clone the document it will still hold the reference back to the original right?
Motivation
See issue: #1035
Solution
OrderbookYaml
andDotrainYaml
Checks
By submitting this for review, I'm confirming I've done the following:
Missing things
DotrainYaml