Skip to content

Commit

Permalink
get rid of 'OnDanglingChange' option for fee balancing
Browse files Browse the repository at this point in the history
  This option allowed choosing between two modes: SaveMoney and
  PayAndBalance. The former was used with cardano-node, and the latter
  used with jormungandr. The reason for having a difference is was
  because of discrepency in the minimum transaction expected by both
  ledger. On jormungandr, transactions have to be _exactly_ balanced and
  leave exactly the expected fees required by the network. On the
  counterpart, the fee calculation was only a function of the number of
  inputs and outputs... therefore much easier to satisfy than on
  cardano-node. Now that we've removed jormungandr, this extra
  indirection / complexity is just harmful. Since the 'PayAndBalance'
  mode is never used, I've removed the option entirely and made code
  assume 'SaveMoney' everywhere it used to choose between both
  alternatives.

  This also seemingly remove the 'allowUnbalancedTx' field from the
  transaction layer which was directly related to this option.
  • Loading branch information
KtorZ committed Nov 25, 2020
1 parent 67a30e6 commit fa816a0
Show file tree
Hide file tree
Showing 7 changed files with 23 additions and 104 deletions.
8 changes: 1 addition & 7 deletions lib/core/src/Cardano/Wallet.hs
Original file line number Diff line number Diff line change
Expand Up @@ -273,12 +273,7 @@ import Cardano.Wallet.Primitive.CoinSelection
import Cardano.Wallet.Primitive.CoinSelection.Migration
( depleteUTxO, idealBatchSize )
import Cardano.Wallet.Primitive.Fee
( ErrAdjustForFee (..)
, Fee (..)
, FeeOptions (..)
, OnDanglingChange (..)
, adjustForFee
)
( ErrAdjustForFee (..), Fee (..), FeeOptions (..), adjustForFee )
import Cardano.Wallet.Primitive.Model
( Wallet
, applyBlocks
Expand Down Expand Up @@ -1260,7 +1255,6 @@ feeOpts
feeOpts tl action md txp minUtxo cs = FeeOptions
{ estimateFee = minimumFee tl feePolicy action md
, dustThreshold = minUtxo
, onDanglingChange = if allowUnbalancedTx tl then SaveMoney else PayAndBalance
-- NOTE
-- Our fee calculation is rather good, but not perfect. We make little
-- approximation errors that may lead to us leaving slightly more fees than
Expand Down
37 changes: 4 additions & 33 deletions lib/core/src/Cardano/Wallet/Primitive/Fee.hs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ module Cardano.Wallet.Primitive.Fee
-- * Fee Adjustment
, FeeOptions (..)
, ErrAdjustForFee(..)
, OnDanglingChange(..)
, adjustForFee
, rebalanceSelection
, coalesceDust
Expand Down Expand Up @@ -106,11 +105,6 @@ data FeeOptions = FeeOptions
-- from the created transaction. Setting 'dustThreshold' to 0
-- removes output equal to 0

, onDanglingChange
:: OnDanglingChange
-- ^ What do to when we encouter a dangling change output.
-- See 'OnDanglingChange'

, feeUpperBound
:: Fee
-- ^ An extra upper-bound computed from the transaction max size. This is
Expand All @@ -124,19 +118,6 @@ data FeeOptions = FeeOptions
-- transaction size based on how many inputs it has.
} deriving (Generic)

-- | We call 'dangling' a change output that would be too expensive to add. This
-- can happen when a change output is small, but adding it generate more fees
-- than not having it.
--
-- In case where nodes accept slightly unbalanced transactions, we may choose to
-- save money and keep the transaction slightly unbalanced. In case where node
-- demands exactly balanced transaction, we have no choice but to add the extra
-- dangling change output and pay for the extra cost induced.
data OnDanglingChange
= PayAndBalance
| SaveMoney
deriving (Generic, Show, Eq)

newtype ErrAdjustForFee
= ErrCannotCoverFee Word64
-- ^ UTxO exhausted during fee covering
Expand Down Expand Up @@ -300,21 +281,11 @@ rebalanceSelection opts s
rebalanceSelection opts (s { change = chgs' })

-- we've left too much, but adding a change output would be more
-- expensive than not having it. Here we have two choices:
--
-- a) If the node allows unbalanced transaction, we can stop here and do
-- nothing. We'll leave slightly more than what's needed for fees, but
-- having an extra change output isn't worth it anyway.
--
-- b) If we __must__ balance the transaction, then we can choose to pay
-- the extra cost by adding the change output and pick a new input to
-- pay for the fee.
-- expensive than not having it. Sicne the node allows unbalanced transaction,
-- we can stop here and do nothing. We'll leave slightly more than what's
-- needed for fees, but having an extra change output isn't worth it anyway.
| φ_dangling >= δ_original && φ_dangling > δ_dangling =
case onDanglingChange opts of
SaveMoney ->
(s, Fee 0)
PayAndBalance ->
(sDangling, Fee (φ_dangling - δ_dangling))
(s, Fee 0)

-- So, we can simply add the change output, and iterate.
| otherwise =
Expand Down
4 changes: 0 additions & 4 deletions lib/core/src/Cardano/Wallet/Transaction.hs
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,6 @@ data TransactionLayer t k = TransactionLayer
-- For example, Byron nodes do not allow null output amounts. Jörmungandr
-- on its side doesn't support more than 255 inputs or outputs.

, allowUnbalancedTx
:: Bool
-- ^ Whether the transaction layer accepts unbalanced transactions.

, decodeSignedTx
:: ByteString -> Either ErrDecodeSignedTx (Tx, SealedTx)
-- ^ Decode an externally-signed transaction to the chain producer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import Cardano.Wallet.Primitive.CoinSelection.Migration
import Cardano.Wallet.Primitive.CoinSelectionSpec
()
import Cardano.Wallet.Primitive.Fee
( Fee (..), FeeOptions (..), OnDanglingChange (..) )
( Fee (..), FeeOptions (..) )
import Cardano.Wallet.Primitive.FeeSpec
()
import Cardano.Wallet.Primitive.Types.Address
Expand Down Expand Up @@ -122,7 +122,6 @@ spec = do
, estimateFee = \s -> Fee
$ fromIntegral
$ 5 * (length (inputs s) + length (outputs s))
, onDanglingChange = PayAndBalance
, feeUpperBound = Fee maxBound
, maximumNumberOfInputs = maxBound
}
Expand Down Expand Up @@ -245,7 +244,6 @@ genFeeOptions (Coin dust) = do
let x = fromIntegral (length (inputs s) + length (outputs s))
in Fee $ (dust `div` 100) * x + dust
, dustThreshold = Coin dust
, onDanglingChange = PayAndBalance
, feeUpperBound = Fee maxBound
, maximumNumberOfInputs = maxBound
}
Expand Down
71 changes: 17 additions & 54 deletions lib/core/test/unit/Cardano/Wallet/Primitive/FeeSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import Cardano.Wallet.Primitive.Fee
( ErrAdjustForFee (..)
, Fee (..)
, FeeOptions (..)
, OnDanglingChange (..)
, adjustForFee
, coalesceDust
, divvyFee
Expand Down Expand Up @@ -514,23 +513,19 @@ propDivvyFeeInvariantEmptyList (fee, outs) =

prop_rebalanceSelection
:: CoinSelection
-> OnDanglingChange
-> Coin
-> Property
prop_rebalanceSelection sel onDangling threshold = do
prop_rebalanceSelection sel threshold = do
let (sel', fee') = first withCoalescedDust $ rebalanceSelection opts sel

let selectionIsBalanced = case onDangling of
PayAndBalance ->
delta sel' == fromIntegral (getFee $ estimateFee opts sel')
SaveMoney ->
delta sel' >= fromIntegral (getFee $ estimateFee opts sel')
let selectionIsBalanced =
delta sel' >= fromIntegral (getFee $ estimateFee opts sel')

