LNP-WG / lnp-node

Lightning network protocol daemon (suitable for generalized Lightning Network)
MIT License
145 stars 40 forks source link

Use `ln-types` crate? #44

Open Kixunil opened 2 years ago

Kixunil commented 2 years ago

You may be interested in my ln-types crate which provides multiple primitive types that can be shared between LN applications/libraries.

I could move it to one of the orgs (rust-bitcoin seems more suitable, IDK what the requirements would be there), if people are interested.

Not sure how relevant it is, see also issue at rust-lightning where the decision was to postpone it until there's more interest.

dr-orlovsky commented 2 years ago

Oh, where have you been a year ago when I had to do LNP Core lib (https://github.com/LNP-BP/lnp-core) and inet2_addr with all of those primitives...

... but from what I see not your types are pretty much different from mines and still many places. I will try to integrate/use them in inet2_addr.

As for moving the repo, I will be happy to have it here. In rust-bitcoin the requirement is MSRV 1.29 and having multiple contributors (one of the reasons I was not able tot bring https://github.com/LNP-BP/descriptor-wallet there, which also have multiple utility types on top of rust-bitcoin)

dr-orlovsky commented 2 years ago

Actually Amount is very needed and I was thinking of doing it myself. I will use it from ln-types right away and re-export in lnp-core as millistat amount.

BTW rust-bitcoin Amount is so terrible that I was thinking about doing Sats type in descriptor-wallet to use it here as well.

Kixunil commented 2 years ago

Yeah wanted to do this for a while but couldn't find the time. Happy to see that being later is still more helpful than not doing it.

In rust-bitcoin the requirement is MSRV 1.29

I mean the organization, not library. It doesn't even fit architecturally into the library. For instance rust-lightning library has higher MSRV and is in rust-bitcoin organization.

BTW rust-bitcoin Amount is so terrible

Which aspects do you find terrible? For me it's mostly overflow being unchecked and it not being used in TxOut (there's a PR to fix that). Other things seem OK to me.

dr-orlovsky commented 2 years ago

I mean the organization, not library.

I also meant organization :D It has the same name as the library - and same rules

For instance rust-lightning library has higher MSRV and is in rust-bitcoin organization.

It is not there anymore and was there due to historical reasons. I was told by org requirements for adding repos by A.Poelstra several months ago, so ...

Which aspects do you find terrible? F

Kixunil commented 2 years ago

Ah, now I see! Will look into migrating later, I'm too tired now.

I feel that displaying it directly with denomination is a good way to avoid footguns and my PR fixes the inability to be precise. Do you have other ideas what could be improved? Per Rust convention FromStr should be capable of parsing whatever came out of Display.

Kixunil commented 2 years ago

I tried to transfer ownership but got this error: "You don’t have the permission to create public repositories on LNP-BP"

dr-orlovsky commented 2 years ago

Sorry for taking so long: GiHub permission structure constantly change and hard to understand. I have allowed repository creation, so pls try now.

Kixunil commented 2 years ago

Done, looks like everything went well.