Remove 'coin_selection' field from the Balance API's response

  (a) the implementation of this field was wrong, and getting it right is quite involved as it requires revising several parts of the core wallet. It is perhaps not desirable / out-of-scope for this endpoint actually.

  (b) the coin_selection result is only truly useful when constructing a transaction which is under control of the wallet. Balancing may be done on any arbitrary foreign transaction and as such, breaks several invariants of the wallet (only one withdrawal, only one delegation certificate at a time etc..).

lib/core-integration/src/Test/Integration/Framework/DSL.hs

@@ -241,8 +241,8 @@ import Cardano.Wallet.Api.Types
     , ApiMaintenanceAction (..)
     , ApiNetworkInformation
     , ApiNetworkParameters (..)
+    , ApiSerialisedTransaction
     , ApiSharedWallet (..)
-    , ApiSignedTransaction
     , ApiStakePool
     , ApiT (..)
     , ApiTransaction
@@ -2250,7 +2250,7 @@ signTx ctx w apiTx = do
                            }|]
     let signEndpoint = Link.signTransaction @'Shelley w
     getFromResponse (#transaction) <$>
-        request @ApiSignedTransaction ctx signEndpoint Default toSign
+        request @ApiSerialisedTransaction ctx signEndpoint Default toSign
 
 submitTx
     :: MonadUnliftIO m

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

@@ -32,7 +32,7 @@ import Cardano.Wallet.Api.Types
     , ApiCoinSelectionInput (..)
     , ApiConstructTransaction (..)
     , ApiFee (..)
-    , ApiSignedTransaction
+    , ApiSerialisedTransaction
     , ApiStakePool
     , ApiT (..)
     , ApiTransaction
@@ -933,7 +933,7 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
                                }|]
         let signEndpoint = Link.signTransaction @'Shelley wa
         signedTx <- getFromResponse #transaction <$>
-            request @ApiSignedTransaction ctx signEndpoint Default toSign
+            request @ApiSerialisedTransaction ctx signEndpoint Default toSign
 
         -- Submit tx
         txId <- submitTx ctx signedTx [ expectResponseCode HTTP.status202 ]
@@ -1064,12 +1064,11 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
                   }
               ]
             }|]
-        rTx <- request @(ApiConstructTransaction n) ctx
+        rTx <- request @ApiSerialisedTransaction ctx
             (Link.balanceTransaction @'Shelley wa) Default balancePayload
         verify rTx
             [ expectSuccess
             , expectResponseCode HTTP.status202
-            , expectField (#coinSelection . #inputs) (`shouldSatisfy` (not . null))
             ]
 
     it "TRANS_NEW_BALANCE_01e - plutus with missing covering inputs wallet enough funds" $ \ctx -> runResourceT $ do
@@ -1093,12 +1092,11 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
                   }
               ]
           }|]
-        rTx <- request @(ApiConstructTransaction n) ctx
+        rTx <- request @ApiSerialisedTransaction ctx
             (Link.balanceTransaction @'Shelley wa) Default balancePayload
         verify rTx
             [ expectSuccess
             , expectResponseCode HTTP.status202
-            , expectField (#coinSelection . #inputs) (`shouldSatisfy` (not . null))
             ]
 
     it "TRANS_NEW_SIGN_01 - Sign single-output transaction" $ \ctx -> runResourceT $ do
@@ -1117,7 +1115,7 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
                 }|]
         let signEndpoint = Link.signTransaction @'Shelley w
         signedTx <- getFromResponse #transaction <$>
-            request @ApiSignedTransaction ctx signEndpoint Default toSign
+            request @ApiSerialisedTransaction ctx signEndpoint Default toSign
 
         -- Submit tx
         void $ submitTx ctx signedTx [ expectResponseCode HTTP.status202 ]
@@ -1151,7 +1149,7 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
                 }|]
         let signEndpoint = Link.signTransaction @'Shelley w
         signedTx <- getFromResponse #transaction
-            <$> request @ApiSignedTransaction ctx signEndpoint Default toSign
+            <$> request @ApiSerialisedTransaction ctx signEndpoint Default toSign
 
         -- Submit tx
         void $ submitTx ctx signedTx [ expectResponseCode HTTP.status202 ]
@@ -1183,7 +1181,7 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
                 }|]
         let signEndpoint = Link.signTransaction @'Shelley w
         signedTx <- getFromResponse #transaction <$>
-            request @ApiSignedTransaction ctx signEndpoint Default toSign
+            request @ApiSerialisedTransaction ctx signEndpoint Default toSign
 
         -- Submit Tx
         -- TODO:
