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

feat: Onboarding through a hodl invoice #2560

Merged
merged 28 commits into from
May 27, 2024

Conversation

holzeis
Copy link
Contributor

@holzeis holzeis commented May 22, 2024

Introduces a lightning onboarding by leveraging an external lnd instance to manage a hodl invoice flow.

TODO:

  • Submit pre image when posting the order after the lightning funding event has been received.
  • Settle the invoice with the pre image received from the app.
  • Initiate a one sided channel opening with the app.
  • Add minimum trader reserve (e.g. 10k sats) to cover for price movements.
  • Add e2e test

In the next step we should look into the following issues.

Additionally we should look into adding an HTLC to our buffer transaction to make this swap trust less.

@holzeis holzeis requested a review from bonomat May 22, 2024 19:06
@bonomat
Copy link
Contributor

bonomat commented May 23, 2024

Simulator.Screen.Recording.-.iPhone.15.-.2024-05-23.at.15.14.45.mp4

🚀

@bonomat bonomat force-pushed the feat/setup-lnd-and-get-inbound-liquidity branch from d8e5b52 to 0bdeea6 Compare May 23, 2024 05:21
@bonomat bonomat marked this pull request as ready for review May 23, 2024 05:21
@bonomat bonomat requested review from bonomat and luckysori May 23, 2024 05:21
@holzeis holzeis self-assigned this May 23, 2024
@holzeis holzeis force-pushed the feat/setup-lnd-and-get-inbound-liquidity branch from 31b2351 to e113a8b Compare May 23, 2024 12:22
@holzeis
Copy link
Contributor Author

holzeis commented May 23, 2024

@bonomat I continued working on this and mainly tested the feature.

  • Added a way to cancel running watch tasks
  • Fixed the hash encoding (needs to be url safe as otherwise it could be interpreted as path)
  • Moved the pre-image argument also to the channel opening params (combined it with the flag)
  • Reworked the channel opening params addressing the todo to introduce an internal model. This model can now hold the external funding amount. Based on that amount I've added a check when opening the position to reject if the amount is not sufficient. I think this is crucial as everything else would be a huge vulnerability.

However, we need to introduce a minimum trader reserve for that as otherwise price movements could also result into a rejected order. (we need margin orders). I tried to add this to the ui, but failed, would be great if you could take over.

@holzeis
Copy link
Contributor Author

holzeis commented May 23, 2024

@luckysori probably best to review the changed files.

crates/lnd-bridge/examples/create_invoice_api.rs Outdated Show resolved Hide resolved
crates/xxi-node/src/commons/pre_image.rs Show resolved Hide resolved
use sha2::Sha256;

pub struct PreImage {
// TODO(bonomat): instead of implementing `PreImage` we should create our
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, only the hash needs to be url safe.

mobile/native/src/event/api.rs Outdated Show resolved Hide resolved
}

