Add fee info to GetSentInfo by akumaigorodski · Pull Request #1070 · ACINQ/eclair

akumaigorodski · 2019-07-12T07:53:57Z

A feeMsat field has been added to OutgoingPayment, it is set to 0 for all states except SUCCEDDED.

codecov-io · 2019-07-12T08:15:26Z

Codecov Report

Merging #1070 into master will decrease coverage by 0.05%.
The diff coverage is 76.31%.

@@            Coverage Diff             @@
##           master    #1070      +/-   ##
==========================================
- Coverage    82.5%   82.45%   -0.06%     
==========================================
  Files          99       99              
  Lines        7587     7610      +23     
  Branches      298      288      -10     
==========================================
+ Hits         6260     6275      +15     
- Misses       1327     1335       +8

Impacted Files	Coverage Δ
...src/main/scala/fr/acinq/eclair/db/PaymentsDb.scala	`100% <ø> (ø)`	⬆️
...c/main/scala/fr/acinq/eclair/payment/Relayer.scala	`88.65% <0%> (ø)`	⬆️
...a/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala	`96.09% <84%> (-3.03%)`	⬇️
...ala/fr/acinq/eclair/payment/PaymentLifecycle.scala	`88.59% <88.88%> (+0.1%)`	⬆️
...lockchain/bitcoind/rpc/ExtendedBitcoinClient.scala	`84.61% <0%> (-7.7%)`	⬇️
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala	`92.3% <0%> (-3.85%)`	⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala	`74.92% <0%> (-0.31%)`	⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	`83.97% <0%> (-0.29%)`	⬇️
...main/scala/fr/acinq/eclair/wire/CommonCodecs.scala	`97.56% <0%> (-0.06%)`	⬇️
...ain/scala/fr/acinq/eclair/wire/ChannelCodecs.scala	`100% <0%> (ø)`	⬆️
... and 5 more

eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala

araspitzu · 2019-07-12T12:06:34Z

eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala

+    // 1 -> 2 was backwards compatible so this method is used to migrate both 1 and 2 to 3
    statement.executeUpdate("CREATE TABLE IF NOT EXISTS received_payments (payment_hash BLOB NOT NULL PRIMARY KEY, preimage BLOB NOT NULL, payment_request TEXT NOT NULL, received_msat INTEGER, created_at INTEGER NOT NULL, expire_at INTEGER, received_at INTEGER)")
    statement.executeUpdate("CREATE TABLE IF NOT EXISTS sent_payments (id TEXT NOT NULL PRIMARY KEY, payment_hash BLOB NOT NULL, preimage BLOB, amount_msat INTEGER NOT NULL, created_at INTEGER NOT NULL, completed_at INTEGER, status VARCHAR NOT NULL)")
+    statement.executeUpdate("ALTER TABLE sent_payments ADD COLUMN fee_msat INTEGER DEFAULT 0 NOT NULL")


Why not using a nullable field here?

And use feeMsat: Option[Long] instead of 0 by default in OutgoingPayment?

Right, having feeMsat: Option[Long] may be a stretch, the alternative would be to do the conversion in the db layer (keeping feeMsat: Long and nullable in the table) but that's even worse. I think we can keep it NOT NULL and use 0 as default value, after all if the payment wasn't successful we spent exactly 0 msat in fees.

So no changes here then?

Let's wait for more reviewers but for me is okay as it is, cc @sstone @pm47

I don't understand what made you change your mind @araspitzu, a nullable field seems to indeed be better here.

I don't have strong feelings about this but having it nullable forces the use of Option[Long] which is very correct but will lead to (IMHO) less elegant code where we need to check for the presence of the feeMsat on a finalized outgoing payment. On the other hand one could argue that using a default value of 0 is logically correct because a non finalized payment pays exactly 0 msat in fees.

FWIW on a client side I'm working on using 0 is bit more comfortable as I can just value + fee instead of value + feeOpt.getOrElse(0).

eclair-core/src/main/scala/fr/acinq/eclair/db/PaymentsDb.scala

pm47 · 2019-07-19T09:52:19Z

eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala

