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

wallet: add gettransaction #890

Merged
merged 2 commits into from
Oct 30, 2023
Merged

Conversation

ErikEk
Copy link
Contributor

@ErikEk ErikEk commented Sep 23, 2023

This PR adds a method to the wallet that allows to get transaction data on any transaction given its ID. Useful for (lightningnetwork/lnd#7654).

@ErikEk ErikEk changed the title add gettransaction wallet: add gettransaction Sep 23, 2023
@ErikEk ErikEk marked this pull request as ready for review September 23, 2023 17:08
@guggero guggero self-requested a review September 25, 2023 09:03
Copy link
Collaborator

@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 addition, should definitely be useful for different use cases in lnd.

wallet/wallet.go Outdated Show resolved Hide resolved
wallet/wallet.go Outdated
return nil, err
}

var res GetTransactionResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have this be the only variable outside of the walletdb.View call and move the switch below inside the callback? That way it's more clear what the result will be if txDetail is nil.

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. Now I return an error if the transaction isn't found instead.

@guggero guggero self-requested a review September 27, 2023 07:56
Copy link
Collaborator

@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.

Getting pretty close!
Main ask is a unit test.

wallet/wallet.go Show resolved Hide resolved
wallet/wallet.go Show resolved Hide resolved
wallet/wallet.go Outdated Show resolved Hide resolved
wallet/wallet.go Show resolved Hide resolved
@yyforyongyu
Copy link
Collaborator

feel free to ping me when it's ready for review

@ErikEk ErikEk force-pushed the add-gettransaction branch 2 times, most recently from 72043ca to 8737067 Compare October 12, 2023 07:55
Copy link
Collaborator

@yyforyongyu yyforyongyu 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! Left some comments about tests and the returned format.

wallet/wallet.go Show resolved Hide resolved
wallet/wallet_test.go Outdated Show resolved Hide resolved
wallet/wallet_test.go Show resolved Hide resolved
@ErikEk ErikEk force-pushed the add-gettransaction branch 4 times, most recently from 67495ab to df18e0d Compare October 18, 2023 18:10
@ErikEk ErikEk requested a review from yyforyongyu October 19, 2023 13:46
@ErikEk ErikEk force-pushed the add-gettransaction branch 2 times, most recently from 5c3b5d8 to 5c19b97 Compare October 20, 2023 12:52
wallet/wallet.go Outdated Show resolved Hide resolved
wallet/wallet_test.go Outdated Show resolved Hide resolved
wallet/wallet_test.go Outdated Show resolved Hide resolved
wallet/wallet_test.go Outdated Show resolved Hide resolved

// If the transaction should be known to the store, we
// write txdetail to disk.
if test.txKnown {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this approach works, tho imo a better version would be creating testing a txMined and txUnmined, then define the table tests as

{
  name string
  txid chainhash.Hash
  expectedTx *GetTransactionResult
  expectedErr error
}

Then you construct three txids - txMined = ..., txUnmined= ..., and txUnknown = .... And finally check return returned values from GetTransaction,

require.ErrorIs(t, tc.expectedErr, err)
require.Equal(t, tc.expectedTx, tx)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yyforyongyu I have updated the test code. I am not sure if you wanted me to add all transactions to the wallet storage at once or one for each test cycle like I have done now?

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 had issues storing the full GetTransactionResults array and opted for comparing the txid and block height as validators.

@ErikEk ErikEk force-pushed the add-gettransaction branch 3 times, most recently from 757ced5 to 998ee11 Compare October 27, 2023 07:30
Copy link
Collaborator

@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.

Nice, LGTM 🎉

wallet/wallet.go Outdated Show resolved Hide resolved
@ErikEk ErikEk force-pushed the add-gettransaction branch from 998ee11 to e099daa Compare October 27, 2023 08:56
Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

LGTM🙏

wallet/wallet_test.go Show resolved Hide resolved
wallet/wallet_test.go Show resolved Hide resolved
@ErikEk ErikEk force-pushed the add-gettransaction branch from e099daa to 4453270 Compare October 30, 2023 16:26
@ErikEk
Copy link
Contributor Author

ErikEk commented Oct 30, 2023

@yyforyongyu PR updated.

@Roasbeef Roasbeef merged commit 9c13542 into btcsuite:master Oct 30, 2023
3 checks passed
buck54321 pushed a commit to buck54321/btcwallet that referenced this pull request Apr 21, 2024
buck54321 pushed a commit to buck54321/btcwallet that referenced this pull request Apr 21, 2024
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.

4 participants