Send events when HTLCs settle on-chain by pm47 · Pull Request #884 · ACINQ/eclair

pm47 · 2019-03-06T16:43:35Z

This is a simpler alternative to #867, with the following limitations:

no OnChainRefundsDb and associated API calls
PaymentSettlingOnChain/PaymentLostOnChain events will be sent
exactly once per payment and have less information
we don't touch HtlcTimeoutTx
use json4s type hints instead of manual attributes to case classes

This is a simpler alternative to #867, with the following limitations: - no `OnChainRefundsDb` and associated API calls - `PaymentSettlingOnChain` event will be sent exactly once per payment and have less information - we don't touch `HtlcTimeoutTx` - use json4s type hints instead of manual attributes to case classes

# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala # eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentEvents.scala # eclair-core/src/test/scala/fr/acinq/eclair/api/JsonSerializersSpec.scala

araspitzu · 2019-03-28T15:10:50Z

TODO: update the doc

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

pm47

This LGTM to me but is missing a test on PaymentSettlingOnChain. Will take a stab at it

pm47 · 2019-03-28T16:25:30Z

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

-        case received: PaymentReceived => flowInput.offer(received.paymentHash.toString)
+        case message: PaymentFailed => flowInput.offer(Serialization.write(message)(formatsWithTypeHint))
+        case message: PaymentEvent => flowInput.offer(Serialization.write(message)(formatsWithTypeHint))
+        case other => logger.info(s"Unexpected ws message: $other")


Unhandled message should be handled differently

pm47 · 2019-03-29T15:29:30Z

eclair-core/src/main/scala/fr/acinq/eclair/api/JsonSerializers.scala


  implicit val shouldWritePretty: ShouldWritePretty = ShouldWritePretty.True

