-
Notifications
You must be signed in to change notification settings - Fork 267
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
Relay onion messages #2061
Relay onion messages #2061
Conversation
@t-bast Not ready yet but does the way I use actors look good? |
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.
@t-bast Not ready yet but does the way I use actors look good?
I think it's the right direction, yes!
I put a few comments to guide you to akka typed.
eclair-core/src/main/scala/fr/acinq/eclair/io/MessageRelay.scala
Outdated
Show resolved
Hide resolved
eclair-node/src/main/scala/fr/acinq/eclair/api/handlers/Message.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/io/MessageRelay.scala
Outdated
Show resolved
Hide resolved
c8a5960
to
92cf397
Compare
Codecov Report
@@ Coverage Diff @@
## master #2061 +/- ##
==========================================
+ Coverage 87.54% 87.60% +0.05%
==========================================
Files 165 166 +1
Lines 12688 12717 +29
Branches 536 545 +9
==========================================
+ Hits 11108 11141 +33
+ Misses 1580 1576 -4
|
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.
A couple of comments on the actor architecture itself.
I think that if you apply all these comments, it will both simplify the PR and make the system more testable and extensible in the future.
eclair-core/src/main/scala/fr/acinq/eclair/io/MessageRelay.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/io/MessageRelaySpec.scala
Outdated
Show resolved
Hide resolved
e1752a1
to
7399078
Compare
eclair-core/src/main/scala/fr/acinq/eclair/io/MessageRelay.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/remote/EclairInternalsSerializer.scala
Outdated
Show resolved
Hide resolved
4c8bd39
to
ac61c53
Compare
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.
Looking good, thanks for the changes!
A few minor comments and additional tests (in a separate commit in my branch) and we should be good to go.
eclair-core/src/test/scala/fr/acinq/eclair/io/MessageRelaySpec.scala
Outdated
Show resolved
Hide resolved
Co-authored-by: Pierre-Marie Padiou <pm47@users.noreply.github.com>
473ad4e
to
1942b4c
Compare
* Use typed actor framework for MessageRelaySpec * Fix port conflicts in integration tests * Use pipeTo instead of blocking for integration tests * Increase timeout for integration spec * Add tests for user content tlvs * Add tests for automatic reconnection
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.
Good job, LGTM 🚀
With this new API it should be easy to test onion messages against other implementations, have you been able to test with rusty's latest updates for c-lightning?
If we're compatible (again) it's an important milestone, it will be worth mentioning on the onion messages spec PR!
I've tested it ElementsProject/lightning#4921 can relay messages between eclair nodes. |
Actually relay onion messages and add API to send onion messages.
Follows #1957