Skip to content

feature: add support for unannounced channels #377

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
Closed
positiveblue wants to merge 7 commits into from
Closed

feature: add support for unannounced channels #377

positiveblue wants to merge 7 commits into from

Conversation

@positiveblue
Copy link
Contributor

@positiveblue positiveblue commented Jul 8, 2022 •
edited
Loading

Askers are able to decide (and price properly) in which market they want to provide liquidity:

  • No preference (default)
  • Only announced channels
  • Only unannounced channels

Bidders can define what kind of channels they are interested in: announced or unannounced (exclusive or)

Roasbeef
Roasbeef reviewed Jul 8, 2022
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Nice work!

Once we slot in the zero conf channels, the combo of: zero conf, this, and sidecar channels will allow from pretty much the best on boarding experience imo.

@dstadulis dstadulis changed the title feature: add support for matching with private channels feature: add support for matching with private/unannounced channels Jul 11, 2022
guggero
guggero reviewed Jul 15, 2022
View reviewed changes
Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Did a first pass. Main thing missing IMO is the channel acceptor that makes sure a new channel is actually opened with the correct flag.

@positiveblue positiveblue force-pushed the private_channels branch 2 times, most recently from dd9e72d to e9289d9 Compare August 1, 2022 05:34
@positiveblue positiveblue changed the title feature: add support for matching with private/unannounced channels feature: add support for unannounced channels Aug 1, 2022
@positiveblue positiveblue force-pushed the private_channels branch 4 times, most recently from da63630 to a1491c1 Compare August 2, 2022 19:26
positiveblue
positiveblue commented Aug 2, 2022
View reviewed changes
@positiveblue positiveblue force-pushed the private_channels branch 2 times, most recently from e835c7e to bd80579 Compare August 2, 2022 20:02
@positiveblue
Copy link
Contributor Author

positiveblue commented Aug 2, 2022

Ready for a second pass @Roasbeef @guggero Main changes:

  • Asker has a new field to specify which market s/he is interested selling the liquidity in
  • New channel acceptor to ensure that the new channel matches the announcement constraints.
  • Bump latest BatchVersion to signal that the client supports unannounced channels
  • MatchAnnouncementConstraints only takes into account if the orders are compatible or not, the server is responsible of filtering clients who do not support unannounced channels yet.

@Roasbeef
Copy link
Member

Roasbeef commented Aug 4, 2022

My suggestion would be to add a new enum on the ask order side that has these three values (with the default value of 0 being no_preference) in this PR, that would be covered by the same order version as the unannounced flag. That way all existing ask orders would become no_preference as soon as the maker updates their software (new batch version that signals that they now understand the unannounced flag and actually set it when opening the channel).

This approach SGTM!

Roasbeef
Roasbeef approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

The latest diff is looking really good, nice work!

I think we just need to cap this off with proper itests on the server side.

guggero
guggero reviewed Aug 8, 2022
View reviewed changes
Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very clean and straightforward PR, great work 🎉

Almost ready to merge, found just one small bug.

guggero
guggero reviewed Aug 8, 2022
View reviewed changes
@positiveblue positiveblue force-pushed the private_channels branch 2 times, most recently from 2fe32af to 04a743a Compare August 9, 2022 06:10
guggero
guggero approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice, LGTM 🎉

I think this now needs to be stacked on top of #375 because the server side PR for it was just merged. Otherwise you won't be able to pull in this version without getting errors.

@positiveblue positiveblue mentioned this pull request Aug 11, 2022
Merged
1 task
@lightninglabs-deploy
Copy link
Collaborator

lightninglabs-deploy commented Sep 20, 2022

@positiveblue, remember to re-request review from reviewers when ready

@positiveblue
rpc: add new fields for unannounced channels
a2e44af 
Bids can decide if they are interested in announced or unannounced
channels (exclusive or).

Asks can decide in what market they want to provide liquidity:
announced channels, unannounced channels or both.
@positiveblue
order: add channel announcement constraint fields
6af04f0 
Includes the definition of matchable orders based on the new constraint
fields.
guggero
guggero approved these changes Sep 23, 2022
View reviewed changes
@positiveblue
Copy link
Contributor Author

positiveblue commented Sep 23, 2022

closed in favor of #385

@guggero guggero deleted the private_channels branch September 23, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

+2 more reviewers

@dstadulis dstadulis dstadulis left review comments

@guggero guggero guggero approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@positiveblue @Roasbeef @lightninglabs-deploy @guggero @dstadulis