+    // 1 -> 2 was backwards compatible so this method is used to migrate both 1 and 2 to 3
    statement.executeUpdate("CREATE TABLE IF NOT EXISTS received_payments (payment_hash BLOB NOT NULL PRIMARY KEY, preimage BLOB NOT NULL, payment_request TEXT NOT NULL, received_msat INTEGER, created_at INTEGER NOT NULL, expire_at INTEGER, received_at INTEGER)")
    statement.executeUpdate("CREATE TABLE IF NOT EXISTS sent_payments (id TEXT NOT NULL PRIMARY KEY, payment_hash BLOB NOT NULL, preimage BLOB, amount_msat INTEGER NOT NULL, created_at INTEGER NOT NULL, completed_at INTEGER, status VARCHAR NOT NULL)")
+    statement.executeUpdate("ALTER TABLE sent_payments ADD COLUMN fee_msat INTEGER DEFAULT 0 NOT NULL")


I don't understand what made you change your mind @araspitzu, a nullable field seems to indeed be better here.

pm47 · 2019-07-19T10:02:08Z

eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala


-  using(sqlite.createStatement()) { statement =>
-    require(getVersion(statement, DB_NAME, CURRENT_VERSION) <= CURRENT_VERSION, s"incompatible version of $DB_NAME DB found") // version 2 is "backward compatible" in the sense that it uses separate tables from version 1. There is no migration though
+  def migration123(statement: Statement): Unit = {


migaration123 doesn't mean anything, I'd prefer taking inspiration from SqliteAuditDb (with migration12 and migration23, maybe one of them being noop).

pm47 · 2019-07-19T12:13:19Z

eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala

+  // 1 -> 2 was backwards compatible so this method is noop
+  def migration12(statement: Statement): Unit = ()
+
+  def migration23(statement: Statement): Unit = {


That doesn't seem right: if we are in version 2 then we already have those tables.

That should be:

def migration23(statement: Statement): Unit = { statement.executeUpdate("ALTER TABLE sent_payments ADD COLUMN fee_msat INTEGER DEFAULT 0 NOT NULL") }

Right?

Also the original comment for version 2 was much more informative

All CREATE TABLE/INDEX has IF NOT EXISTS which neutralizes them.

Sure, that's how the case CURRENT_VERSION => works (because starting from nothing or being up to date is the same from a migration point of view) but there is no need to use that in the 2->3 scenario.

araspitzu · 2019-08-08T08:09:35Z

Needs rebase on master and the new field feeMsat: Long should become fee: MilliSatoshi

# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/db/PaymentsDb.scala # eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala # eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentLifecycle.scala # eclair-core/src/test/scala/fr/acinq/eclair/db/SqlitePaymentsDbSpec.scala

# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentLifecycle.scala # eclair-core/src/test/scala/fr/acinq/eclair/db/SqlitePaymentsDbSpec.scala

t-bast · 2019-09-20T12:31:05Z

This change has been included in #1130 🎉

anton added 4 commits May 30, 2019 12:27

Correctly decode requests without multipliers

0292fb5

Merge remote-tracking branch 'upstream/master'

35a08bb

Merge remote-tracking branch 'upstream/master'

c2b6aa8

Add feeMsat field to OutgoingPayment

cf22c6e

araspitzu reviewed Jul 12, 2019

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqlitePaymentsDb.scala Outdated Show resolved Hide resolved

araspitzu reviewed Jul 12, 2019

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/db/PaymentsDb.scala Show resolved Hide resolved

Minor fixes

cbfc9a1

pm47 reviewed Jul 19, 2019

View reviewed changes

Fix migration methods naming

22cfea4

pm47 reviewed Jul 19, 2019

View reviewed changes

Split db updates across migration versions

c3273f2

anton added 3 commits August 9, 2019 15:23

Merge remote-tracking branch 'upstream/master' into getsentinfo-fees

38c3f51

# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentLifecycle.scala # eclair-core/src/test/scala/fr/acinq/eclair/db/SqlitePaymentsDbSpec.scala

Minor fix

33ac71e

t-bast mentioned this pull request Sep 17, 2019

Payment lifecycle refactoring #1130

Merged

t-bast closed this Sep 20, 2019

akumaigorodski deleted the getsentinfo-fees branch November 3, 2019 13:53

Conversation

akumaigorodski commented Jul 12, 2019

Uh oh!

codecov-io commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pm47 Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pm47 Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

araspitzu commented Aug 8, 2019

Uh oh!

t-bast commented Sep 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jul 12, 2019 •

edited

Loading

pm47 Jul 19, 2019 •

edited

Loading

pm47 Jul 19, 2019 •

edited

Loading