Store more payment info for sent payments#1048
Closed
Conversation
fbf4c86 to
198d307
Compare
Codecov Report
@@ Coverage Diff @@
## master #1048 +/- ##
==========================================
+ Coverage 81.93% 82.25% +0.31%
==========================================
Files 97 97
Lines 7480 7783 +303
Branches 298 313 +15
==========================================
+ Hits 6129 6402 +273
- Misses 1351 1381 +30
|
Use description from the invoice in paymentDB.sent_payments, use datatype BLOB to store descriptions
dpad85
reviewed
Jun 28, 2019
| * @param targetNodeId the recipient of this payment | ||
| */ | ||
| case class OutgoingPayment(id: UUID, paymentHash: ByteVector32, preimage:Option[ByteVector32], amountMsat: Long, createdAt: Long, completedAt: Option[Long], status: OutgoingPaymentStatus.Value) | ||
| case class OutgoingPayment( |
Member
There was a problem hiding this comment.
Indentation is a bit weird, it should be:
case class OutgoingPayment(id: UUID,
paymentHash: ByteVector32,
preimage: Option[ByteVector32],
amountMsat: Long,
createdAt: Long,
completedAt: Option[Long],
status: OutgoingPaymentStatus.Value,
paymentRequest_opt: Option[PaymentRequest] = None,
description_opt: Option[String] = None,
targetNodeId: PublicKey)|
|
||
| def getByteVector32Nullable(columnLabel: String): Option[ByteVector32] = getByteVectorNullable(columnLabel).map(ByteVector32(_)) | ||
|
|
||
| def getStringNullable(columnLabel: String): Option[String] = { |
Member
There was a problem hiding this comment.
Why is getStringNullable not alongside getNullableLong (see above)?
We should move getNullableLong into ExtendedResultSet for consistency.
| case Event(c: SendPayment, WaitingForRequest) => | ||
| router ! RouteRequest(nodeParams.nodeId, c.targetNodeId, c.amountMsat, c.assistedRoutes, routeParams = c.routeParams) | ||
| paymentsDb.addOutgoingPayment(OutgoingPayment(id, c.paymentHash, None, c.amountMsat, Platform.currentTime, None, OutgoingPaymentStatus.PENDING)) | ||
| // val desc: Option[String] = c.paymentRequest_opt.map(_.description.fold(_ => _, _.toHex)) |
| statement.executeUpdate("ALTER TABLE sent_payments ADD payment_request TEXT") | ||
| statement.executeUpdate("ALTER TABLE sent_payments ADD description BLOB") | ||
| statement.executeUpdate(s"ALTER TABLE sent_payments ADD target_node_id BLOB DEFAULT '03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f' NOT NULL") | ||
| } |
Member
There was a problem hiding this comment.
Having a default node id for the payment destination does not sound right. I think we should either make this field nullable, meaning that OutgoingPayment.targetNodeId would be optional, or have a "" or "unknown"default value for the target_node_id column.
Contributor
Author
There was a problem hiding this comment.
Good point, i've made a fix in 6c71ea2 where the targetNodeId becomes optional (and there is no default for migrated records)
| maxAttempts: Int, | ||
| routeParams: Option[RouteParams] = None) extends GenericSendPayment { | ||
| routeParams: Option[RouteParams] = None, | ||
| paymentRequest_opt: Option[PaymentRequest] = None) extends GenericSendPayment { |
Member
There was a problem hiding this comment.
I think that the paymentRequest_opt attribute should be moved upward in the constructor arguments list, so that it's alongside paymentHash, targetNodeId, and not alongside payment parameters arguments.
case class SendPayment(amountMsat: Long,
paymentHash: ByteVector32,
targetNodeId: PublicKey,
paymentRequest_opt: Option[PaymentRequest] = None,
assistedRoutes: Seq[Seq[ExtraHop]] = Nil,
finalCltvExpiry: Long = Channel.MIN_CLTV_EXPIRY,
maxAttempts: Int,
routeParams: Option[RouteParams] = None) extends ...
Member
|
Superceded by #1130 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Rationale
For various reasons the mobile app is getting rid of its own database and we need to store some extra info in the eclair-core database. This PR adds to each sent payment record the recipient node id, the payment request that is being paid and the deserialized description from the payment request. The description is stored in deserialized form to avoid potentlially costly operations on mobile.
Database layer changes
The
sent_paymentstable in 'payment' DB has been altered to include three new optional columns where we store the payment request that was being paid ( aspayment_request), the deserialized description and the node id of the recipient, all fields are nullable. Thesent_paymentstable is now migrated to version 3.Implementation details
The new feature is only exposed through eclair's internal API send and is not exposed via HTTP APIs.