transaction search with destination address

[collins-okafor]

This pull request makes the following changes:

• Fixes issue Make destination address a search result #12527

[collins-okafor]

Hi @lontivero
This PR is ready for review

[turbolay]

I'm not able to review 100% this PR, but many things are incorrect:

• We always work with the current network, so for example we don't want to search for transactions that are on Testnet when we are on Main
• We do not use try/catch as part of the logic that way
• The logic is really weird because you first search in TransactionStore, which will search in absolutely all transactions, but then apply the results on transactions of loaded wallets. Well, why not look directly in the transactions of the loaded wallets?
• Your function GetDestinationAddresses is really weird because it takes a DestinationAddress and returns a DestinationAddress... Something is clearly wrong.
• At least one variable is created but unused
• Have you tested your PR? Because it doesn't work correctly, and finds too many results.

Here would be my approach for this PR, which is based on your idea but greatly simplifies the logic. It's still not perfect for several reasons, and takes a design choice of presenting in the same way a found transaction Id and an address found as destination in a transaction, which might be rejected, but you can modify your PR based on this code:

index 7f59d1e92..e54aeffe0 100644
--- a/WalletWasabi.Fluent/ViewModels/SearchBar/Sources/TransactionsSearchSource.cs
+++ b/WalletWasabi.Fluent/ViewModels/SearchBar/Sources/TransactionsSearchSource.cs
@@ -4,6 +4,7 @@ using System.Reactive.Disposables;
 using System.Reactive.Linq;
 using System.Threading.Tasks;
 using DynamicData;
+using NBitcoin;
 using ReactiveUI;
 using WalletWasabi.Fluent.Extensions;
 using WalletWasabi.Fluent.ViewModels.SearchBar.Patterns;
@@ -49,6 +50,13 @@ public class TransactionsSearchSource : ReactiveObject, ISearchSource, IDisposab
                return historyItemViewModelBase.Transaction.Id.ToString().Contains(queryStr, StringComparison.CurrentCultureIgnoreCase);
        }
 
+       private static bool ContainsDestinationAddress((WalletViewModel, HistoryItemViewModelBase) tupleWalletHistoryItem, string queryStr)
+       {
+               var txid = tupleWalletHistoryItem.Item2.Transaction.Id;
+               return tupleWalletHistoryItem.Item1.WalletModel.Transactions.GetDestinationAddresses(txid)
+                       .Contains(BitcoinAddress.Create(queryStr, tupleWalletHistoryItem.Item1.WalletModel.Network));
+       }
+
        private static Task NavigateTo(WalletViewModel wallet, HistoryItemViewModelBase item)
        {
                var walletPageViewModel = MainViewModel.Instance.NavBar.Wallets.FirstOrDefault(x => x.WalletViewModel == wallet);
@@ -117,6 +125,6 @@ public class TransactionsSearchSource : ReactiveObject, ISearchSource, IDisposab
        private static IEnumerable<(WalletViewModel, HistoryItemViewModelBase)> Filter(string queryStr)
        {
                return Flatten(GetTransactionsByWallet())
-                       .Where(tuple => ContainsId(tuple.Item2, queryStr));
+                       .Where(tuple => ContainsId(tuple.Item2, queryStr) || ContainsDestinationAddress(tuple, queryStr));
        }
 }

[collins-okafor]

Thanks for the corrections and suggestions. I will look into all of them now.

[collins-okafor]

Hi @turbolay,
Pls, check this out. I've made some changes based on the suggestions you gave earlier. The issue is resolved now. Though had to put a check before calling the ContainsDestinationAddress method as it expects only addresses and not a string (txid).

[turbolay]

Thanks, it's much better!

Still few minor comments:

• Dont create unused variables, instead discard the returned result
• Follow the style, maybe you need CodeMaid or configure your IDE with StyleCop? See https://github.com/zkSNACKs/WalletWasabi/blob/master/CONTRIBUTING.md
• Prefer lambda expression rather than if/else if readability is still good
• Use as small objects as you can as arguments (eg: don't pass the whole WalletModel if you only need one field out of it, the Network)

I applied those suggestions here: 0ad5222
Can you cherry-pick this commit and add it to your branch, please? You can simply apply the diff if you prefer.

Your logic still contains a try/catch but I think it's OK, I don't know how to do it better, maybe others will.

[collins-okafor]

Thanks a lot @turbolay

All corrections have been applied.

[turbolay]

tACK

[soosr]

last bit

[soosr]

tACK

[yahiheb]

tACK

[turbolay]

Congrats @collins-okafor !!! Nice PR handling, thank you for your contribution.

[MaxHillebrand]

Thanks @collins-okafor and congrats on your first merged PR, much appreciated!

[collins-okafor]

Congrats @collins-okafor !!! Nice PR handling, thank you for your contribution.

Glad I could contribute. And thanks for the corrections and feedbacks. I really appreciate. Looking forward to doing more😊