Skip to content
This repository has been archived by the owner on Jul 3, 2024. It is now read-only.

Refactor project to the "state of the art" #4

Merged
merged 1 commit into from
Nov 13, 2021

Conversation

xn3cr0nx
Copy link
Contributor

@xn3cr0nx xn3cr0nx commented Nov 9, 2021

Preamble

Hello, I recently started looking into LNP-BP code, willing to start supporting the development. I found out this project is sort of outdated, including a modernize branch with the same goal of this PR, probably stopped due to time-consuming refactoring. I tried to take over, although I took some "design decisions" that should probably be discussed, due to changes in dependencies. I might have asked before, but I was primarily interested into start digging into LNP-BP's project, then if I wasted time with some changes, that's fine to me.

I'm a full-time Go dev, so I also take the opportunity to improve my Rust, I only have experience with in embedded systems.

Approach

I started looking into this repository cause I had issues compiling due to outdated dependencies, rust-lnpbp in particular. I entered the rabbit hole, and the result is the current PR that involves almost 40 files changes. I took lnp-node as ground base for some decisions and make the project "up to date".

Changes

First I investigated moved dependencies. I included bp-core in the dependencies, but on this standpoint, I had to fork this repo and use my branch in the current PR because short_id is private to the crate. If this is fine I can create a further PR in bp-core in order to make it public.

The second issue is related to former lnpbp::{TryService, Service}, now lying into microservices crate. This was not just a matter of changing the import, but since lnpbp refactoring, microservices crate does no longer support async based traits, and this involved multiple changes in the current PR. For this reason I removed the non-blocking/async IO and APIs from README file.

There are further changes related to different imports required in clap v3, bumped dependencies, msgbus that was half implemented here (no longer available in rust-lnpbp). I'm open to discussion to improve the PR, removing changes that were out of scope or not aligned to design goals.

Future

There are no tests in the project, then in order to verify the refactoring worked properly I manually tested the binaries. It's pretty clear the set of functionalities is pretty limited to date. I'm aware there are other projects/repositories that deserve more focus at the moment. Is it worth to invest time in this project or would you prefer to have support on other projects like Bitfrost?

…te of the art LNP/BP suite like internet2 microservices to modernize the project
@dr-orlovsky
Copy link
Member

Wow, huge work! We left BP Node for later refactoring once we will finish LNP Node and RGB, but now with your help we can move them in parallel!

I will be able to dig into this PR early next week. Meanwhile feel free to propose PRs to bp-core exposing types required here.

@dr-orlovsky dr-orlovsky self-assigned this Nov 13, 2021
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is huge, thank you very much! I did not found anything what I will do other way, so I am ready to merge this. The only question I have is why do we need to introduce src/msgbus/encode.rs mod? (I guess it is used somehow, but can't see how from the code)

For the future, feel free to do such large changes as a number of commits; for instance you can do rustfmt as a separate commit and other changes in other commits, such it will be easier to review business logic changes from pure formatting changes.

Thank you again for great work. Regarding BP Node vs Bifrost focus, answering in a next comment

@dr-orlovsky
Copy link
Member

So, we have the following roadmap:

  1. Complete LNP Core & LNP Node
    1.1. Have a full interoperability with modern ("legacy") LN
    1.2. Implement core of Bifrost + RGB-specific applications on top
    This will require completion of Taproot support in rust-bitcoin, which is my current WIP, and, potentially modifications to client-side-validation libraries introducing sign-to-contract commitments and deterministic bitcoin commitment refactoring (we will talk about the details on our next dev call on next Wednesday)

  2. Release RGB Core & RGB Node, well integrated with LNP Core

  3. Do a proper wallet on top of both (which also require work on private key handing / signature module external to LNP/RGB nodes) - we use MyCitadel for that

  4. Work on BP Node to fulfill two ambitions, in the following order:
    4.1. Get a more efficient (than Electrum server/Electrs) indexing of blockchain data, especially for Taproot/Miniscript/RGB/Bifrost-specific purposes
    4.2. Use libbitcoinconsensus to upgrade BP Node into a full bitcoin validating node, using not only bitcoin wire protocol, but also Bifrost for block propagation

You are welcome to join any part of this story - the one you like the most. It is not that pt 4 can't be done in parallel with 1, 2 and 3; the ordering is more about my personal priorities (defined by the ability to provide use for RGB as soon as possible) than of the implementation order.

@xn3cr0nx
Copy link
Contributor Author

xn3cr0nx commented Nov 13, 2021

This is huge, thank you very much! I did not found anything what I will do other way, so I am ready to merge this. The only question I have is why do we need to introduce src/msgbus/encode.rs mod? (I guess it is used somehow, but can't see how from the code)

For the future, feel free to do such large changes as a number of commits; for instance you can do rustfmt as a separate commit and other changes in other commits, such it will be easier to review business logic changes from pure formatting changes.

Thank you again for great work. Regarding BP Node vs Bifrost focus, answering in a next comment

Thanks! It definitely makes sense to split in smaller commits 😅 I didn't because I chased in order to remove all errors from rust-analyzer and ended up with so many files changed.

About src/msgbus/encode.rs, I had to re-introduce it due to VecEncoding from queryd

I didn't open bp-core PR yet cause I'm completing documentation for making short_id public and also implementing some unit_test in order to understand properly the module.

So, we have the following roadmap:

Complete LNP Core & LNP Node
1.1. Have a full interoperability with modern ("legacy") LN
1.2. Implement core of Bifrost + RGB-specific applications on top
This will require completion of Taproot support in rust-bitcoin, which is my current WIP, and, potentially modifications to client-side-validation libraries introducing sign-to-contract commitments and deterministic bitcoin commitment refactoring (we will talk about the details on our next dev call on next Wednesday)

Release RGB Core & RGB Node, well integrated with LNP Core

Do a proper wallet on top of both (which also require work on private key handing / signature module external to LNP/RGB nodes) - we use MyCitadel for that

Work on BP Node to fulfill two ambitions, in the following order:
4.1. Get a more efficient (than Electrum server/Electrs) indexing of blockchain data, especially for Taproot/Miniscript/RGB/Bifrost-specific purposes
4.2. Use libbitcoinconsensus to upgrade BP Node into a full bitcoin validating node, using not only bitcoin wire protocol, but also Bifrost for block propagation

You are welcome to join any part of this story - the one you like the most. It is not that pt 4 can't be done in parallel with 1, 2 and 3; the ordering is more about my personal priorities (defined by the ability to provide use for RGB as soon as possible) than of the implementation order.

Awesome, thanks for the overview! I'm going to dig into the code in order to understand where I can provide more support 👍

@dr-orlovsky dr-orlovsky merged commit 0d4089e into BP-WG:master Nov 13, 2021
@dr-orlovsky
Copy link
Member

Merged it and opened an issue for the follow-up work: #5

@xn3cr0nx xn3cr0nx deleted the feat/state-of-the-art branch November 13, 2021 15:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants