Add `W.balanceTx` helper #4629

Anviking · 2024-06-13T15:47:01Z

Add W.balanceTx helper used by the following endpoints

constructTransaction
constructTransactionShared
balanceTransaction

such that

A future PR can make W.balanceTx automatically query the node for the UTxO corresponding to the inputs of the PartialTx (this is part of the fix for the integration test in the 8.11 branch; if we have the UTxO of the reference input, then Ledger.getMinFeeTxUtxo pp tx utxo will correctly account for minFeeRefScriptCoinsPerByte * totalRefScriptSize)
We can hide some balanceTx configuration (utxoAssumptions, argGenChange) to the api handlers

Comments

Issue Number

ADP-3373

Anviking · 2024-06-13T19:57:52Z

lib/wallet/src/Cardano/Wallet.hs

+    sharedWalletScriptLookup
+        :: SharedState (NetworkOf s) SharedKey -> Address -> CA.Script KeyHash
+    sharedWalletScriptLookup s addr = case fst (isShared addr s) of
+            Nothing ->
+                error $ "Some inputs selected by coin selection do not belong "
+                <> "to multi-signature wallet"
+            Just (ix,role) ->
+                let template = paymentTemplate s
+                    role' = case role of
+                        UtxoExternal -> CAShelley.UTxOExternal
+                        UtxoInternal -> CAShelley.UTxOInternal
+                        MutableAccount ->
+                            error "role is specified only for payment credential"
+                in replaceCosignersWithVerKeys role' template ix


Simply moved from constructUnbalancedSharedTransaction

Anviking · 2024-06-13T19:58:31Z

lib/api/src/Cardano/Wallet/Api/Http/Server.hs

-        :<|> balanceTransaction
-            shelley
-            (delegationAddressS @n)
-            (utxoAssumptionsForWallet ShelleyWallet)
-            mempty
+        :<|> balanceTransaction shelley


☝️ (nicer)

Anviking · 2024-06-13T20:01:00Z

lib/wallet/src/Cardano/Wallet.hs

+                (Shared.paymentTemplate (getState wallet))
+                (sharedWalletScriptLookup (getState wallet)
+                    . Convert.toWalletAddress)
+


Since we are in IO, we will be able to query the node for extra UTxO info here later.

lib/api/src/Cardano/Wallet/Api/Http/Shelley/Server.hs

Anviking · 2024-06-13T20:12:04Z

lib/wallet/src/Cardano/Wallet.hs

+    -> IO (Write.Tx era)
+balanceTx wrk pp timeTranslation partialTx = do
+    (utxo, wallet, _txs) <- liftIO $ readWalletUTxO wrk
+    -- FIXME: Why are we not using the availableUTxO here?


🤔

This is not the PR to change it, but it probably should be changed.

lib/wallet/src/Cardano/Wallet.hs

paweljakubas · 2024-06-14T10:01:43Z

lib/wallet/src/Cardano/Wallet.hs

@@ -3854,6 +3892,13 @@ data PoolRetirementEpochInfo = PoolRetirementEpochInfo
    }
    deriving (Eq, Generic, Show)

+throwOnErr :: (MonadIO m, Exception e) => Either e a -> m a


I wonder if those things should not be somewhere else to be reused by other modules and packages... Possible here https://github.com/cardano-foundation/cardano-wallet/blob/master/lib/wallet/src/UnliftIO/Compat.hs or somewhere close to it. Maybe this is proper place as it is not needed elsewhere

I agree it would be nice to co-locate these helpers.

But perhaps somewhere else other than UnliftIO, as that is specific to the unliftio way of doing things.

Thoughts?

paweljakubas

Looks like a nice code reshuffling that gives rise to simplification on api side and moving logic and details under the hood 👍 If everything passes we are good here IMHO

jonathanknowles

LGTM! Just have a small number of suggestions.

jonathanknowles · 2024-06-17T08:12:35Z

lib/api/src/Cardano/Wallet/Api/Http/Shelley/Server.hs

+toApiSerialisedTransaction maybeEncoding tx =
+    let
+        encoding = fromMaybe Base64Encoded maybeEncoding
+    in
+        ApiSerialisedTransaction (ApiT $ sealWriteTx tx) encoding


Just curious, why not:

Suggested change

toApiSerialisedTransaction maybeEncoding tx =

let

encoding = fromMaybe Base64Encoded maybeEncoding

in

ApiSerialisedTransaction (ApiT $ sealWriteTx tx) encoding

toApiSerialisedTransaction maybeEncoding tx =

ApiSerialisedTransaction (ApiT $ sealWriteTx tx) encoding

where

encoding = fromMaybe Base64Encoded maybeEncoding

(Saves both horizontal and vertical space.)

In this specific example I think I like how you see the flow of

maybeEncoding introduced (parameter)

maybeEncoding transformed into encoding

encoding is used

in order, as opposed to 1-3-2 with where.

lib/wallet/src/Cardano/Wallet.hs

jonathanknowles · 2024-06-17T08:16:02Z

lib/wallet/src/Cardano/Wallet.hs

@@ -3854,6 +3892,13 @@ data PoolRetirementEpochInfo = PoolRetirementEpochInfo
    }
    deriving (Eq, Generic, Show)

+throwOnErr :: (MonadIO m, Exception e) => Either e a -> m a


I agree it would be nice to co-locate these helpers.

But perhaps somewhere else other than UnliftIO, as that is specific to the unliftio way of doing things.

Thoughts?

lib/api/src/Cardano/Wallet/Api/Http/Shelley/Server.hs

- 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](https://buildkite.com/cardano-foundation/cardano-wallet/builds/5254#018fee3c-01ad-441e-bb7e-25acd78cb7f3); 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 self-assigned this Jun 13, 2024

Anviking force-pushed the anviking/ADP-3373/wrap-balanceTx branch 2 times, most recently from 03326ac to 92d8064 Compare June 13, 2024 17:49

Anviking commented Jun 13, 2024

View reviewed changes

Anviking marked this pull request as ready for review June 13, 2024 20:01

Anviking commented Jun 13, 2024

View reviewed changes

lib/api/src/Cardano/Wallet/Api/Http/Shelley/Server.hs Show resolved Hide resolved

Anviking commented Jun 13, 2024

View reviewed changes

Anviking mentioned this pull request Jun 13, 2024

Resolve PartialTx inputs over LSQ #4630

Merged

Anviking commented Jun 14, 2024

View reviewed changes

lib/wallet/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved

Anviking commented Jun 14, 2024

View reviewed changes

lib/wallet/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved

Anviking force-pushed the anviking/ADP-3373/wrap-balanceTx branch from f9aaec2 to 35e7807 Compare June 14, 2024 08:51

Anviking requested review from HeinrichApfelmus and paweljakubas June 14, 2024 09:16

paweljakubas reviewed Jun 14, 2024

View reviewed changes

paweljakubas approved these changes Jun 14, 2024

View reviewed changes

jonathanknowles approved these changes Jun 17, 2024

View reviewed changes

Anviking force-pushed the anviking/ADP-3373/wrap-balanceTx branch 2 times, most recently from 6b6f182 to 81b160c Compare June 24, 2024 19:15

Anviking added this pull request to the merge queue Jun 24, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 25, 2024

Anviking added this pull request to the merge queue Jun 25, 2024

Add W.balanceTx helper

620ff36

Anviking removed this pull request from the merge queue due to a manual request Jun 25, 2024

Anviking force-pushed the anviking/ADP-3373/wrap-balanceTx branch from 81b160c to 620ff36 Compare June 25, 2024 11:00

Anviking enabled auto-merge June 25, 2024 11:01

Anviking added this pull request to the merge queue Jun 25, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 25, 2024

Anviking added this pull request to the merge queue Jun 25, 2024

Merged via the queue into master with commit fae66a5 Jun 25, 2024
3 checks passed

Anviking deleted the anviking/ADP-3373/wrap-balanceTx branch June 25, 2024 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `W.balanceTx` helper #4629

Add `W.balanceTx` helper #4629

Anviking commented Jun 13, 2024 •

edited

Loading

Anviking Jun 13, 2024

Anviking Jun 13, 2024 •

edited

Loading

Anviking Jun 13, 2024

Anviking Jun 13, 2024 •

edited

Loading

paweljakubas Jun 14, 2024

jonathanknowles Jun 17, 2024

paweljakubas left a comment

jonathanknowles left a comment

jonathanknowles Jun 17, 2024

Anviking Jun 24, 2024

jonathanknowles Jun 17, 2024

Add W.balanceTx helper #4629

Add W.balanceTx helper #4629

Conversation

Anviking commented Jun 13, 2024 • edited Loading

Comments

Issue Number

Anviking Jun 13, 2024

Choose a reason for hiding this comment

Anviking Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Anviking Jun 13, 2024

Choose a reason for hiding this comment

Anviking Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

paweljakubas Jun 14, 2024

Choose a reason for hiding this comment

jonathanknowles Jun 17, 2024

Choose a reason for hiding this comment

paweljakubas left a comment

Choose a reason for hiding this comment

jonathanknowles left a comment

Choose a reason for hiding this comment

jonathanknowles Jun 17, 2024

Choose a reason for hiding this comment

Anviking Jun 24, 2024

Choose a reason for hiding this comment

jonathanknowles Jun 17, 2024

Choose a reason for hiding this comment

Add `W.balanceTx` helper #4629

Add `W.balanceTx` helper #4629

Anviking commented Jun 13, 2024 •

edited

Loading

Anviking Jun 13, 2024 •

edited

Loading

Anviking Jun 13, 2024 •

edited

Loading