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.

Sign up for GitHub

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

Jump to bottom

Record fees for self initiated sweeps to self (utxo management) #180

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Record fees for self initiated sweeps to self (utxo management) #180

merged 1 commit into from
Feb 8, 2024

Conversation

mrfelton
Copy link
Contributor

@mrfelton mrfelton commented Jan 27, 2024 •
edited
Loading

Create onchain fee entry for self initiated sweeps that pay to self. Ensures that fees relating to UTXO management (utxo consolidation or splitting) are included in the audit reports.

A note is added to the management entries to denote them as such.

Fixes #137

Example entry

{
    "timestamp":  "1706630501",
    "on_chain":  true,
    "amount":  "3300000",
    "credit":  false,
    "asset":  "BTC",
    "type":  "FEE",
    "custom_category":  "",
    "txid":  "2dfb78838f254a2042f3279d66e7017e5f5850b1d4ccf4d6e008e10851b5c095",
    "fiat":  "0",
    "reference":  "2dfb78838f254a2042f3279d66e7017e5f5850b1d4ccf4d6e008e10851b5c095:-1",
    "note":  "fees for utxo management transaction: 2dfb78838f254a2042f3279d66e7017e5f5850b1d4ccf4d6e008e10851b5c095",
    "btc_price":  {
        "price":  "0",
        "price_timestamp":  "0",
        "currency":  ""
    }
}

Questions

  1. What would be the most reliable way to identify these types of pay to self zero amount sweep transactions? The approach I'm using is to inspect all the inputs and outputs and verify that they belong to us.
  2. What entries should we have in the report for these types of transactions? Approach here is is to just create a single onchain FEE entry. But maybe we should also have an onchain payment and an onchain receive to go with it? Or, we could create it as SWEEP_FEE (and SWEEP) entry

@mrfelton mrfelton marked this pull request as ready for review January 28, 2024 10:50
@mrfelton mrfelton force-pushed the consolidation-fees branch 3 times, most recently from 119b246 to dc2efc5 Compare January 28, 2024 11:01
@mrfelton mrfelton changed the title Record fees for self initiated sweeps to self Record fees for self initiated sweeps to self (utxo management) Jan 29, 2024
@guggero guggero self-requested a review January 30, 2024 16:00
guggero
guggero reviewed Feb 5, 2024
View reviewed changes
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 PR. I think this gets the job done. Can't really think of an other or easier way to detect a manual consolidation TX.

Just to complete my understanding, how did such a transaction show up in faraday before?
Maybe it would be worth adding the scenario to the integration tests to make sure the result is what we expect.

Also, can you please squash your commits? Thanks.

accounting/entries.go Outdated Show resolved Hide resolved
accounting/entries.go Show resolved Hide resolved
accounting/entries_test.go Outdated Show resolved Hide resolved
@mrfelton
Copy link
Contributor Author

mrfelton commented Feb 5, 2024

Just to complete my understanding, how did such a transaction show up in faraday before?

It didn't. It was totally omitted.

Maybe it would be worth adding the scenario to the integration tests to make sure the result is what we expect.

I have added tests that prove that these types of transactions are now included in the report.

@mrfelton
Copy link
Contributor Author

mrfelton commented Feb 5, 2024

Also, can you please squash your commits? Thanks.

Comments addressed and commits squashed.

@mrfelton mrfelton requested a review from guggero February 5, 2024 21:49
guggero
guggero reviewed Feb 6, 2024
View reviewed changes
accounting/entries.go Outdated
}

// createOnchainFeeEntry creates a fee entry for an on chain transaction.
func createOnchainFeeEntry(
Copy link
Member

Choose a reason for hiding this comment

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

Can you please take a look at the formatting guideline doc I shared? Or the surrounding code? This isn't how we want our function definitions to be formatted.
Alternatively I can also push a fix commit to your branch if you don't want to deal with it.

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 missed the link in your prior comment. I've pushed an update with that style fix. Shame that make fmt doesn't handle that properly.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Shame that make fmt doesn't handle that properly.

Yeah, definitely. One of the biggest downsides of Go is that there is no configurable formatter such as prettier...

@mrfelton mrfelton requested a review from guggero February 6, 2024 08:26
@guggero guggero requested a review from bhandras February 6, 2024 08:51
bhandras
bhandras approved these changes Feb 7, 2024
View reviewed changes
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 for the fix @mrfelton 🥇

accounting/entries.go Outdated
@@ -260,6 +309,16 @@ func onChainEntries(tx lndclient.Transaction,
return nil, nil
}

// If this is a utxo management transaction, we return a fee entry only.
if utxoManagement {
feeEntry, err := createOnchainFeeEntry(tx, category, utxoManagementFeeNote(tx.TxHash), u)
Copy link
Member

Choose a reason for hiding this comment

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

style nit: line too long

accounting/entries.go

// createOnchainFeeEntry creates a fee entry for an on chain transaction.
func createOnchainFeeEntry(tx lndclient.Transaction, category string,
note string, u entryUtils) (*HarmonyEntry, error) {
Copy link
Member

Choose a reason for hiding this comment

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

style-nit: we usually add a new line to separate the function body a bit better visually if the signature is multiline.

accounting/entries_test.go Outdated
@@ -502,11 +502,12 @@ func TestSweepEntry(t *testing.T) {
// generation of a fee entry where applicable.
func TestOnChainEntry(t *testing.T) {
getOnChainEntry := func(amount btcutil.Amount,
hasFee bool, label string) []*HarmonyEntry {
hasFee bool, isUtxoManagement bool, label string, note string) []*HarmonyEntry {
Copy link
Member

Choose a reason for hiding this comment

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

style nit: line too long

@mrfelton
Copy link
Contributor Author

mrfelton commented Feb 7, 2024 •
edited
Loading

Thanks @bhandras I've addressed the style nits and force pushed an updated squashed commit

@guggero guggero merged commit 256bde5 into lightninglabs:master Feb 8, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero approved these changes

@bhandras bhandras bhandras approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Report fees for onchain transactions that split UTXOs
3 participants
@mrfelton @bhandras @guggero