Refactor channel state machine type hierarchy#2225
Conversation
| classOf[DATA_CLOSING], | ||
| classOf[DATA_WAIT_FOR_REMOTE_PUBLISH_FUTURE_COMMITMENT] | ||
| ), typeHintFieldName = "type") | ||
| val channelStates: CustomTypeHints = CustomTypeHints(Map( |
There was a problem hiding this comment.
I was forced to move to a CustomTypeHints because with ShortTypeHints we were now getting ChannelStateData$DATA_* strings which is really ugly...
t-bast
force-pushed
the
refactor-channel-data
branch
from
4cbc43f to
248ecc8
Compare
We slightly decouple the channel data from the channel state machine. This lets us have a 1:1 mapping between FSM states and FSM data. We also rename `HasCommitments` and some channel state commands. Fixes #1676
The transition handler for channel updates was really hard to read and broken after the changes from the previous commit. It was a good opportunity to clean it up and explicitly list all cases.
t-bast
force-pushed
the
refactor-channel-data
branch
from
248ecc8 to
e9773b6
Compare
| final case class DATA_WAIT_FOR_OPEN_CHANNEL(initFundee: INPUT_INIT_FUNDEE) extends ChannelData { | ||
| val channelId: ByteVector32 = initFundee.temporaryChannelId | ||
| /** Data associated to a [[ChannelState]], with a 1:1 mapping. */ | ||
| sealed trait ChannelStateData extends PossiblyHarmful { |
There was a problem hiding this comment.
I fail to see what is short-term and what is long-term in the design.
You are creating distinct types for ChannelStateData and ChannelData, but you are not taking advantage of that to store implementation-specific data (like the current connection).
You also had to introduce a WrappedChannelData to deal with the OFFLINE/SYNCING/... cases.
Why not make all ChannelStateData be WrappedChannelData (which was pretty much the previous design). What prompted you to change that?
The PossiblyHarmful is a nice touch.
There was a problem hiding this comment.
You're right, this is probably premature. I wanted to import changes from #2188, but they may be completely unnecessary now that we keep a single actor. I'll make some progress on dual funding and other refactoring first, and we'll see what is actually necessary.
|
|
||
| when(WAIT_FOR_INIT_INTERNAL)(handleExceptions { | ||
| case Event(initFunder@INPUT_INIT_FUNDER(temporaryChannelId, fundingSatoshis, pushMsat, initialFeeratePerKw, fundingTxFeeratePerKw, localParams, remote, remoteInit, channelFlags, channelConfig, channelType), Nothing) => | ||
| case Event(initFunder@INPUT_INIT_FUNDER(temporaryChannelId, fundingSatoshis, pushMsat, initialFeeratePerKw, fundingTxFeeratePerKw, localParams, remote, remoteInit, channelFlags, channelConfig, channelType), _) => |
There was a problem hiding this comment.
Maybe it shouldn't be done in this PR, but I believe this handler belongs to ChannelOpenSingleFunder.
2 participants
This PR imports parts of #2188, with a few differences in the type hierarchy. We refactor the channel data types to keep a 1:1 mapping between FSM states and FSM data, while keeping child traits for channel-related business logic.
This version contains less changes than what was previously proposed in #2188, however it touches a few dangerous parts of the code, reviewers beware!
Fixes #1676