sim-lib: Get rid of nested json for node definition #152
Conversation
sr-gi
force-pushed
the
20231025_improve_json_parsing
branch
from
adf31b7 to
3938066
Compare
| @@ -25,10 +25,9 @@ mod random_activity; | |||
| mod serializers; | |||
|
|
|||
| #[derive(Serialize, Deserialize, Debug, Clone)] | |||
| #[serde(untagged)] | |||
There was a problem hiding this comment.
Oh yeah, there's a bunch of these you can use depending on what you're aiming for:
https://serde.rs/enum-representations.html
https://serde.rs/attr-flatten.html
sim-cli/src/main.rs
Outdated
| serde_json::from_str(&std::fs::read_to_string(cli.sim_file)?) | ||
| .map_err(|e| anyhow!("Cannot deserialize node connection data into LndConnection nor ClnConnection (line {}, col {})", e.line(), e.column()))?; |
There was a problem hiding this comment.
I just realised this may not be good, given this can error out both for node parsing and activity parsing
There was a problem hiding this comment.
Maybe just make the error more holistic?
Could not deserialize node connection (LND or CLN) or activity description: (line {}, col {}) in simulation file.
Most folks should be able to figure it out based on the line). Perhaps also include the error string if there is one? Since it's often hanging , or some other annoying json error.
There was a problem hiding this comment.
I like your version better.
Regarding including the original string, there's actually no meaningful info there, that's why I decided to go with a custom error.
Error: data did not match any variant of untagged enum NodeConnection at line 11 column 8
Not even the line and column are that useful, they mark the end of the section where the error is at, not even the real line :/
There was a problem hiding this comment.
Hm ok, let's just get as user friendly as we can and ship it.
|
Let's get this done with high priority if we're going to change it, told the warnet folks to wait on a stable config file before they proceed. |
On it |
sr-gi
force-pushed
the
20231025_improve_json_parsing
branch
from
3938066 to
76c8ced
Compare
README.md
Outdated
| ``` | ||
| { | ||
| "id": <node_id>, | ||
| "address": <ip_and_port_or_domain_and_port>, |
There was a problem hiding this comment.
Should we just move this line up?
Line 113 in 76c8ced
There was a problem hiding this comment.
Works for me! But I'd still hard code it in before to get as close as possible - https://<ip:port or domain:port>
There was a problem hiding this comment.
Ok, I'm going to open an issue so we don't require this, and we can do that in a low-prio followup
sim-cli/src/main.rs
Outdated
| serde_json::from_str(&std::fs::read_to_string(cli.sim_file)?) | ||
| .map_err(|e| anyhow!("Cannot deserialize node connection data into LndConnection nor ClnConnection (line {}, col {})", e.line(), e.column()))?; |
There was a problem hiding this comment.
Maybe just make the error more holistic?
Could not deserialize node connection (LND or CLN) or activity description: (line {}, col {}) in simulation file.
Most folks should be able to figure it out based on the line). Perhaps also include the error string if there is one? Since it's often hanging , or some other annoying json error.
I must have been way more sleep-deprived than I thought when originally designing this,
but we don't really need the nested json with node type tags to parse node information
(as long as the node info has, **at least** one different field name).
Mainly, this goes from:
```json
"nodes": [
{
"LND": {
"id": "...",
"address": "...",
"macaroon": "...",
"cert": "..."
}
},
{
"CLN": {
"id": "...",
"address": "...",
"ca_cert": "...",
"client_cert": "...",
"client_key": "..."
}
}
]
```
To:
```json
"nodes": [
{
"id": "...",
"address": "...",
"macaroon": "...",
"cert": "..."
},
{
"id": "...",
"address": "...",
"ca_cert": "...",
"client_cert": "...",
"client_key": "..."
}
]
```
sr-gi
force-pushed
the
20231025_improve_json_parsing
branch
from
76c8ced to
8ad307b
Compare
|
@carlaKC should be good now |
I must have been way more sleep-deprived than I thought when originally designing this, but we don't really need the nested json with node type tags to parse node information (as long as the node info has, at least one different field name).
Mainly, this goes from:
To: