cardano-foundation / cardano-wallet

HTTP server & command-line for managing UTxOs and HD wallets in Cardano.
Apache License 2.0
758 stars 213 forks source link

[ADP-3272] Simplify handling of UTxOs in inner helper function of `balanceTx`. #4548

Closed jonathanknowles closed 3 months ago

jonathanknowles commented 3 months ago

Issue

ADP-3272

Description

This PR simplifies the handling of UTxOs within the inner helper function of balanceTransaction, so that it only has to handle two UTxO-related data structures, both supplied by the outer function:

As a bonus, because this PR moves the computation of UTxO-related data structures from the inner helper function to the outer function, the above data structures should now be evaluated at most once, instead of multiple times (once per strategy).

Notes

The balanceTransaction function delegates the main portion of its work to an inner helper function that is parameterised by SelectionStrategy. It initially calls the inner helper function with SelectionStrategyOptimal, but if that strategy fails, then it (potentially) calls the inner helper function a second time with SelectionStrategyMinimal.

In the event that the inner helper function is evaluated more than once (with two different strategies), we ideally want to avoid recomputing potentially expensive data structures that should be constant across both evaluations.

jonathanknowles commented 3 months ago

@Anviking wrote:

One request (in addition to the comment): could we rename balanceTransactionWithSelectionStrategyAndNoZeroAdaAdjustment to something else?

Sure!

Would balanceTransactionInner work for you?

Or perhaps we could use the following even shorter names:

Justification: the abbreviation Tx is already a de facto standard throughout the code. For example:

Anviking commented 3 months ago

Or perhaps we could use the following even shorter names:

balanceTx balanceTxInner

Sounds great to me, just note renaming Transaction -> Tx could cause some cascading breakage and changes (for instance BalanceSpec has a balanceTx helper)... Maybe separate PR would be better for that...

(Though btw, long term, I'd wonder if we couldn't structure things to not have to call something inner, but that's not of too much concern now)

jonathanknowles commented 3 months ago

Sounds great to me, just note renaming Transaction -> Tx could cause some cascading breakage and changes (for instance BalanceSpec has a balanceTx helper)... Maybe separate PR would be better for that...

I agree, it probably makes more sense to have a separate PR for any renaming of the public function. 👍🏻