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.

Sign up for GitHub

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

Jump to bottom

fix: Wait for outbound capacity before paying jit channel open fee #1015

Merged
merged 1 commit into from
Jul 27, 2023
Merged

fix: Wait for outbound capacity before paying jit channel open fee #1015

merged 1 commit into from
Jul 27, 2023

Conversation

holzeis
Copy link
Contributor

@holzeis holzeis commented Jul 26, 2023 •
edited
Loading

fixes #883

@holzeis holzeis self-assigned this Jul 26, 2023
@holzeis holzeis force-pushed the fix/skipping-direct-payment-route branch 2 times, most recently from 9f22690 to 9418f0c Compare July 26, 2023 17:50
@holzeis holzeis changed the title fix: Add delay before paying jit channel open fees fix: Wait for channel to be usable before paying jit channel open fee Jul 26, 2023
@holzeis holzeis force-pushed the fix/skipping-direct-payment-route branch 3 times, most recently from 0bc00bc to b888ba0 Compare July 26, 2023 20:29
@holzeis holzeis changed the title fix: Wait for channel to be usable before paying jit channel open fee fix: Wait for outbound capacity before paying jit channel open fee Jul 27, 2023
@holzeis holzeis force-pushed the fix/skipping-direct-payment-route branch from 51cd76c to e916e13 Compare July 27, 2023 07:23
@holzeis holzeis requested a review from luckysori July 27, 2023 07:23
@holzeis holzeis marked this pull request as ready for review July 27, 2023 07:23
@holzeis holzeis requested a review from bonomat July 27, 2023 07:24
@holzeis holzeis force-pushed the fix/skipping-direct-payment-route branch from e916e13 to d37b56f Compare July 27, 2023 07:48
luckysori
luckysori approved these changes Jul 27, 2023
View reviewed changes
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!

Try to use tracing fields wherever you can though 😛

mobile/native/src/api.rs Show resolved Hide resolved
mobile/native/src/channel_fee.rs Outdated Show resolved Hide resolved
mobile/native/src/channel_fee.rs Outdated Show resolved Hide resolved
mobile/native/src/channel_fee.rs Outdated Show resolved Hide resolved
mobile/native/src/channel_fee.rs Outdated Show resolved Hide resolved
mobile/native/src/channel_fee.rs Outdated
})
.await
.map_err(|e| anyhow!("{e:#}"))
.context(format!(
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 Should use with_context instead.

Suggested change
.context(format!(
.with_context(|| format!(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well.. happy to change it, but do you think printing the text is such a huge performance cost, that we have to do it lazily? or do you have other reasons for suggesting 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'm sure it's negligible, but I think it's more correct to use with_context when you have to build the error so that it is evaluated lazily.

So obviously not a big deal at all, but I wouldn't know why not to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@holzeis just pointing out that I didn't mean to say to always use with_context. It depends on the... context 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

format! itself also incurs an allocation since it makes a String

mobile/native/src/channel_fee.rs Outdated Show resolved Hide resolved
@holzeis holzeis force-pushed the fix/skipping-direct-payment-route branch from d37b56f to 3c68e20 Compare July 27, 2023 08:15
@holzeis holzeis force-pushed the fix/skipping-direct-payment-route branch from 3c68e20 to 22932d2 Compare July 27, 2023 08:16
bonomat
bonomat approved these changes Jul 27, 2023
View reviewed changes
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.

If it works, I'd say let's get it in.

@holzeis
Copy link
Contributor Author

holzeis commented Jul 27, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 27, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 6e7d067 into main Jul 27, 2023
7 checks passed
@bors bors bot deleted the fix/skipping-direct-payment-route branch July 27, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Restioson Restioson Restioson left review comments

@luckysori luckysori luckysori approved these changes

@bonomat bonomat bonomat approved these changes

Assignees

@holzeis holzeis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Skipping direct route to payee when making payments from app to coordinator
4 participants
@holzeis @bonomat @Restioson @luckysori