Fund channel flow #2544

bonomat · 2024-05-16T05:16:58Z

Ready for review :)

We know if finished syncing because `syncing` is `false` and the screen will finish reloading.

bonomat · 2024-05-20T06:25:46Z

Final version before review :)

Simulator.Screen.Recording.-.iPhone.15.Plus.-.2024-05-20.at.16.25.03.mp4

mobile/native/src/max_quantity.rs

mobile/native/src/api.rs

mobile/native/src/event/api.rs

holzeis · 2024-05-20T18:05:07Z

mobile/lib/features/trade/channel_creation_flow/channel_funding_screen.dart

+  Column buildInfoBox(FundingChannelTaskStatus? value, FundinType selectedBox) {
+    String transactionStatusText = "Waiting for payment...";
+    String transactionStatusInformationText =
+        "Please wait. If you leave now, your position won’t be opened when the funds arrive.";


🤔 if the user would want to switch to another app to send the onchain funds this message might be misleading.

Could be. Do you have a proposal to change this?

holzeis · 2024-05-20T18:08:10Z

❓ It's a bit weird to me that one jumps from the modal to a screen. What do you think about showing the new screens also as modals?

holzeis · 2024-05-20T18:08:40Z

❓ Did you test this with smaller devices?

holzeis · 2024-05-20T18:10:35Z

❓ Do we still need to show the QR code when the trade is executing? I think I would prefer if the user sees directly the position tab.

mobile/lib/features/trade/channel_creation_flow/channel_configuration_screen.dart

holzeis · 2024-05-20T18:19:01Z

mobile/lib/features/trade/channel_creation_flow/channel_configuration_screen.dart

+                          children: [
+                            TextSpan(
+                              text:
+                                  "🔔 This is your first trade and you do not have a channel yet. "


In the video this is very hard to read. I also think its too much text, maybe the title is enough?

Feel free to propose something else :)

maybe the title is enough?

😅

holzeis · 2024-05-20T18:21:34Z

mobile/lib/features/trade/channel_creation_flow/channel_configuration_screen.dart

+                                  value: ownTotalCollateral,
+                                  label: 'Channel size'),
+                              FeeExpansionTile(
+                                  label: "Fee*",


🔧 The asterix isn't pointing to anything.

Fair, I wanted to indicate that one can click on it.

mobile/native/src/unfunded_orders.rs

bonomat · 2024-05-21T01:20:26Z

❓ It's a bit weird to me that one jumps from the modal to a screen. What do you think about showing the new screens also as modals?

I agree, I switched to an extra screen because the modals were a pita to design.

bonomat · 2024-05-21T01:22:56Z

❓ Did you test this with smaller devices?

Yes, tested iPhone SE and looks ok.

bonomat · 2024-05-21T01:23:49Z

❓ Do we still need to show the QR code when the trade is executing? I think I would prefer if the user sees directly the position tab.

Probably not. I think there is lots we could improve. For example, I wouldn't show the popup dialog here at all but it's kinda impossible to control 😬

luckysori

LGTM! I did not delve into the Flutter code, but the screens look like a good starting point!

luckysori · 2024-05-22T03:38:19Z

crates/xxi-node/src/node/mod.rs

+                tracing::error!(
+                    txid = tx.txid.to_string(),
+                    address = address.to_string(),
+                    "Output not found. This should not happen, but if, it indicates something is wrong.");


Suggested change

"Output not found. This should not happen, but if, it indicates something is wrong.");

"Output not found. This should not happen, but if it does, it indicates something is wrong.");

luckysori · 2024-05-22T03:43:12Z

mobile/native/src/event/api.rs

@@ -125,6 +129,7 @@ impl Subscriber for FlutterSubscriber {
            EventType::ServiceHealthUpdate,
            EventType::ChannelStatusUpdate,
            EventType::BackgroundNotification,
+            EventType::FundingChannelNotification,


I only spent 1 hour yesterday debugging because I forgot to add my new EventType ❤️

Sorry to hear ... this literally happened to me as well :/

luckysori · 2024-05-22T03:43:56Z

mobile/native/src/lib.rs

@@ -36,3 +36,4 @@ pub use report_error::report_error_to_coordinator;
 )]
 mod bridge_generated;
 mod position;
+pub mod unfunded_orders;


🙃 Should go with the other pub mods in this file.

luckysori · 2024-05-22T03:45:03Z

mobile/native/src/max_quantity.rs

