Skip to content
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

Include outgoing payment description in audit memo #182

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

mrfelton
Copy link
Contributor

@mrfelton mrfelton commented Feb 9, 2024

Include invoice memo in audit nodes for outgoing payments that pay to an invoice. This allows for more options to correlate money movements during reconciliation.

Context: Improves ability to reconcile payments related to peerswaps. See ElementsProject/peerswap#254 for additional context.

This pull request involves changes to the accounting package, specifically the entries.go, entries_test.go, filter.go, and filter_test.go files. The changes primarily involve the addition of a description field to the paymentInfo struct and the modification of the paymentNote function to include the memo field. The changes also involve modifications to the paymentEntry and paymentRequestDetails functions to accommodate the new description field.

Fix #181

Changes to entries.go and entries_test.go:

  • func paymentReference(sequenceNumber uint64, preimage lntypes.Preimage) string {: Modified the paymentNote function to include a memo field and to append the memo and dest fields to a notes slice. The function now returns a string joined by the notes slice elements instead of just the dest string.
  • func paymentEntry(payment paymentInfo, paidToSelf bool,: Modified the paymentNote function call in the paymentEntry function to include the payment.description field.
  • func TestPaymentEntry(t *testing.T) {: Modified the paymentNote function call in the TestPaymentEntry test function to include the &invoiceMemo field. [1] [2]

Changes to filter.go and filter_test.go:

  • type paymentInfo struct {: Added a description field to the paymentInfo struct.
  • func preProcessPayments(payments []lndclient.Payment,: Modified the preProcessPayments function to include the description field in the paymentInfo struct.
  • func paymentRequestDetails(paymentRequest string, decode decodePaymentRequest) (*route.Vertex, *string, error) {: Renamed the paymentRequestDestination function to paymentRequestDetails and modified it to return a description field in addition to the destination field.
  • func TestPaymentRequestDestination(t *testing.T) {: Renamed the TestPaymentRequestDestination test function to TestPaymentRequestDetails and modified it to test the description field in addition to the destination field. [1] [2]

@mrfelton mrfelton force-pushed the payment-description branch 3 times, most recently from a19de94 to c05f82a Compare February 9, 2024 21:25
@mrfelton mrfelton marked this pull request as ready for review February 9, 2024 21:26
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvement!
One question about order of precedence.

accounting/filter.go Outdated Show resolved Hide resolved
}

if destination == nil {
destination, _ = paymentHtlcDestination(payment)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change alters the preference. Before, the data from paymentHtlcDestination took preference over paymentRequestDestination(). Now it's the other way around.

Copy link
Contributor Author

@mrfelton mrfelton Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to inspect the payment request in order to get the description. So, given that we will always be inspecting the payment request if one exists, it seems that inspecting the HTLCs only makes sense as a fallback in case we weren't able to get the destination from the payment request.

Is there an expectation that the destination obtained from the HTLC's could be different from the destination in the payment request?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm not sure. That's why I'm not really comfortable changing the logic here. Would need to dig into the lnd code and docs to find out. But maybe @carlaKC has a quick answer for me?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed an additional commit which returns the original order of precedence for the destination

c478467

It does add some additional overhead though since in many cases we'd be inspecting HTLCs to find the destination even though we already know it from inspecting the payment request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please squash the two commits?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mrfelton mrfelton force-pushed the payment-description branch 3 times, most recently from 5598875 to fa72d02 Compare February 12, 2024 18:17
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, LGTM 🎉

Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @mrfelton 🎉

accounting/entries.go Outdated Show resolved Hide resolved
}

if destination == nil {
destination, _ = paymentHtlcDestination(payment)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please squash the two commits?

@mrfelton
Copy link
Contributor Author

Thanks for the reviews. Addressed the style nit and squashed all the commits.

@guggero guggero merged commit 9f4a63a into lightninglabs:master Feb 14, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include invoice description from outgoing payments in audit notes
3 participants