Improve usages of wallet.GetTransactions()

[turbolay]

Basically found by @kiminuo

31533e4: Create wallet.GetTransaction() to perform more efficiently operations in CancelTransactionDialogViewModel and SpeedUpDialogViewModel. Also add some comments
b9c61aa: Avoid to recompute OrderByBlockchain on an object which is already ordered.

Note
It would probably be more efficient to iterate Coins from last to first in wallet.GetTransaction()

[kiminuo]

How about to do it like this kiminuo@c0ca01c? It's the same as what you did but we would have nicer API in Wallet (it's slightly worse in the UI code but it forces people to handle correctly that there might be no transaction found and support for TryGet in IDEs is very good lately)

[kiminuo]

Basically found by @kiminuo

31533e4: Create wallet.GetTransaction() to perform more efficiently operations in CancelTransactionDialogViewModel and SpeedUpDialogViewModel. Also add some comments b9c61aa: Avoid to recompute OrderByBlockchain on an object which is already ordered.

Note It would probably be more efficient to iterate Coins from last to first in wallet.GetTransaction()

It would be great to actually use TransactionStore for this but that storage has transactions for all wallets AFAIK so we can't do it right now. Too bad.

Even though, thinking about it right now: What if one just queries AllTransactionStore by the txid and then if a transaction is found we would just verify that it contains a coin of the wallet? WDYT? If this is a viable way then I would say we would make progress towards #11421.

[turbolay]

How about to do it like this kiminuo@c0ca01c?

Initially, I wanted to make a TryGetTransaction as well or ContainsTransaction as you suggested, but I discarded the options seeing that it would not play nicely with the usages. If you prefer like that, even considering that it reduces readability of current usages, I'm ok with that.

Even though, thinking about it right now: What if one just queries AllTransactionStore by the txid and then if a transaction is found we would just verify that it contains a coin of the wallet? WDYT? If this is a viable way then I would say we would make progress towards #11421.

To be clear, progressing towards #11421 is my priority. I've been wondering for quite some time how to do it properly. I believe that SmartCoin.Transaction should become a function fetching from the sql storage based on SmartCoin.TransactionId. In order to do so, we just need to add a new field, SmartCoin.SpenderTransactionId.

I've seen that in #11421 you're storing the txid. Great, so I completely agree with you: Wallet.GetTransactions and Wallet.TryGetTransaction should fetch from the AllTransactionStore. But in order to know if an Input/Output is part of the wallet, we have to call GetAllCoins, then enumerate with Contains for each Input/Output. Well, if we have both the TXIDs as I suggested above, better to:

• TryGetTransaction: first enumerate through all the coins to check if one has the txid as TransactionId/SpenderTransactionId (almost exactly as this PR/your suggestion is doing) then fetch the tx from db if found.
• GetTransactions: enumerate through all the coins, create the hashset of txids (almost exactly as we are doing) and select many from the db.

So in any case I believe it would be better to first implement this PR/your suggestion, then slightly update it while we are merging #11421

[kiminuo]

How about to do it like this kiminuo@c0ca01c?

Initially, I wanted to make a TryGetTransaction as well or ContainsTransaction as you suggested, but I discarded the options seeing that it would not play nicely with the usages. If you prefer like that, even considering that it reduces readability of current usages, I'm ok with that.

I think that TryGetTransaction is better. It's an idiom we use often lately. You are right about usages but then it's just a few lines of code, no biggie. Plus maybe that observable can be defined better for a TryGet* method, IDK.

Even though, thinking about it right now: What if one just queries AllTransactionStore by the txid and then if a transaction is found we would just verify that it contains a coin of the wallet? WDYT? If this is a viable way then I would say we would make progress towards #11421.

To be clear, progressing towards #11421 is my priority. I've been wondering for quite some time how to do it properly. I believe that SmartCoin.Transaction should become a function fetching from the sql storage based on SmartCoin.TransactionId. In order to do so, we just need to add a new field, SmartCoin.SpenderTransactionId.

I've seen that in #11421 you're storing the txid. Great, so I completely agree with you: Wallet.GetTransactions and Wallet.TryGetTransaction should fetch from the AllTransactionStore. But in order to know if an Input/Output is part of the wallet, we have to call GetAllCoins, then enumerate with Contains for each Input/Output. Well, if we have both the TXIDs as I suggested above, better to:

* `TryGetTransaction`: first enumerate through all the coins to check if one has the txid as `TransactionId`/`SpenderTransactionId` (almost exactly as this PR/your suggestion is doing) then fetch the tx from db if found.

* `GetTransactions`: enumerate through all the coins, create the hashset of txids (almost exactly as we are doing) and select many from the db.

So in any case I believe it would be better to first implement this PR/your suggestion, then slightly update it while we are merging #11421

I need to think about this a bit, I'll leave it open in a tab and I'll respond later.

[kiminuo]

Could we go with #11493 (comment) and then go with #11493 (comment) as a follow-up?

[turbolay]

I pushed your code.
Something I thought about is that it might also be great to have an API with direct filtering, a bit similar to what we are doing in the KeyManager

Like this with a predicate:
https://github.com/zkSNACKs/WalletWasabi/blob/fe2bcd19748981875e4981f78352bf618a8012f1/WalletWasabi/Blockchain/Keys/KeyManager.cs#L375-L388

Or this with some fields with filtering:
https://github.com/zkSNACKs/WalletWasabi/blob/fe2bcd19748981875e4981f78352bf618a8012f1/WalletWasabi/Blockchain/Keys/KeyManager.cs#L390-L396

I believe it would make sense instead of having to filter afterwards, like what we are doing in both usages: .WhereNotNull().Where(s => s.Confirmed)

[kiminuo]

I pushed your code.

Thank you

Something I thought about is that it might also be great to have an API with direct filtering, a bit similar to what we are doing in the KeyManager

Like this with a predicate:

https://github.com/zkSNACKs/WalletWasabi/blob/fe2bcd19748981875e4981f78352bf618a8012f1/WalletWasabi/Blockchain/Keys/KeyManager.cs#L375-L388

Or this with some fields with filtering:

https://github.com/zkSNACKs/WalletWasabi/blob/fe2bcd19748981875e4981f78352bf618a8012f1/WalletWasabi/Blockchain/Keys/KeyManager.cs#L390-L396

I believe it would make sense instead of having to filter afterwards, like what we are doing in both usages: .WhereNotNull().Where(s => s.Confirmed)

I think it might be good to do it. We will work with SQLite in similar way because there we can easily filter using WHERE and we need to specify those filter conditions. So I think it makes good sense to identify the most common filter criteria (this would in turn tell us what are good indices for SQLite to add) and do what KeyManager does. It would move us closer to the "database approach".

[kiminuo]

LGTM