Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ln-types crate? #44

Open
Kixunil opened this issue Dec 17, 2021 · 8 comments
Open

Use ln-types crate? #44

Kixunil opened this issue Dec 17, 2021 · 8 comments
Assignees
Labels
dependency Problem/bug caused by broken dependency refactoring Refactoring of the existing code
Milestone

Comments

@Kixunil
Copy link
Contributor

Kixunil commented Dec 17, 2021

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
Copy link
Contributor

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 dr-orlovsky added dependency Problem/bug caused by broken dependency refactoring Refactoring of the existing code labels Dec 21, 2021
@dr-orlovsky dr-orlovsky added this to To do in LNP implementation via automation Dec 21, 2021
@dr-orlovsky
Copy link
Contributor

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.

@dr-orlovsky dr-orlovsky added this to the v0.5.0 milestone Dec 21, 2021
@dr-orlovsky dr-orlovsky self-assigned this Dec 21, 2021
@Kixunil
Copy link
Contributor Author

Kixunil commented Dec 21, 2021

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
Copy link
Contributor

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

  • Overflow
  • Display
  • FromStr

@Kixunil
Copy link
Contributor Author

Kixunil commented Dec 21, 2021

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
Copy link
Contributor Author

Kixunil commented Dec 22, 2021

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

@dr-orlovsky
Copy link
Contributor

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

@Kixunil
Copy link
Contributor Author

Kixunil commented Jan 3, 2022

Done, looks like everything went well.

@dr-orlovsky dr-orlovsky removed this from To do in LNP implementation Mar 12, 2022
@dr-orlovsky dr-orlovsky modified the milestones: v0.5.0, v0.9.0 Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency Problem/bug caused by broken dependency refactoring Refactoring of the existing code
Projects
None yet
Development

No branches or pull requests

2 participants