@@ -1234,7 +1232,7 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
                     , "passphrase": #{fixturePassphrase}
                     }|]
             (_, signedTx) <- second (view #transaction) <$>
-                unsafeRequest @ApiSignedTransaction ctx signEndpoint toSign
+                unsafeRequest @ApiSerialisedTransaction ctx signEndpoint toSign
 
             -- Submit
             txid <- submitTx ctx signedTx [ expectResponseCode HTTP.status202 ]
@@ -1258,7 +1256,7 @@ spec = describe "NEW_SHELLEY_TRANSACTIONS" $ do
                     , "passphrase": #{fixturePassphrase}
                     }|]
             (_, signedTx') <- second (view #transaction) <$>
-                unsafeRequest @ApiSignedTransaction ctx signEndpoint toSign'
+                unsafeRequest @ApiSerialisedTransaction ctx signEndpoint toSign'
 
             -- Submit
             void $ submitTx ctx signedTx' [ expectResponseCode HTTP.status202 ]

lib/core/src/Cardano/Wallet/Api.hs

@@ -191,11 +191,11 @@ import Cardano.Wallet.Api.Types
     , ApiPostRandomAddressData
     , ApiPutAddressesDataT
     , ApiSelectCoinsDataT
+    , ApiSerialisedTransaction
     , ApiSharedWallet
     , ApiSharedWalletPatchData
     , ApiSharedWalletPostData
     , ApiSignTransactionPostData
-    , ApiSignedTransaction
     , ApiStakeKeysT
     , ApiT
     , ApiTransactionT
@@ -541,7 +541,7 @@ type SignTransaction n = "wallets"
     :> Capture "walletId" (ApiT WalletId)
     :> "transactions-sign"
     :> ReqBody '[JSON] ApiSignTransactionPostData
-    :> PostAccepted '[JSON] ApiSignedTransaction
+    :> PostAccepted '[JSON] ApiSerialisedTransaction
 
 -- | https://input-output-hk.github.io/cardano-wallet/api/#operation/postTransaction
 type CreateTransactionOld n = "wallets"
@@ -586,7 +586,7 @@ type BalanceTransaction n = "wallets"
     :> Capture "walletId" (ApiT WalletId)
     :> "transactions-balance"
     :> ReqBody '[JSON] (ApiBalanceTransactionPostDataT n)
-    :> PostAccepted '[JSON] (ApiConstructTransactionT n)
+    :> PostAccepted '[JSON] ApiSerialisedTransaction
 
 {-------------------------------------------------------------------------------
                                  Shelley Migrations
@@ -863,7 +863,7 @@ type SignByronTransaction n = "byron-wallets"
     :> Capture "walletId" (ApiT WalletId)
     :> "transactions-sign"
     :> ReqBody '[JSON] ApiSignTransactionPostData
-    :> PostAccepted '[JSON] ApiSignedTransaction
+    :> PostAccepted '[JSON] ApiSerialisedTransaction
 
 -- | https://input-output-hk.github.io/cardano-wallet/api/#operation/postByronTransaction
 type CreateByronTransactionOld n = "byron-wallets"

lib/core/src/Cardano/Wallet/Api/Client.hs

@@ -75,8 +75,8 @@ import Cardano.Wallet.Api.Types
     , ApiPostRandomAddressData
     , ApiPutAddressesDataT
     , ApiSelectCoinsDataT
+    , ApiSerialisedTransaction (..)
     , ApiSignTransactionPostData
-    , ApiSignedTransaction (..)
     , ApiStakeKeysT
     , ApiT (..)
     , ApiTransactionT
@@ -166,7 +166,7 @@ data TransactionClient = TransactionClient
     , signTransaction
         :: ApiT WalletId
         -> ApiSignTransactionPostData
-        -> ClientM ApiSignedTransaction
+        -> ClientM ApiSerialisedTransaction
     , postTransaction
         :: ApiT WalletId
         -> PostTransactionOldDataT Aeson.Value
@@ -193,7 +193,7 @@ data TransactionClient = TransactionClient
     , balanceTransaction
         :: ApiT WalletId
         -> ApiBalanceTransactionPostDataT Aeson.Value
-        -> ClientM (ApiConstructTransactionT Aeson.Value)
+        -> ClientM ApiSerialisedTransaction
     }
 
 data AddressClient = AddressClient

lib/core/src/Cardano/Wallet/Api/Server.hs

@@ -230,13 +230,13 @@ import Cardano.Wallet.Api.Types
     , ApiPutAddressesData (..)
     , ApiScriptTemplateEntry (..)
     , ApiSelectCoinsPayments
+    , ApiSerialisedTransaction (..)
     , ApiSharedWallet (..)
     , ApiSharedWalletPatchData (..)
     , ApiSharedWalletPostData (..)
     , ApiSharedWalletPostDataFromAccountPubX (..)
     , ApiSharedWalletPostDataFromMnemonics (..)
     , ApiSignTransactionPostData (..)
-    , ApiSignedTransaction
     , ApiSlotId (..)
     , ApiSlotReference (..)
     , ApiStakeKeys (..)
@@ -1853,7 +1853,7 @@ signTransaction
     => ctx
     -> ApiT WalletId
     -> ApiSignTransactionPostData
-    -> Handler ApiSignedTransaction
+    -> Handler ApiSerialisedTransaction
 signTransaction ctx (ApiT wid) body = do
     let pwd = coerce $ body ^. #passphrase . #getApiT
     let sealedTx = body ^. #transaction . #getApiT
@@ -1864,7 +1864,7 @@ signTransaction ctx (ApiT wid) body = do
     -- TODO: The body+witnesses seem redundant with the sealedTx already. What's
     -- the use-case for having them provided separately? In the end, the client
     -- should be able to decouple them if they need to.
-    pure $ Api.ApiSignedTransaction
+    pure $ Api.ApiSerialisedTransaction
         { transaction = ApiT sealedTx'
         }
 
@@ -2212,23 +2212,22 @@ balanceTransaction
     :: forall ctx s k (n :: NetworkDiscriminant).
         ( ctx ~ ApiLayer s k
         , HasNetworkLayer IO ctx
-        , IsOurs s Address
         , GenChange s
         )
     => ctx
     -> ArgGenChange s
     -> ApiT WalletId
     -> ApiBalanceTransactionPostData n
-    -> Handler (ApiConstructTransaction n)
+    -> Handler ApiSerialisedTransaction
 balanceTransaction ctx genChange (ApiT wid) body = do
     pp <- liftIO $ NW.currentProtocolParameters nl
     -- TODO: This throws when still in the Byron era.
     nodePParams <- fromJust <$> liftIO (NW.currentNodeProtocolParameters nl)
 
     let partialTx = body ^. #transaction . #getApiT
-    let (outputs, txWithdrawal, txMetadata, oldFee) = extractFromTx partialTx
+    let (outputs, txWithdrawal, txMetadata) = extractFromTx partialTx
 
-    (newFee, balancedTx) <- withWorkerCtx ctx wid liftE liftE $ \wrk -> do
+    (newFee, extraInputs, extraCollateral, extraOutputs) <- withWorkerCtx ctx wid liftE liftE $ \wrk -> do
         (internalUtxoAvailable, wallet, pendingTxs) <-
             liftHandler $ W.readWalletUTxOIndex @_ @s @k wrk wid
 
@@ -2268,9 +2267,11 @@ balanceTransaction ctx genChange (ApiT wid) body = do
         -- transaction. A long-term fix is to handle this case properly during
         -- balancing.
         let transform s sel =
-                let (sel', s') = W.assignChangeAddresses genChange sel s
+                let (sel', _) = W.assignChangeAddresses genChange sel s
                  in ( const (selectionDelta txOutCoin sel')
-                    , W.selectionToUnsignedTx NoWithdrawal sel' s'
+                    , fst <$> F.toList (sel' ^. #inputs)
+                    , fst <$> (sel' ^. #collateral)
+                    , sel' ^. #change
                     )
         liftHandler $ W.selectAssets @_ @s @k wrk W.SelectAssetsParams
             { outputs
@@ -2282,26 +2283,10 @@ balanceTransaction ctx genChange (ApiT wid) body = do
             }
             transform
 
-    -- FIXME:
-    -- The resulting coin-selection view is currently missing many pieces:
-    --
-    -- - deposits
-    -- - delegation actions
-    -- - withdrawals
-    --
-    -- Fees are also likely wrong. All in all, this 'coin_selection' should not
-    -- exists.
-    let coinSelection = mkApiCoinSelection [] Nothing txMetadata balancedTx
-    let UnsignedTx colls inps _outs change _wdrl = balancedTx
-    let txUpdate = TxUpdate
-            { extraInputs = (\(txin, _, _) -> txin) <$> F.toList inps
-            , extraCollateral = (\(txin, _, _) -> txin) <$> F.toList colls
-            , extraOutputs = toTxOut <$> change
-            , newFee
-            , newExUnits
-            }
+    let txUpdate =
+            TxUpdate { extraInputs, extraCollateral, extraOutputs, newFee, newExUnits }
           where
-            -- TODO: At this stage, we set all execution units for all redeemers to the
+            -- FIXME: At this stage, we set all execution units for all redeemers to the
             -- max cost, which is guaranteed to succeed (given the coin selection above
             -- was done with the same assumption) but also terribly ineffective when it
             -- comes to reducing the cost. This is however sufficient to start
@@ -2310,11 +2295,7 @@ balanceTransaction ctx genChange (ApiT wid) body = do
 
     case ApiT <$> updateTx tl nodePParams partialTx txUpdate of
         Left err -> liftHandler $ throwE $ ErrBalanceTxUpdateError err
-        Right transaction -> pure $ ApiConstructTransaction
-            { coinSelection
-            , transaction
-            , fee = coinToQuantity (newFee oldFee)
-            }
+        Right transaction -> pure $ ApiSerialisedTransaction { transaction }
   where
     nl = ctx ^. networkLayer
     tl = ctx ^. W.transactionLayer @k
@@ -2324,11 +2305,8 @@ balanceTransaction ctx genChange (ApiT wid) body = do
       , TxOut addr (TokenBundle (Coin $ fromIntegral amt) assets)
       )
 
-    toTxOut (TxChange addr amt assets _) =
-        TxOut addr (TokenBundle amt assets)
-
     extractFromTx tx =
-        let (Tx _id fee _coll _inps outs wdrlMap meta _vldt) = decodeTx tl tx
+        let (Tx _id _fee _coll _inps outs wdrlMap meta _vldt) = decodeTx tl tx
             -- TODO: Find a better abstraction that can cover this case.
             wdrl = WithdrawalSelf
                 (error $ "WithdrawalSelf: reward-account should never been use "
@@ -2338,7 +2316,7 @@ balanceTransaction ctx genChange (ApiT wid) body = do
                       <> "when balancing transactions but it was!"
                 )
                 (sumCoins wdrlMap)
-         in (outs, wdrl, meta, fromMaybe (Coin 0) fee)
+         in (outs, wdrl, meta)
 
     -- | Wallet coin selection is unaware of many kinds of transaction content
     -- (e.g. datums, redeemers), which could be included in the input to

lib/core/src/Cardano/Wallet/Api/Types.hs

@@ -95,7 +95,7 @@ module Cardano.Wallet.Api.Types
     , ApiSignTransactionPostData (..)
     , PostTransactionOldData (..)
     , PostTransactionFeeOldData (..)
-    , ApiSignedTransaction (..)
+    , ApiSerialisedTransaction (..)
     , ApiTransaction (..)
     , ApiMintedBurnedTransaction (..)
     , ApiMintedBurnedInfo (..)
@@ -996,7 +996,7 @@ data PostTransactionFeeOldData (n :: NetworkDiscriminant) = PostTransactionFeeOl
 
 type ApiBase64 = ApiBytesT 'Base64 ByteString
 
-newtype ApiSignedTransaction = ApiSignedTransaction
+newtype ApiSerialisedTransaction = ApiSerialisedTransaction
     { transaction :: ApiT SealedTx
     } deriving stock (Eq, Generic, Show)
       deriving anyclass (NFData)
@@ -2698,9 +2698,9 @@ parseSealedTxBytes =
 sealedTxBytesValue :: forall (base :: Base). HasBase base => SealedTx -> Value
 sealedTxBytesValue = toJSON . ApiBytesT @base . view #serialisedTx
 
-instance FromJSON ApiSignedTransaction where
+instance FromJSON ApiSerialisedTransaction where
     parseJSON = genericParseJSON defaultRecordTypeOptions
-instance ToJSON ApiSignedTransaction where
+instance ToJSON ApiSerialisedTransaction where
     toJSON = genericToJSON defaultRecordTypeOptions
 
 instance FromJSON ApiSignTransactionPostData where
@@ -3410,7 +3410,7 @@ instance FromText a => FromHttpApiData (ApiT a) where
 instance ToText a => ToHttpApiData (ApiT a) where
     toUrlPiece = toText . getApiT
 
-instance MimeRender OctetStream ApiSignedTransaction where
+instance MimeRender OctetStream ApiSerialisedTransaction where
    mimeRender ct = mimeRender ct . view #transaction
 
 instance FromHttpApiData ApiTxId where