pub async fn get_hodl_invoice_from_coordinator(amount: Amount) -> Result<HodlInvoice> {
// TODO(bonomat): we might want to store this in the db so.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔I don't think it's necessary..

mobile/native/src/hodl_invoice.rs Outdated Show resolved Hide resolved
@holzeis holzeis force-pushed the feat/setup-lnd-and-get-inbound-liquidity branch 2 times, most recently from a026c96 to 81d6e17 Compare May 24, 2024 11:29
@holzeis holzeis force-pushed the feat/setup-lnd-and-get-inbound-liquidity branch 4 times, most recently from 67a2bb4 to 4824f28 Compare May 24, 2024 13:23
Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

I reviewed d81a770 and the logic seems sound.

If the external funding is insufficient, I think we should fail the whole thing earlier though. It seems to me like we have all the information available as soon as we get the match from the orderbook in process_new_market_order, so we don't have to wait until we call open_dlc_channel.

coordinator/src/trade/mod.rs Outdated Show resolved Hide resolved
This will allow us to generically wrap a api model into a signed value model which covers the signature creation and verification.
@holzeis holzeis force-pushed the feat/setup-lnd-and-get-inbound-liquidity branch from 4824f28 to 19a23f6 Compare May 24, 2024 18:22
@holzeis
Copy link
Contributor Author

holzeis commented May 24, 2024

I reviewed d81a770 and the logic seems sound.

If the external funding is insufficient, I think we should fail the whole thing earlier though. It seems to me like we have all the information available as soon as we get the match from the orderbook in process_new_market_order, so we don't have to wait until we call open_dlc_channel.

Thanks for you feedback, I've adapted the code, moved the check early and also now subtract the funding tx reserve and the channel fee reserve from the trader reserve.

I think this is now ready to go, but I will wait for your final review and approval.

@holzeis holzeis requested a review from luckysori May 24, 2024 18:25
Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

LGTM, I think this should work!

coordinator/src/cli.rs Outdated Show resolved Hide resolved
pub struct ChannelOpeningParams {
pub trader_reserve: Amount,
pub coordinator_reserve: Amount,
pub external_funding: Option<Amount>,
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 It might be simpler to just use Amount, becase a zero amount and a None are the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm.. I kind of like the option better because it is a bit more expressive than automatically implying that a zero amount is a OpenSingleFundedChannel

match params.external_funding {
    Some(external_funding) => {
        TradeAction::OpenSingleFundedChannel { external_funding }
    }
    None => TradeAction::OpenDlcChannel,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

But what does it mean to have a 0-valued external_funding then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably a bug. Any value below 250k is probably a bug.

coordinator/src/node/invoice.rs Outdated Show resolved Hide resolved
coordinator/src/orderbook/async_match.rs Outdated Show resolved Hide resolved
coordinator/src/routes.rs Outdated Show resolved Hide resolved
crates/xxi-node/src/commons/message.rs Outdated Show resolved Hide resolved
Comment on lines -70 to -72
EventType::PaymentClaimed,
EventType::PaymentSent,
EventType::PaymentFailed,
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

mod position;
mod unfunded_channel_opening_order;
Copy link
Contributor

Choose a reason for hiding this comment

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

🙃 Hmmm, we should align the terminology between coordinator and app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean? the coordinator doesn't know of an unfunded channel opening order.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think you are right. I was confused at first because this is implicitly about a single-funded channel, which is a different name.

Maybe it would be clearer with unfunded_wallet_channel_opening_order? The name would be annoyingly long though.

mobile/native/src/unfunded_channel_opening_order.rs Outdated Show resolved Hide resolved
mobile/native/src/watcher.rs Outdated Show resolved Hide resolved
Creates a hodl invoices, subscribes to updates to it and returns the invoice to the app.
bonomat and others added 22 commits May 27, 2024 09:48
Co-authored-by: Richard Holzeis <richard@holzeis.me>
Before the button was jumping depending on whether the slider or the button was shown.

I also changed the button text from "Confirm" to "Next" since there wouldn't be any semantical difference between funding with internal wallet or external.
The asterix was just not clear enough. I also did some minor styling improvements and removed the unused label argument from the fee expansion widget.
This will submit the order once a hodl invoice is paid.

In the next step we have to extend the post order api to revail the pre image so that the coordinator can claim the payment and open a single sided channel.
api.rs should remain as a thin proxy to the flutter frontend.
we need to store the pre image to be able to refer to it later again. Also, it will help us get a good overview of who asked for what.
if the trader receives the note that the ln invoice has been funded, he shares the preimage with him when creating a new order. Upon receiving said preimage, the coordinator checks if the preimage fits to a known hash in the db. Next: redeem payment before matching the order
this is needed because we send a url_safe base64 version over the via.
If we received the lightning payment from the user we open the channel to the user with only the coordinator's funds.

Co-authored-by: Richard Holzeis <richard@holzeis.me>
Otherwise, we might end up with "/" in the hash what will result in an endpoint not found error as the "/" is interpreted as path.
That should be enough to pay the lightning invoice and reduces the risk of price movements.
@holzeis holzeis force-pushed the feat/setup-lnd-and-get-inbound-liquidity branch from 19a23f6 to 06909be Compare May 27, 2024 08:14
@holzeis holzeis added this pull request to the merge queue May 27, 2024
Comment on lines -381 to +427
TraderRequiredLiquidity::None => (
margin_coordinator
+ collateral_reserve_coordinator.to_sat()
+ margin_trader
+ collateral_reserve_trader
// If the trader doesn't bring their own UTXOs, including the order matching fee
// is not strictly necessary, but it's simpler to do so.
+ order_matching_fee,
0,
dlc::FeeConfig::AllOffer,
),
TraderRequiredLiquidity::None => {
(
margin_coordinator
+ collateral_reserve_coordinator.to_sat()
+ margin_trader
+ collateral_reserve_trader
// If the trader doesn't bring their own UTXOs, including the order matching fee
// is not strictly necessary, but it's simpler to do so.
+ order_matching_fee,
0,
dlc::FeeConfig::AllOffer,
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🙃 I think nothing actually changed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jap only the brakets have been added 🙈

Comment on lines -238 to -240
let collateral_reserve_trader = params
.trader_reserve
.context("Missing trader collateral reserve")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's debatable, so none of this is a deal breaker at all.

What I dislike is leaking the knowledge of the external funding any further than necessary, and the fact that the trader_reserve field is ignored, when it could be used to carry this information from the outside.

And I think we could avoid code duplication by reusing a helper function.

Merged via the queue into main with commit a4f12f7 May 27, 2024
23 checks passed
@holzeis holzeis deleted the feat/setup-lnd-and-get-inbound-liquidity branch May 27, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants