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.