Add Serde implementations to some structs

[amackillop]

I'm working on this issue in ldk-node and I think it requires adding Serialization/Deserialization implementations to some types here first. I only added implementations to to the minimal amount of structs needed based on what is returned by the ldk-node api. I'm happy to add this to more structs while I'm here if that is desired.

I understand that there has been some disagreement about adding this support to the library. My use case here is that I want to be able to use some of the ldk-node structs (which depend on these ones) as JSON responses from a server built on top of it.

The current state of this draft is just the minimal set of implementations to fix type errors when adding the the same implementations in ldk-node.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 89.76%. Comparing base (669a459) to head (5b2698e).

Files	Patch %	Lines
lightning/src/ln/types.rs	0.00%	3 Missing ⚠️
lightning/src/events/mod.rs	0.00%	2 Missing ⚠️
lightning/src/util/config.rs	0.00%	2 Missing ⚠️
lightning/src/chain/mod.rs	0.00%	1 Missing ⚠️
lightning/src/ln/channelmanager.rs	0.00%	1 Missing ⚠️
lightning/src/ln/msgs.rs	0.00%	1 Missing ⚠️
lightning/src/offers/offer.rs	0.00%	1 Missing ⚠️
lightning/src/util/logger.rs	0.00%	1 Missing ⚠️
lightning/src/util/ser.rs	0.00%	1 Missing ⚠️
lightning/src/util/string.rs	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3157      +/-   ##
==========================================
- Coverage   89.83%   89.76%   -0.08%     
==========================================
  Files         121      121              
  Lines       99454    99468      +14     
  Branches    99454    99468      +14     
==========================================
- Hits        89345    89286      -59     
- Misses       7506     7560      +54     
- Partials     2603     2622      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

Cool, thanks! Not sure about the prior disagreement, but I for one think there is value to provide serde support for our public datatypes. Given that it's an optional feature, there shouldn't be a lot of drawbacks to adding this, IMO.

[tnull]

Could you set default-features = false to reduce the serde dependency to the minum required?

[TheBlueMatt]

I'm a bit confused by the set of things being serialized/deserialized here - what specifically are you trying to get serializable here?

[TheBlueMatt]

This seems like a very weird thing to serialize?

[tnull]

I suspect the motivation here is to be able to de/serialize LDK Node's Config object, which holds the used LogLevel.

[amackillop]

Correct. It's to avoid writing stuff like this

[TheBlueMatt]

Mmm, right. I guess it makes sense, though we should probably implement a serialization here that makes it readable, rather than just what I presume is writing an integer.

[amackillop]

For sure will do this basically

[TheBlueMatt]

IIUC just blindly implementing Deserialize here will allow for something that violates our conditions on the internal field's value.

[TheBlueMatt]

Also, if your goal is just to have a json version of some structs, maybe we should only implement Serialize, not De?

[tnull]

Also, if your goal is just to have a json version of some structs, maybe we should only implement Serialize, not De?

Mh, but if you're able to serialize something, say to a JSON-based config file, you'd like to be able to read it afterwards, no?

[amackillop]

I'm a bit confused by the set of things being serialized/deserialized here - what specifically are you trying to get serializable here?

This set of things is based on the structs being returned by the LDK Node API. It's the set of structs that are included in structs returned by those methods or accepted as arguments plus some structs included in configuration. I'm currently doing something similar to this but was exploring what the use of JSON instead of protocol buffers would look like. With Ser/De implementations, it makes it easier to use JSON.

That said, I'm still actively building the thing and some of the default implementations for these types don't actually work for what I'm doing. For example, the default implementation for Deserialize for SocketAddress doesn't work for something like 127.0.0.1:3000 (Error("unknown variant 127.0.0.1:3000, expected one of TcpIpV4, TcpIpV6, OnionV2, OnionV3, Hostname", line: 5, column: 41)) so it needs some tweaking to work how I want it but I'm not sure what is appropriate for the library. I've also discovered that the std::net::SocketAddr type behaves how I want out of the box I think due to the Display implementation and it's trivial to convert that to a SocketAddress later to configure the node. So I don't actually need that one. As I build more I'll have a better idea of what I actually need which is why I'm leaving this as a draft for now. Also the stuff that I don't need might still be required for the issue depending on what we want Ser/De implementations for in ldk-node.

[tnull]

This needs a rebase by now. @amackillop still interested in working on this?

[amackillop]

Yep I can get back to this sometime this week. This will be useful for the Prober project that I am also working on

[tnull]

Seems CI is failing unfortunately.

[amackillop]

I know why, serde_json as a test dependency causes ambiguity for some trait implementations

[TheBlueMatt]

Huh, I can't reproduce locally either. Can you rebase fix the non-spurious errors and then we'll look at it further?

eg

error[E0277]: the trait bound `Level: serde::ser::Serialize` is not satisfied
    --> lightning/src/util/logger.rs:400:42
     |
400  |         let serialized = serde_json::to_string(&level).unwrap();
     |                          --------------------- ^^^^^^ the trait `serde::ser::Serialize` is not implemented for `Level`
     |                          |
     |                          required by a bound introduced by this call
     |
     = note: for local types consider adding `#[derive(serde::Serialize)]` to your `Level` type
     = note: for types from other crates check whether the crate offers a `serde` feature flag
     = help: the following other types implement trait `serde::ser::Serialize`:
               &'a T
               &'a mut T
               ()
               (T,)
               (T0, T1)
               (T0, T1, T2)
               (T0, T1, T2, T3)
               (T0, T1, T2, T3, T4)
             and 131 others
note: required by a bound in `serde_json::to_string`
    --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde_json-1.0.128/src/ser.rs:2209:17
     |
2207 | pub fn to_string<T>(value: &T) -> Result<String>
     |        --------- required by a bound in this function
2208 | where
2209 |     T: ?Sized + Serialize,
     |                 ^^^^^^^^^ required by this bound in `to_string`

[TheBlueMatt]

Ah, nevermind, yea, these aren't spurious, you'll need to specify the types explicitly because serde adds another option for those calls.

[TheBlueMatt]

Closing as abandoned. If you want to pick this back up no problem, we can reopen.