Skip to content

[anchors] lnrpc: add commitment_type to listchannels#4068

Merged
Roasbeef merged 1 commit into
lightningnetwork:masterfrom
halseth:anchors-rpc-committype
Mar 13, 2020
Merged

[anchors] lnrpc: add commitment_type to listchannels#4068
Roasbeef merged 1 commit into
lightningnetwork:masterfrom
halseth:anchors-rpc-committype

Conversation

@halseth
Copy link
Copy Markdown
Contributor

@halseth halseth commented Mar 11, 2020

Now that we have a third commitment type active (#3829), we deprecate the static_remote_key field on listchannels, and instead add another commitment_type.

@halseth halseth requested a review from joostjager March 11, 2020 08:14
@halseth halseth changed the title lnrpc: add commitment_type to listchannels [anchors] lnrpc: add commitment_type to listchannels Mar 11, 2020
@bhandras bhandras self-requested a review March 11, 2020 08:15
@halseth halseth force-pushed the anchors-rpc-committype branch from 5cfd2b6 to 08d668a Compare March 11, 2020 08:17
Comment thread lnrpc/rpc.proto Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Legacy as a type isn't very scalable. Static remote key will be legacy at some point too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OG? :P

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

GENESIS

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sticking with legacy 😛

Comment thread rpcserver.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is StaticRemoteKey also true for anchors?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't that confusing? We could also define the bool as indicating that particular format, not a format feature.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True, it could be either or. Not sure how much it matters, as we are deprecating it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed, it doesn't matter much. My preference would be to consider the boolean a channel type and not a channel feature, because it is simpler to explain. ListChannels only communicates channel types.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Went with your suggestion :)

Copy link
Copy Markdown
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

Comment thread lnrpc/rpc.proto Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: maybe (but i'm also a bit unsure) "does not change upon state changes"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sticking with this, as that was the original doc on static_remote_key

Comment thread lnrpc/rpc.proto Outdated
@Roasbeef Roasbeef added this to the 0.10.0 milestone Mar 11, 2020
@halseth halseth force-pushed the anchors-rpc-committype branch from 08d668a to 7d305fd Compare March 13, 2020 19:01
@Roasbeef Roasbeef merged commit 7f78054 into lightningnetwork:master Mar 13, 2020
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.

5 participants