Skip to content

rustfmt: Run on onion_message #3576

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

Merged
merged 10 commits into from
Feb 1, 2025

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Jan 30, 2025

We run the formatter on the onion_message sub-directory.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

LGTM, but it looks like this conflicts significantly with some of @shaavan's PRs. @jkczyz, shaavan, would it be fine to land or should we hold up?

Comment on lines +1199 to +1209
impl<
ES: Deref,
NS: Deref,
L: Deref,
NL: Deref,
MR: Deref,
OMH: Deref,
APH: Deref,
DRH: Deref,
CMH: Deref,
> OnionMessenger<ES, NS, L, NL, MR, OMH, APH, DRH, CMH>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge fan of this but not obvious to me how it can be helped either

@jkczyz
Copy link
Contributor

jkczyz commented Jan 30, 2025

LGTM, but it looks like this conflicts significantly with some of @shaavan's PRs. @jkczyz, shaavan, would it be fine to land or should we hold up?

The changes in #3412 don't look too large, but unfortunately #3246 was rebased on it and has more substantial changes. Probably better to wait.

Copy link
Member

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Thanks so much for looking out, @valentinewallace and @jkczyz! Really appreciate it! ✨

For the offers message flow refactor, the changes are definitely manageable on my end, so I can take care of any adjustments if this gets merged first.

As for No Path Offer, since it builds on flow, it’ll be a little while before it's ready to merge anyway.

So from my side, I’d say we’re good to go ahead with this one first, and I’ll handle the refactor afterward!

@tnull
Copy link
Contributor Author

tnull commented Feb 1, 2025

So from my side, I’d say we’re good to go ahead with this one first, and I’ll handle the refactor afterward!

Cool, thanks! Going ahead and landing this.

@tnull tnull merged commit c7c3973 into lightningdevkit:main Feb 1, 2025
24 of 25 checks passed
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.

4 participants