[refactoring] Move methods out of TransactionHistoryBuilder
#11471
Conversation
… `SmartTransactionExtensions`. Co-authored-by: Turbolay <clement.ogame@gmail.com>
Co-authored-by: Turbolay <clement.ogame@gmail.com>
kiminuo
force-pushed
the
feature/2023-09-12-Initial-refactoring-of-TransactionHistoryBuilder
branch
from
3c7c949
to
f1fd6c9
Compare
|
|
||
| public static class SmartTransactionExtensions | ||
| { | ||
| public static IEnumerable<Output> GetOutputs(this SmartTransaction smartTransaction, Network network) |
There was a problem hiding this comment.
I don't like the method name here. Imho it should be more descriptive because SmartTransaction has already many output types.
| return GetDestinationAddresses(inputs, outputs); | ||
| } | ||
|
|
||
| private static IEnumerable<BitcoinAddress> GetDestinationAddresses(ICollection<IInput> inputs, ICollection<Output> outputs) |
There was a problem hiding this comment.
I don't like the method name here. Imho it should start with the Compute prefix and imho there should be a short documentation what it does.
| @@ -36,4 +36,46 @@ public static IEnumerable<IInput> GetInputs(this SmartTransaction transaction) | |||
|
|
|||
| return known.Concat(unknown); | |||
| } | |||
|
|
|||
| public static IEnumerable<BitcoinAddress> GetDestinationAddresses(this SmartTransaction transaction, Network network, out List<IInput> inputs, out List<Output> outputs) | |||
There was a problem hiding this comment.
Why did you prefer to explicitely pass the network instead of using Services.WalletManager.Network?
There was a problem hiding this comment.
I would say it's personal preference because it allows easy unit-testing. Also note that this is in the WalletWasabi project and not in the Fluent project.
It can change later if deemed necessary/useful.
|
|
||
| txRecordList.Add(new TransactionSummary(spenderTransaction, Money.Zero - coin.Amount, GetInputs(spenderTransaction), outputs, destinationAddresses)); | ||
| var destinationAddresses = spenderTransaction.GetDestinationAddresses(wallet.Network, out _, out List<Output> outputs); | ||
| txRecordList.Add(new TransactionSummary(spenderTransaction, Money.Zero - coin.Amount, spenderTransaction.GetInputs(), outputs, destinationAddresses)); |
There was a problem hiding this comment.
Input is there above but discarded. You could use them. Are those the same?
There was a problem hiding this comment.
Good catch, fixed.
kiminuo
deleted the
feature/2023-09-12-Initial-refactoring-of-TransactionHistoryBuilder
branch
This PR is meant to simplify #11458 and ease reviews. It should be easy to review the PR commit by commit.
Credits to @turbolay.