+  case class CustomTypeHints(custom: Map[Class[_], String]) extends TypeHints {


Can this be put in a separate file?

I think it belongs here logically, it's where we define our application-wide JsonSupport and all the custom serializers.

pm47 · 2019-03-29T15:29:38Z

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

+  // used to send typed messages over the websocket
+  val formatsWithTypeHint = formats.withTypeHintFieldName("type") +
+    CustomTypeHints(Map(
+      classOf[PaymentSent] -> "payment-sent",


pm47 · 2019-03-29T15:30:57Z

eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

      }
+      // for our outgoing payments, let's send events if we know that they will settle on chain
+      Closing
+        .onchainOutgoingHtlcs(d.commitments.localCommit, d.commitments.remoteCommit, d.commitments.remoteNextCommitInfo.left.toOption.map(_.nextRemoteCommit), tx)


@sstone can you double check this? this is quite sensitive

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

pm47 · 2019-03-29T15:34:01Z

eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentRequest.scala

        case 18 if prefix == "lnbc" => Base58Check.encode(Base58.Prefix.ScriptAddress, data)
-        case 17 if prefix == "lntb" => Base58Check.encode(Base58.Prefix.PubkeyAddressTestnet, data)
-        case 18 if prefix == "lntb" => Base58Check.encode(Base58.Prefix.ScriptAddressTestnet, data)
+        case 17 if prefix == "lntb" || prefix == "lnbcrt" => Base58Check.encode(Base58.Prefix.PubkeyAddressTestnet, data)


@sstone can you double check this?

It's correct because Base58 addresses on testnet and regtest have the same prefix, but not Bech32 addresses :(
But I think handling "lnbcrt" on a separate line looks a bit cleaner

…gy for websocket queue.

…d messages be handled by akka's default

Co-Authored-By: araspitzu <a.raspitzu@protonmail.com>

Co-Authored-By: pm47 <pm47@users.noreply.github.com>

sstone · 2019-03-29T17:21:38Z

eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentRequest.scala

        case 18 if prefix == "lnbc" => Base58Check.encode(Base58.Prefix.ScriptAddress, data)
-        case 17 if prefix == "lntb" => Base58Check.encode(Base58.Prefix.PubkeyAddressTestnet, data)
-        case 18 if prefix == "lntb" => Base58Check.encode(Base58.Prefix.ScriptAddressTestnet, data)
+        case 17 if prefix == "lntb" || prefix == "lnbcrt" => Base58Check.encode(Base58.Prefix.PubkeyAddressTestnet, data)


It's correct because Base58 addresses on testnet and regtest have the same prefix, but not Bech32 addresses :(
But I think handling "lnbcrt" on a separate line looks a bit cleaner

sstone · 2019-03-29T17:22:36Z

eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentRequest.scala

+        case 18 if prefix == "lntb" || prefix == "lnbcrt" => Base58Check.encode(Base58.Prefix.ScriptAddressTestnet, data)
        case version if prefix == "lnbc" => Bech32.encodeWitnessAddress("bc", version, data)
-        case version if prefix == "lntb" => Bech32.encodeWitnessAddress("tb", version, data)
+        case version if prefix == "lntb" || prefix == "lnbcrt" => Bech32.encodeWitnessAddress("tb", version, data)


Bech32 prefix should be "bcrt", not "tb"

Done in 5e476fd

…pi-pm

…ress

araspitzu · 2019-04-03T09:35:35Z

LGTM, i'll add the fallback address for /receive in #885

pm47 force-pushed the extended-api-pm branch from 663d111 to ea00496 Compare March 7, 2019 09:51

pm47 force-pushed the extended-api-pm branch from ea00496 to 85df143 Compare March 7, 2019 10:14

pm47 mentioned this pull request Mar 7, 2019

Add a proper payments database #885

Merged

araspitzu added 6 commits March 28, 2019 10:44

Merge branch 'master' into extended-api-pm

8ec8574

# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala # eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentEvents.scala # eclair-core/src/test/scala/fr/acinq/eclair/api/JsonSerializersSpec.scala

Finish merging master

c557f70

Merge branch 'master' into extended-api-pm

cd703f2

Reorganize specialized formats for the websocket

10bc4ab

Remove 'fallbackAddress' from /receive API

5b423b1

remote println from test,

c8d3887

araspitzu changed the title ~~[WIP] Send events when HTLCs settle on-chain~~ Send events when HTLCs settle on-chain Mar 28, 2019

Add test for websocket

9a7e215

pm47 commented Mar 28, 2019

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala Outdated Show resolved Hide resolved

Use nicer custom type hints when serializing to json (websocket)

52b17fe

pm47 commented Mar 29, 2019

View reviewed changes

araspitzu and others added 5 commits March 29, 2019 16:57

Log error with warn level in websocket actor, use backpressure strate…

09618a8

…gy for websocket queue.

re-introduce dropTail strategy for websocket buffer, let the unhandle…

008b2ca

…d messages be handled by akka's default

Update eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

14da6d3

Co-Authored-By: araspitzu <a.raspitzu@protonmail.com>

cleaner isSentByLocal

9624383

Co-Authored-By: pm47 <pm47@users.noreply.github.com>

added tests

dd52cdc

sstone requested changes Mar 29, 2019

View reviewed changes

araspitzu added 3 commits March 29, 2019 18:59

Fix bech32 prefix for regtest

5e476fd

Merge remote-tracking branch 'origin/extended-api-pm' into extended-a…

77eb1de

…pi-pm

Separate cases for bech32 testnet and regtest for BOLT11 fallback add…

1b91609

…ress

araspitzu requested a review from sstone April 2, 2019 12:11

Formatting, remove unused param in comments

e3f8d9b

sstone approved these changes Apr 3, 2019

View reviewed changes

araspitzu merged commit 3a56ad9 into master Apr 3, 2019

araspitzu deleted the extended-api-pm branch April 3, 2019 09:41

pm47 mentioned this pull request Apr 15, 2019

Extended API for fine-grained payment tracking #867

Closed


		implicit val shouldWritePretty: ShouldWritePretty = ShouldWritePretty.True

		case class CustomTypeHints(custom: Map[Class[_], String]) extends TypeHints {

Conversation

pm47 commented Mar 6, 2019

Uh oh!

araspitzu commented Mar 28, 2019

Uh oh!

Uh oh!

pm47 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

araspitzu commented Apr 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants