Resolve PartialTx inputs over LSQ

[Anviking]

• Add getUTxOByTxIn local state query
• Resolve PartialTx inputs over LSQ in Cardano.Wallet.balanceTx

Comments

This is part of the fix for the integration test in the 8.11 branch; if we have the UTxO of the reference input containing the script used for minting in constructTx, then Ledger.getMinFeeTxUtxo pp tx utxo will correctly account for minFeeRefScriptCoinsPerByte * totalRefScriptSize.

Previous step: #4629
Next step: #4631

Issue Number

ADP-3373

[Anviking]

Not sure if this is already done under the hood, but it's probably worth adding a special case for mempty -> mempty. Then we'd incur no additional performance cost in the common path. No exchange and daedalus related tx creation will construct PartialTxs with existing inputs for lookup. Only the balanceTx endpoint, and constructTx in the specific case of minting using a reference input.

[HeinrichApfelmus]

I agree that it's worth adding the special case — we don't want to do an IO roundtrip over the node socket if we already know the answer.

But which era should the result be in?

At the very least, calling getUTxOByTxIn will entail a query about the current node era — which we would need to cache in order to avoid sending something to the node.

[Anviking]

But which era should the result be in?

Right, good point. Luckily we already have access to a cached value of the era, so it should be easy.

I think we could also imagine using your Data.These here with something isomorophic to Maybe (Data.These (UTxO ConwayEra) (UTxO BabbageEra)), to be able to return Just $ These mempty mempty, but doesn't seem worth it for this problem alone (particularly as Cardano.Wallet.balanceTx is still uncertain about the UTxO's era even when the node was queried)

[Anviking]

1272251

[HeinrichApfelmus]

Thank you! 😊

One request concerning the location of the new helper functions in balanceTx.

On the implementation of getUTxOByTxIn, I agree that it's worth adding the special case for mempty, because that avoids a roundtrip over the node socket. However, this requires information about the era in which we want to return the result. We can do this optimization later when cleaning up the NetworkLayer.

[HeinrichApfelmus]

I think that we want

Suggested change

	, getUTxOByTxIn
	:: Set Write.TxIn
	-> m (MaybeInRecentEra Write.UTxO)
	, getUTxOByTxIn
	:: Set Read.TxIn
	-> m (Read.EraValue Read.UTxO)

at some point here — the function simply returns the UTxO in whatever era the node is currently in. But we can adjust that later, your version is fine for now.

[HeinrichApfelmus]

I agree that it's worth adding the special case — we don't want to do an IO roundtrip over the node socket if we already know the answer.

But which era should the result be in?

At the very least, calling getUTxOByTxIn will entail a query about the current node era — which we would need to cache in order to avoid sending something to the node.

[HeinrichApfelmus]

Can you move these helper functions to Internal.Cardano.Write.Tx?

These function relate to the issue of converting MaybeInRecentEra Write.UTxO to a specific Write.UTxO era; their internal workings should not be a concern of balanceTx.

Notation: Instead of calling the function assumeUTxOInEra, I would call the function forceUTxOInEra, because the function can forcefully downgrade a Conway UTxO to a Babbage UTxO.

[jonathanknowles]

I would call the function forceUTxOInEra

In that case, would suggest changing "in" to "to", making it forceUTxOToEra.

[Anviking]

Moved

forceUTxOToEra
    :: forall era1 era2. (IsRecentEra era1, IsRecentEra era2)
    => UTxO era1
    -> Either ErrInvalidTxOutInEra (UTxO era2)
forceUTxOToEra (UTxO utxo) =
    utxoFromTxOutsInRecentEra
    $ map (second wrapTxOutInRecentEra)
    $ Map.toList utxo

to Write.Tx, but left the dealing with the MaybeInRecentEra in a helper in Cardano.Wallet.balanceTx.

[HeinrichApfelmus]

I'm not particularly fond of the TxOutInRecentEra type and the wrappings / unwrappings — why not directly type TxOutInRecentEra = TxOut LatestLedgerEra?

But we can fix that later, this is fine for now.

[Anviking]

Yes — I started questioning it as well. Will fix in another PR 👍 (Would go with newtype though to avoid mixup)

[Anviking]

On the other hand — defining TxOutInRecentEra in terms of the address, value, datum and script directly makes it a little more easy to use, e.g. in tests.

[HeinrichApfelmus]

On the other hand — defining TxOutInRecentEra in terms of the address, value, datum and script directly makes it a little more easy to use, e.g. in tests.

Sure — but that's a general concern: Which functions do we have available for working with TxOut?

The answer that we have chosen by introducing Write is: "Whatever functions the cardano-ledger libraries offer".

If we don't like that answer, we can add our own taste to it, i.e. offer custom functions, lenses or even pattern synonyms that operate on TxOut LatestLedgerEra — but I would stop before defining new types, because that is then outside of the answer above.