@@ -126,7 +139,7 @@ pub fn max_quantity(
    Ok(max_quantity)
 }

-/// Calculates the max quantity for the given input parameters. If an on-chai fee estimate is
+/// Calculates the max quantity. If an on-chai fee estimate is


Suggested change

/// Calculates the max quantity. If an on-chai fee estimate is

/// Calculates the max quantity. If an on-chain fee estimate is

luckysori · 2024-05-22T03:46:53Z

mobile/native/src/unfunded_orders.rs

+use std::time::Duration;
+use xxi_node::commons::ChannelOpeningParams;
+
+pub(crate) async fn submit_unfunded_channel_opening_order(


🙃 Every channel that is not yet open is unfunded. We could go for submit_unfunded_wallet_channel_opening_order or submit_channel_opening_order_without_utxos.

luckysori · 2024-05-22T03:47:44Z

mobile/native/src/unfunded_orders.rs

+        event::publish(&EventInternal::FundingChannelNotification(
+            FundingChannelTask::Pending,
+        ));


❓ Why do you need to send Pending so many times?

good question 🙈

luckysori · 2024-05-22T03:52:31Z

mobile/native/src/unfunded_orders.rs

+        if let Err(error) = bdk_node.sync_on_chain_wallet().await {
+            tracing::error!("Failed at syncing wallet {error:?}")
+        }
+
+        let balance = bdk_node.get_on_chain_balance();
+        tracing::debug!(balance = balance.to_string(), "Wallet synced");
+


🔧 The second log message can contradict the first one.

Should we just loop until the on-chain balance is enough? We confirmed that the UTXOs are there, but we can also confirm that the balance is up to date.

Alternatively, we could try to do it without the manual sync. It might not be necessary if we have broken out of the loop above.

I figured syncing the wallet is more expensive than looking up just a single address. But you are right, this would be an alternative.

Alternatively, we could try to do it without the manual sync. It might not be necessary if we have broken out of the loop above.

Unfortunately we need to sync the wallet once we found the address holds enough funds as otherwise we might fail later on because the wallet thinks it doesn't have enough funds.

I figured syncing the wallet is more expensive than looking up just a single address. But you are right, this would be an alternative.

Right. My first suggestion was to sync in a loop after the first loop has exited successfully. In theory, you only need to sync once, but there is an error path that we are currently just logging. I think if we fail to sync we want to keep trying until the UTXOs that we think should be there appear in the wallet.

Unfortunately we need to sync the wallet once we found the address holds enough funds as otherwise we might fail later on because the wallet thinks it doesn't have enough funds.

Okay, so one successful sync is necessary.

luckysori · 2024-05-22T03:56:08Z

mobile/native/src/unfunded_orders.rs

+    }
+    .remote_handle();
+
+    // We need to store the handler which will drop any old handler if present.


Suggested change

// We need to store the handler which will drop any old handler if present.

// We need to store the handle which will drop any old handle if present.

We introduce a new flow to fund the first position and with it the first channel. The idea is that the user does not have to have any on-chain funds before opening the first position. The flow is divided into two parts: 1) open a position using on-chain funds: if the user has already on-chain funds, then he can opt to use said funds and open the channel immediately 2) open a position using external funds: the user has the option to fund the wallet using an external wallet such as a lightning wallet, another on-chain wallet, or maybe even using a hardware wallet in the future. In this patch, we only introduce funding using an on-chain transaction. The app waits until it sees the funds incoming and then proceeds with the channel creation.

We can't navigate using the mocked router (I don't understand why). Which means, we can't navigate to the new channel configuration flow. Because of this, I've split up the tests into more fine granular tests. We test now each widget separately. This has the advantage that our tests are smaller and easier to understand. It seems like that if we want to test navigation, we should write integration tests instead.

bonomat force-pushed the feat/on-boarding-dialog branch 2 times, most recently from 92ccceb to 33202e7 Compare May 16, 2024 06:01

This comment was marked as outdated.

Sign in to view

bonomat added 3 commits May 20, 2024 11:43

chore: extract bitcoin balance into component

427c8e5

chore: remove unneeded log message.

219b85f

We know if finished syncing because `syncing` is `false` and the screen will finish reloading.

chore: extract qr code component for reusability

4d69f37

bonomat force-pushed the feat/on-boarding-dialog branch from c2c1b98 to 4868b9d Compare May 20, 2024 06:22

bonomat marked this pull request as ready for review May 20, 2024 06:25

bonomat requested review from holzeis and luckysori May 20, 2024 06:25