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

Display more fee information when opening a DLC channel #2062

Merged
merged 7 commits into from
Mar 1, 2024

Conversation

luckysori
Copy link
Contributor

@luckysori luckysori commented Feb 19, 2024

Fixes #2042.

channel-fee-info.mp4

The video does not correspond to the latest version, but it's close enough.


See get10101/rust-dlc#5 for the rust-dlc side of things.

// spots.
let epsilon = *fund_output_amount as i64 - expected_fund_output_amount.to_sat() as i64;

assert!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this test really panic? If it panics, why do we need an assert, or is the assert panicking?

@@ -307,7 +307,6 @@ async fn funding_transaction_pays_expected_fees() {

#[tokio::test(flavor = "multi_thread")]
#[ignore]
#[should_panic]
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, got it, you made the test fail first, then fixed it, very nice :)

pub fn estimated_fee_reserve() -> Result<Amount> {
let node = state::get_node();

// TODO: Here we assume that the coordinator will use the same confirmation target AND that
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs to be a TODO, it can just be a note given this function is just estimating.

// this is not 100% correct as fees are not exactly divided by 2. The fee a
// user has to pay depends on his final address.
reserved_fee_sats: details.fee.map(|fee| fee / 2),
// this is not 100% correct as fees are not exactly divided by 2. The share
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just fix this in rust-dlc and say the fee is always 50:50

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 don't think it should be fixed there, because it wouldn't make sense to me to charge the user more just because we as the coordinator bring 7 inputs or something.

I think it would be nicer to record all the fee contributions in a channel metadata table. I think this is something we should be able to achieve after the new model is introduced.

justfile Outdated
@@ -289,10 +289,19 @@ coordinator args="":

cargo run --bin coordinator -- {{args}}

run_maker_args := if os() == "linux" {
Copy link
Contributor

@bonomat bonomat Feb 21, 2024

Choose a reason for hiding this comment

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

Nice, alternatively you could use

docker run -d --name maker ghcr.io/get10101/aristides/aristides:main regtest --orderbook http://172.17.0.1:8000

It seems that on Linux 172.17.0.1 is always localhost

That being said, can you update the CI file to use this api as well?

docker run --pull always -d --name maker ghcr.io/get10101/aristides/aristides:main regtest --orderbook http://172.17.0.1:8000

Copy link
Contributor

@bonomat bonomat left a comment

Choose a reason for hiding this comment

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

Looks good.

@luckysori
Copy link
Contributor Author

I will keep this open until #2094 merges.

@luckysori luckysori force-pushed the feat/show-channel-tx-fees branch 2 times, most recently from 94c3783 to e89d7b7 Compare March 1, 2024 06:11
@luckysori
Copy link
Contributor Author

Now looking into those annoying UI test failures: https://github.com/get10101/10101/actions/runs/8107349092/job/22158820647?pr=2062#step:10:180.

Soon `rust-dlc` will enforce including at least one change output per
party in the funding transaction, so that either party can fee-bump
it.

The original `can_open_channel_with_min_inputs` test actually forced
`rust-dlc` to generate a change-output-less funding transaction. This
is valid for now, but it doesn't help to build on assumptions
that are going to change soon.

The new test simply checks that the funding transaction pays fees at the
expected fee rate. If we can trust `rust-dlc` to respect the fee rate
we give it, we can confidently display this information to a user who
is about to open a channel, so that they know the transaction fee cost
of their action.
The funding tx fee is just an estimate and it is usually slightly
overestimated.

We've had to further increase the size of the bottom sheets to fit all
the information on screen.
On the confirmation bottom sheet, we tell the user that some coins
will be locked up in the DLC channel. This message seems to imply that
these coins will still be owned by the user.

The margin and the collateral reserve make sense, but any fee paid or
reserved should probably be excluded. We could summarise this
information elswhere on the screen, but I don't think it is necessary
as all the information is already present separately.
The value that we were displaying corresponded to half of the funding
transaction fee.

The fee reserve cannot be derived just by looking at the funding
transaction, as it is included in the funding transaction shared
output. Eventually, we should display the fee reserve in the
corresponding wallet history entry (and perhaps in a channel details
section too).
@@ -48,7 +50,7 @@ channelConfiguration({
},
child: SingleChildScrollView(
child: SizedBox(
height: 450,
height: 500,
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've had to bump this number even further because otherwise the unit tests fail 🙈

Copy link
Contributor

@holzeis holzeis Mar 1, 2024

Choose a reason for hiding this comment

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

Noo 🫣 I worked so long to get the tests pass with the correct size 😅

@luckysori luckysori added this pull request to the merge queue Mar 1, 2024
Merged via the queue into main with commit e48d93e Mar 1, 2024
20 checks passed
@luckysori luckysori deleted the feat/show-channel-tx-fees branch March 1, 2024 13:25
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.

Include transaction fee info when opening a DLC channel
3 participants