let equalityModuloChange =
sel { change = [] } == sel' { change = [] }

let noDust =
all (> threshold') (change sel')
all (> threshold) (change sel')

conjoin
[ fee' == Fee 0 ==> selectionIsBalanced
Expand Down Expand Up @@ -566,45 +561,20 @@ prop_rebalanceSelection sel onDangling threshold = do

withCoalescedDust :: CoinSelection -> CoinSelection
withCoalescedDust cs =
cs { change = coalesceDust threshold' (change cs) }

-- NOTE: We only allow non-null dust threshold if 'onDangling' is
-- set to 'SaveMoney'.
threshold' =
case onDangling of
SaveMoney ->
threshold

PayAndBalance ->
Coin 0
cs { change = coalesceDust threshold (change cs) }

opts = FeeOptions
{ estimateFee = \cs ->
case onDangling of
-- NOTE
-- Dummy fee policy but, following a similar rule as the fee
-- policy on Byron / Shelley (bigger transaction cost more) with
-- sensible values.
SaveMoney ->
let
size = fromIntegral $ length $ show cs
in
Fee (100000 + 100 * size)

-- NOTE
-- Dummy fee policy but, following a similar rule as the fee
-- policy on Jormungandr. More inputs/outputs cost more.
PayAndBalance ->
let
ios = fromIntegral
$ length (inputs cs)
+ length (outputs cs)
+ length (change cs)
in
Fee (100000 + 100 * ios)

, dustThreshold = threshold'
, onDanglingChange = onDangling
-- NOTE
-- Dummy fee policy but, following a similar rule as the fee
-- policy on Byron / Shelley (bigger transaction cost more) with
-- sensible values.
let
size = fromIntegral $ length $ show cs
in
Fee (100000 + 100 * size)

, dustThreshold = threshold
, feeUpperBound = Fee maxBound
, maximumNumberOfInputs = maxBound
}
Expand All @@ -629,8 +599,6 @@ feeOptions fee dust = FeeOptions
\_ -> Fee fee
, dustThreshold =
Coin dust
, onDanglingChange =
PayAndBalance
, feeUpperBound =
Fee maxBound
, maximumNumberOfInputs =
Expand Down Expand Up @@ -846,10 +814,6 @@ instance Arbitrary CoinSelection where
>>= genTxOut
genSelectionFor (NE.fromList outs)

instance Arbitrary OnDanglingChange
where
arbitrary = elements [ PayAndBalance, SaveMoney ]

instance Arbitrary FeeOptions where
arbitrary = do
t <- choose (0, 10) -- dust threshold
Expand All @@ -861,11 +825,10 @@ instance Arbitrary FeeOptions where
$ fromIntegral
$ c + a * (length (inputs s) + length (outputs s))
, dustThreshold = Coin t
, onDanglingChange = PayAndBalance
, feeUpperBound = Fee maxBound
, maximumNumberOfInputs = maxBound
}

instance Show FeeOptions where
show (FeeOptions _ dust onDangling maxFee maxN) =
show (dust, onDangling, maxFee, maxN)
show (FeeOptions _ dust maxFee maxN) =
show (dust, maxFee, maxN)
2 changes: 0 additions & 2 deletions lib/core/test/unit/Cardano/WalletSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -743,8 +743,6 @@ dummyTransactionLayer = TransactionLayer
error "dummyTransactionLayer: validateSelection not implemented"
, decodeSignedTx =
error "dummyTransactionLayer: decodeSignedTx not implemented"
, allowUnbalancedTx =
False
}
where
withEither :: e -> Maybe a -> Either e a
Expand Down
1 change: 0 additions & 1 deletion lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,6 @@ newTransactionLayer networkId = TransactionLayer
, minimumFee = _minimumFee @k networkId
, estimateMaxNumberOfInputs = _estimateMaxNumberOfInputs @k networkId
, validateSelection = const $ return ()
, allowUnbalancedTx = True
}
where
_initDelegationSelection
Expand Down

0 comments on commit fa816a0

Please sign in to comment.