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

Consider using an enum for channel phase in single PeerState::channel_by_id map #2422

Closed
dunxen opened this issue Jul 17, 2023 · 0 comments · Fixed by #2495
Closed

Consider using an enum for channel phase in single PeerState::channel_by_id map #2422

dunxen opened this issue Jul 17, 2023 · 0 comments · Fixed by #2495

Comments

@dunxen
Copy link
Contributor

dunxen commented Jul 17, 2023

In a very early iteration of #2077, an enum for different channel structs was considered for the channel_by_id map. However, that early iteration still dealt with the ChannelInterface trait which made it clunky. We should consider it again now that we have a simplified ChannelContext. This is completely internal to ChannelManager and so does not affect the public API.

We'll also propose calling this enum ChannelPhase to not overload the "kind", "type", or "state" nomenclature. Its variants will contain the new channel structs.

It would be worth considering an enum with a single map for a few good reasons:

  • Adding new ChannelPhase variants with new channel structs does not mean adding a new map to PeerState.
  • Channel counting logic would not need to be updated with each additional map
  • An enum can ensure that we handle the cases
  • Often reduces the number of map lookups

One con I can see:

  • Some handling of the enum might be a little clunky in some places, but this might mean those areas could do with some light refactoring.

This depends on the completion of #2382 which is required for the 116 release.

(h/t @jkczyz for bringing this up in review of #2382)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant