-
Notifications
You must be signed in to change notification settings - Fork 594
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
Conversation
0d52d8c
to
7a4ae82
Compare
7a4ae82
to
fe351af
Compare
There was a problem hiding this 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
return nil, err | ||
} | ||
|
||
var res GetTransactionResult |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
fe351af
to
6f11990
Compare
There was a problem hiding this 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.
feel free to ping me when it's ready for review |
72043ca
to
8737067
Compare
There was a problem hiding this 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.
67495ab
to
df18e0d
Compare
5c3b5d8
to
5c19b97
Compare
wallet/wallet_test.go
Outdated
|
||
// If the transaction should be known to the store, we | ||
// write txdetail to disk. | ||
if test.txKnown { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
757ced5
to
998ee11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, LGTM 🎉
998ee11
to
e099daa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM🙏
e099daa
to
4453270
Compare
@yyforyongyu PR updated. |
wallet: add gettransaction
wallet: add gettransaction
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).