wallet: add GetTransaction returning data for any transaction #2
Conversation
| @@ -2359,6 +2359,149 @@ func (w *Wallet) GetTransactions(startBlock, endBlock *BlockIdentifier, | |||
| return &res, err | |||
| } | |||
|
|
|||
| // GetTransaction returns data if any transaction given its id. A mined | |||
There was a problem hiding this comment.
| // GetTransaction returns data if any transaction given its id. A mined | |
| // GetTransaction returns data of any transaction given its id. A mined |
| @@ -2359,6 +2359,149 @@ func (w *Wallet) GetTransactions(startBlock, endBlock *BlockIdentifier, | |||
| return &res, err | |||
| } | |||
|
|
|||
| // GetTransaction returns data if any transaction given its id. A mined | |||
| // transaction is returned in a Block structure which records properties about | |||
| // the block. A bool is also returned to denote if the transaction did exist | |||
There was a problem hiding this comment.
| // the block. A bool is also returned to denote if the transaction did exist | |
| // the block. A bool is also returned to denote if the transaction previously existed |
| header, err := client.GetBlockHeaderVerbose(blockHash) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| blockHeight = header.Height | ||
| case *chain.BitcoindClient: | ||
| var err error | ||
| blockHeight, err = client.GetBlockHeight(blockHash) |
There was a problem hiding this comment.
It's a bit weird how you sometimes declare var err error upfront, and sometimes you use :=. I'd say to do either one or the other consistently instead of mixing the two. Not sure if LND has some convention here but I'd always use := unless I really need to create the variable without initializing it. (Such as before assigning it in both branches of an if or something).
There was a problem hiding this comment.
It actually depends on if a return variable would otherwise overshadow a variable in the scope or not. In this case we are interested in the block height, which is directly returned for the BitcoindClient and therefore directly set to the variable blockHeight. But for the RPCClient instead we first have to declare a local return variable header with := that contains the height and then set the variable blockHeight in the scope to its Height value.
| // database. | ||
| var summary *TransactionSummary | ||
| var inDB bool | ||
| err = walletdb.View(w.db, func(dbtx walletdb.ReadTx) error { |
There was a problem hiding this comment.
It seems like this err is never checked? I suppose that is because rangeFn always returns nil. But in that case I'd use _ instead of assigning to err.
There was a problem hiding this comment.
You are right, I should add it to the return statement!
| // transaction is returned in a Block structure which records properties about | ||
| // the block. A bool is also returned to denote if the transaction did exist | ||
| // in the database or not. | ||
| func (w *Wallet) GetTransaction(txHash *chainhash.Hash) (*GetTransactionsResult, |
There was a problem hiding this comment.
Hm. Looking at the way you use this method from lnd-strike and also the fact that you always return either one mined or one unmined txn, I think this is not the most appropriate return type. We probably need a new type for this, maybe something like
type GetTransactionResult struct {
MinedTransactions *Block
UnminedTransactions *TransactionSummary
}And also we can document that at least one of the fields will be null. Possibly both if we can't find the txn.
There was a problem hiding this comment.
Good point! I was a bit reluctant to do that as I thought this would involve changing the protobuf but we can simply declare the type above the function.
No description provided.