rust-teos icon indicating copy to clipboard operation
rust-teos copied to clipboard

feat(lightning): Add Lightning as transport

Open mariocynicys opened this issue 1 year ago • 7 comments

This PR introduces bolt13 message structs and their lightning (de)serialization, to be used by the tower (for the lightning server) and client apps.

LDK's (de)serialization traits are used to make the message structs sendable over wire (Readable and Writeable).

A small customized macro-based serialization framework similar to LDK's was added to help organize the code and reduce its size.

Will push the lightning server to this PR later on.

Addresses #31

mariocynicys avatar Aug 02 '22 20:08 mariocynicys

Sorry I haven't taken the time to review this yet. Will do as soon as v0.1.2 gets released.

sr-gi avatar Sep 17 '22 10:09 sr-gi

@sr-gi

ser_macros.rs is based on lightning::util::ser_macros.rs but it significantly deviates from it. Is there any good reasoning behind it? Has lightning::util::ser_macros.rs been updated since you wrote this?

I mainly wanted impl_writeable_msg! to be exposed (and of course any macros called inside of it) because it handles all the TLV stuff, but that wasn't what LDK team wanted. They wanted to expose more of the utility macros instead (e.g. encode_tlv_stream!) which then the users could use to craft their things.

Not a big deal too. Exposing the utility macros would do fine. But after going on with BOLT13, some TLV datatypes there weren't covered by LDK's macro system (like vectors and strings) as they don't use it internally nor that it is used in other BOLTs, which led me to make some tweaks to my copy till it deviated that much.

You can also find other parts trimmed out (like check_tlv_order!, check_missing_tlv!, and default macro arm option), this is mainly because these parts weren't used in BOLT13 (and I think not even in other BOLTs, I think they mainly use it for internal messaging stuff).

As a general question, I saw you had some discussion regarding some of the serialization functionality being exposed in lightning. Is that still on track? What parts of this would be exported and how would that affect this part of the codebase?

Exposing them wouldn't help much now, because LDK's macros don't support strings and vector in TLVs, which we would like to have being supported.

mariocynicys avatar Sep 23 '22 17:09 mariocynicys

@sr-gi

ser_macros.rs is based on lightning::util::ser_macros.rs but it significantly deviates from it. Is there any good reasoning behind it? Has lightning::util::ser_macros.rs been updated since you wrote this?

I mainly wanted impl_writeable_msg! to be exposed (and of course any macros called inside of it) because it handles all the TLV stuff, but that wasn't what LDK team wanted. They wanted to expose more of the utility macros instead (e.g. encode_tlv_stream!) which then the users could use to craft their things.

Not a big deal too. Exposing the utility macros would do fine. But after going on with BOLT13, some TLV datatypes there weren't covered by LDK's macro system (like vectors and strings) as they don't use it internally nor that it is used in other BOLTs, which led me to make some tweaks to my copy till it deviated that much.

You can also find other parts trimmed out (like check_tlv_order!, check_missing_tlv!, and default macro arm option), this is mainly because these parts weren't used in BOLT13 (and I think not even in other BOLTs, I think they mainly use it for internal messaging stuff).

As a general question, I saw you had some discussion regarding some of the serialization functionality being exposed in lightning. Is that still on track? What parts of this would be exported and how would that affect this part of the codebase?

Exposing them wouldn't help much now, because LDK's macros don't support strings and vector in TLVs, which we would like to have being supported.

Right. I'd like to do one last pass on this before accepting it.

sr-gi avatar Sep 24 '22 14:09 sr-gi

Ok, I think this looks good. Not super used to reviewing macros but if something is not correct we will notice it when using the macros to implement the LN API.

LGTM modulo making the pending changes.

sr-gi avatar Oct 05 '22 09:10 sr-gi

Not super used to reviewing macros but if something is not correct we will notice it when using the macros to implement the LN API.

Yeah macros are tough but so cool and powerful. It's a pain that it ends up that you need to test macros with other macros :laughing:.

LGTM modulo making the pending changes.

Will patch make them soon :+1:.

mariocynicys avatar Oct 05 '22 10:10 mariocynicys