Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not enforce non-empty outputs for unsigned tx #2255

Merged
merged 2 commits into from
Oct 19, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions lib/core/src/Cardano/Wallet.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1634,7 +1634,7 @@ signTx
-> Maybe TxMetadata
-> UnsignedTx (TxIn, TxOut)
-> ExceptT ErrSignPayment IO (Tx, TxMeta, UTCTime, SealedTx)
signTx ctx wid pwd md (UnsignedTx inpsNE outsNE) = db & \DBLayer{..} -> do
signTx ctx wid pwd md (UnsignedTx inpsNE outs) = db & \DBLayer{..} -> do
withRootKey @_ @s ctx wid pwd ErrSignPaymentWithRootKey $ \xprv scheme -> do
let pwdP = preparePassphrase scheme pwd
nodeTip <- withExceptT ErrSignPaymentNetwork $ currentNodeTip nl
Expand All @@ -1658,7 +1658,6 @@ signTx ctx wid pwd md (UnsignedTx inpsNE outsNE) = db & \DBLayer{..} -> do
tl = ctx ^. transactionLayer @t @k
nl = ctx ^. networkLayer @t
inps = NE.toList inpsNE
outs = NE.toList outsNE

-- | Makes a fully-resolved coin selection for the given set of payments.
selectCoinsExternal
Expand Down Expand Up @@ -1687,8 +1686,7 @@ selectCoinsExternal ctx wid argGenChange selectCoins = do
UnsignedTx
<$> (fullyQualifiedInputs s' cs'
(ErrSelectCoinsExternalUnableToAssignInputs cs'))
<*> ensureNonEmpty (outputs cs')
(ErrSelectCoinsExternalUnableToAssignOutputs cs')
<*> pure (outputs cs')
where
db = ctx ^. dbLayer @s @k

Expand All @@ -1697,7 +1695,6 @@ data ErrSelectCoinsExternal e
| ErrSelectCoinsExternalForPayment (ErrSelectForPayment e)
| ErrSelectCoinsExternalForDelegation ErrSelectForDelegation
| ErrSelectCoinsExternalUnableToAssignInputs CoinSelection
| ErrSelectCoinsExternalUnableToAssignOutputs CoinSelection
deriving (Eq, Show)

signDelegation
Expand Down Expand Up @@ -2265,7 +2262,6 @@ data ErrSelectForDelegation
= ErrSelectForDelegationNoSuchWallet ErrNoSuchWallet
| ErrSelectForDelegationFee ErrAdjustForFee
| ErrSelectForDelegationUnableToAssignInputs ErrNoSuchWallet
| ErrSelectForDelegationUnableToAssignOutputs ErrNoSuchWallet
deriving (Show, Eq)

-- | Errors that can occur when signing a delegation certificate.
Expand All @@ -2283,7 +2279,6 @@ data ErrJoinStakePool
| ErrJoinStakePoolSubmitTx ErrSubmitTx
| ErrJoinStakePoolCannotJoin ErrCannotJoin
| ErrJoinStakePoolUnableToAssignInputs CoinSelection
| ErrJoinStakePoolUnableToAssignOutputs CoinSelection
deriving (Generic, Eq, Show)

data ErrQuitStakePool
Expand All @@ -2293,7 +2288,6 @@ data ErrQuitStakePool
| ErrQuitStakePoolSubmitTx ErrSubmitTx
| ErrQuitStakePoolCannotQuit ErrCannotQuit
| ErrQuitStakePoolUnableToAssignInputs CoinSelection
| ErrQuitStakePoolUnableToAssignOutputs CoinSelection
deriving (Generic, Eq, Show)

-- | Errors that can occur when fetching the reward balance of a wallet
Expand Down
21 changes: 5 additions & 16 deletions lib/core/src/Cardano/Wallet/Api/Server.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1659,7 +1659,7 @@ migrateWallet ctx (ApiT wid) migrateData = do
ti
(txId tx)
(fmap Just <$> NE.toList (W.unsignedInputs cs))
(NE.toList (W.unsignedOutputs cs))
(W.unsignedOutputs cs)
(tx ^. #withdrawals)
(meta, time)
Nothing
Expand Down Expand Up @@ -1699,7 +1699,7 @@ assignMigrationAddresses addrs selections =
makeTx :: CoinSelection -> [Address] -> UnsignedTx (TxIn, TxOut)
makeTx sel addrsSelected = UnsignedTx
(NE.fromList (sel ^. #inputs))
(NE.fromList (zipWith TxOut addrsSelected (sel ^. #change)))
(zipWith TxOut addrsSelected (sel ^. #change))

{-------------------------------------------------------------------------------
Network
Expand Down Expand Up @@ -1916,6 +1916,7 @@ mkApiCoinSelection mcerts (UnsignedTx inputs outputs) =
]
where
apiStakePath = ApiT <$> xs

mkAddressAmount :: TxOut -> AddressAmount (ApiT Address, Proxy n)
mkAddressAmount (TxOut addr (Coin c)) =
AddressAmount (ApiT addr, Proxy @n) (Quantity $ fromIntegral c)
Expand Down Expand Up @@ -2273,11 +2274,8 @@ instance Buildable e => LiftHandler (ErrSelectCoinsExternal e) where
ErrSelectCoinsExternalUnableToAssignInputs e ->
apiError err500 UnableToAssignInputOutput $ mconcat
[ "I'm unable to assign inputs from coin selection: "
, pretty e]
ErrSelectCoinsExternalUnableToAssignOutputs e ->
apiError err500 UnableToAssignInputOutput $ mconcat
[ "I'm unable to assign outputs from coin selection: "
, pretty e]
, pretty e
]

instance Buildable e => LiftHandler (ErrCoinSelection e) where
handler = \case
Expand Down Expand Up @@ -2543,7 +2541,6 @@ instance LiftHandler ErrSelectForDelegation where
, "delegation certificate. I need: ", showT cost, " Lovelace."
]
ErrSelectForDelegationUnableToAssignInputs e -> handler e
ErrSelectForDelegationUnableToAssignOutputs e -> handler e

instance LiftHandler ErrSignDelegation where
handler = \case
Expand Down Expand Up @@ -2582,10 +2579,6 @@ instance LiftHandler ErrJoinStakePool where
apiError err500 UnableToAssignInputOutput $ mconcat
[ "I'm unable to assign inputs from coin selection: "
, pretty e]
ErrJoinStakePoolUnableToAssignOutputs e ->
apiError err500 UnableToAssignInputOutput $ mconcat
[ "I'm unable to assign outputs from coin selection: "
, pretty e]

instance LiftHandler ErrFetchRewards where
handler = \case
Expand Down Expand Up @@ -2626,10 +2619,6 @@ instance LiftHandler ErrQuitStakePool where
apiError err500 UnableToAssignInputOutput $ mconcat
[ "I'm unable to assign inputs from coin selection: "
, pretty e]
ErrQuitStakePoolUnableToAssignOutputs e ->
apiError err500 UnableToAssignInputOutput $ mconcat
[ "I'm unable to assign outputs from coin selection: "
, pretty e]

instance LiftHandler ErrCreateRandomAddress where
handler = \case
Expand Down
2 changes: 1 addition & 1 deletion lib/core/src/Cardano/Wallet/Api/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ data ApiCertificate

data ApiCoinSelection (n :: NetworkDiscriminant) = ApiCoinSelection
{ inputs :: !(NonEmpty (ApiCoinSelectionInput n))
, outputs :: !(NonEmpty (AddressAmount (ApiT Address, Proxy n)))
, outputs :: ![AddressAmount (ApiT Address, Proxy n)]
, certificates :: Maybe (NonEmpty ApiCertificate)
} deriving (Eq, Generic, Show)

Expand Down
14 changes: 13 additions & 1 deletion lib/core/src/Cardano/Wallet/Primitive/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -969,8 +969,20 @@ instance ToText TxStatus where
data UnsignedTx input = UnsignedTx
{ unsignedInputs
:: NonEmpty input
-- Inputs are *necessarily* non-empty because Cardano requires at least
-- one UTxO input per transaction to prevent replayable transactions.
-- (each UTxO being unique, including at least one UTxO in the
-- transaction body makes it seemingly unique).

, unsignedOutputs
:: NonEmpty TxOut
:: [TxOut]
Copy link
Contributor

@hasufell hasufell Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add a comment here why we don't want NonEmpty. Will bite someone later, maybe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-- Unlike inputs, it is perfectly reasonable to have empty outputs. The
-- main scenario where this might occur is when constructing a
-- delegation for the sake of submitting a certificate. This type of
-- transaction does not typically include any target output and,
-- depending on which input(s) get selected to fuel the transaction, it
-- may or may not include a change output should its value be less than
-- the minimal UTxO value set by the network.
}
deriving (Eq, Show)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ prop_coinValuesPreserved (CoinSelectionsSetup cs addrs) = do
let selsCoinValue =
sum $ getCoinValueFromInp . inputs . getCS <$> cs
let getCoinValueFromTxOut (UnsignedTx _ txouts) =
sum $ map (\(TxOut _ (Coin c)) -> c) $ NE.toList txouts
sum $ map (\(TxOut _ (Coin c)) -> c) txouts
let txsCoinValue =
sum . map getCoinValueFromTxOut
txsCoinValue (assignMigrationAddresses addrs sels) === selsCoinValue
Expand All @@ -164,7 +164,7 @@ prop_coinValuesPreservedPerTx f (CoinSelectionsSetup cs addrs) = do
f . map (\(_, TxOut _ (Coin c)) -> c)
let selsCoinValue = getCoinValueFromInp . inputs . getCS <$> cs
let getCoinValueFromTxOut (UnsignedTx _ txouts) =
f $ map (\(TxOut _ (Coin c)) -> c) $ NE.toList txouts
f $ map (\(TxOut _ (Coin c)) -> c) txouts
let txsCoinValue = map getCoinValueFromTxOut
txsCoinValue (assignMigrationAddresses addrs sels) === selsCoinValue

Expand Down Expand Up @@ -202,8 +202,7 @@ prop_fairAddressesRecycled
prop_fairAddressesRecycled (CoinSelectionsSetup cs addrs) = do
let sels = getCS <$> cs
let getAllAddrPerTx (UnsignedTx _ txouts) =
map (\(TxOut addr _) -> addr) $
NE.toList txouts
map (\(TxOut addr _) -> addr) txouts
let getAllAddrCounts =
Map.elems .
foldr (\x -> Map.insertWith (+) x (1::Int)) Map.empty .
Expand Down
4 changes: 2 additions & 2 deletions specifications/api/swagger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ x-transactionInputs: &transactionInputs
x-transactionOutputs: &transactionOutputs
description: A list of target outputs
type: array
minItems: 1
minItems: 0
items:
type: object
required:
Expand Down Expand Up @@ -2947,7 +2947,7 @@ paths:
Random-Improve coin selection algorithm</a>.
<b>Note: </b> Not supported for Byron random wallets.
parameters:
- *parametersWalletId
requestBody:
Expand Down