-
Notifications
You must be signed in to change notification settings - Fork 265
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
Migrate ZmqWatcher to akka-typed #1759
Conversation
Use akka typed for the ZmqWatcher actor. This is a naive port with minimal code changes. More changes will be added in the next commits to leverage akka typed more extensively.
Add missing test cases and enrich existing tests.
This ensures WatchEvents use the same concrete BitcoinEvent as their corresponding Watch.
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala
Outdated
Show resolved
Hide resolved
No functional change, just moving the code at the beginning of the file.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1759 +/- ##
==========================================
+ Coverage 89.17% 89.28% +0.10%
==========================================
Files 144 144
Lines 10822 10933 +111
Branches 494 455 -39
==========================================
+ Hits 9651 9762 +111
Misses 1171 1171
|
Instead of encapsulating a `BitcoinEvent`, flatten all watches types used. It provides better type safety and prevents potential incoherencies with the type of `BitcoinEvent` encapsulated.
We only keep "real" events that can be sent directly to watchers instead of being wrapped in watch events.
There was a race condition with block production, where we didn't properly wait for the right block to be confirmed before setting the watch.
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a pretty cool change!
Use akka-typed for the ZmqWatcher actor.
It is probably best reviewed commit by commit.
BitcoinEvent
(even though they're actually dropped at runtime because of type erasure, but since the compiler enforces it it's enough). Note that this introduces compiler warnings inZmqWatcher.scala
that I don't know how to get rid of.I tried to go even further and also constrain the
replyTo
for eachWatch
, something like:This would let us remove the
unsafeUpcast
inTxPublisher
, but I lost to the compiler on that one...Another reason I gave up this approach is that these type constraints aren't really enforced, they're basically hidden
.asInstanceOf[E]
that will only throw at runtime if we guessed wrong, but not at compile-time, so it may not be worth going in that direction...This is why in the last commits I instead flattened the hierarchy to get rid of generic types as much as possible, which provides much better type safety.
EDIT: these caveats have eventually all been resolved in the last commits. We're now in a good state where we make the most of strong typing without bumping into type erasure!