Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CBR-462] Fix 0 fee transaction #3704

Merged
merged 2 commits into from
Oct 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
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
22 changes: 18 additions & 4 deletions wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic.hs
Original file line number Diff line number Diff line change
Expand Up @@ -590,16 +590,30 @@ nLargestFromListBy f n = \xs ->
dropOne (Just [_]) = Nothing
dropOne (Just (_:as)) = Just as

-- | Proportionally divide the fee over each output
-- | Proportionally divide the fee over each output.
--
-- There's a special 'edge-case' when the given input is a singleton list
-- with one 0 coin. This is artifically created during input selection when
-- the transaction's amount matches exactly the source's balance.
-- In such case, we can't really compute any ratio for fees and simply return
-- the whole fee back with the given change value.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a final section to this comment explaining why we do that: we return the whole fee because at the end of the day "it doesn't matter": as we are doomed anyway (we have depleted our Utxo to cover the amount, no chances we have money to cover the fee, too) this will be caught "one layer up" and the whole coin selection algorithm will fail accordingly, as it should.

divvyFee :: forall dom a. CoinSelDom dom
=> (a -> Value dom) -> Fee dom -> [a] -> [(Fee dom, a)]
divvyFee _ _ [] = error "divvyFee: empty list"
divvyFee f fee as = map (\a -> (feeForOut a, a)) as
divvyFee _ _ [] = error "divvyFee: empty list"
divvyFee f fee [a] | f a == valueZero = [(fee, a)]
divvyFee f fee as = map (\a -> (feeForOut a, a)) as
where
-- All outputs are selected from well-formed UTxO, so their sum cannot
-- overflow
totalOut :: Value dom
totalOut = unsafeValueSum (map f as)
totalOut =
let
total = unsafeValueSum (map f as)
in
if total == valueZero then
error "divyyFee: invalid set of coins, total is 0"
else
total
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shouldn't happen, but ... just-in-case :|


-- The ratio will be between 0 and 1 so cannot overflow
feeForOut :: a -> Fee dom
Expand Down
18 changes: 13 additions & 5 deletions wallet-new/src/Cardano/Wallet/Kernel/CoinSelection/Generic/Fees.hs
Original file line number Diff line number Diff line change
Expand Up @@ -90,18 +90,26 @@ senderPaysFee pickUtxo totalFee css = do
let (css', remainingFee) = feeFromChange totalFee css
(css', ) <$> coverRemainingFee pickUtxo remainingFee

coverRemainingFee :: forall utxo e m. (Monad m, CoinSelDom (Dom utxo))
=> (Value (Dom utxo) -> CoinSelT utxo e m (UtxoEntry (Dom utxo)))
coverRemainingFee :: forall utxo m. (Monad m, CoinSelDom (Dom utxo))
=> (Value (Dom utxo) -> CoinSelT utxo CoinSelHardErr m (UtxoEntry (Dom utxo)))
-> Fee (Dom utxo)
-> CoinSelT utxo e m (SelectedUtxo (Dom utxo))
-> CoinSelT utxo CoinSelHardErr m (SelectedUtxo (Dom utxo))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only thing I don't like about this PR: Edsko worked really hard to make the code completely generic, even in the error e, but here we are now specialising it to CoinSelHardErr. Is there any way we can keep it generic? It would be better, as I also don't have any clue if this would preclude along the lines the possibility to run more interesting simulations, for example.

Copy link
Contributor Author

@KtorZ KtorZ Oct 4, 2018

Choose a reason for hiding this comment

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

Not sure here. I just followed GHC complaints when started to "catchError" and "throwError". We can't keep the generic forall e here, because well, we use explicit CoinSelHardErr constructors in the function body :/

Exactly like functions around actually:

receiverPaysFee :: forall dom. CoinSelDom dom
=> Fee dom
-> [CoinSelResult dom]
-> Except CoinSelHardErr [CoinSelResult dom]
receiverPaysFee totalFee =

adjustForFees :: forall utxo m. (CoinSelDom (Dom utxo), Monad m)
=> FeeOptions (Dom utxo)
-> (Value (Dom utxo) ->
CoinSelT utxo CoinSelHardErr m (UtxoEntry (Dom utxo)))
-> [CoinSelResult (Dom utxo)]
-> CoinSelT utxo CoinSelHardErr m
([CoinSelResult (Dom utxo)], SelectedUtxo (Dom utxo))
adjustForFees feeOptions pickUtxo css = do

I believe this part can't really be 100% generic, unless we provide also "Generic errors"

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, good point. coverRemainingFee is used by senderPaysFee which is already specialised, so I guess this is fine. Alright πŸ˜‰

coverRemainingFee pickUtxo fee = go emptySelection
where
-- | In this context, @CoinSelHardErrUtxoDepleted@ might be thrown by
-- `pickUtxo` as we iterate which here means that we are running out of
-- UTxOs to cover the fee, and therefore, remap the error accordingly.
remapUtxoDepleted :: CoinSelHardErr -> CoinSelHardErr
remapUtxoDepleted CoinSelHardErrUtxoDepleted = CoinSelHardErrCannotCoverFee
remapUtxoDepleted err = err

go :: SelectedUtxo (Dom utxo)
-> CoinSelT utxo e m (SelectedUtxo (Dom utxo))
-> CoinSelT utxo CoinSelHardErr m (SelectedUtxo (Dom utxo))
go !acc
| selectedBalance acc >= getFee fee = return acc
| otherwise = do
io <- pickUtxo $ unsafeValueSub (getFee fee) (selectedBalance acc)
io <- (pickUtxo $ unsafeValueSub (getFee fee) (selectedBalance acc))
`catchError` (throwError . remapUtxoDepleted)
go (select io acc)

-- | Attempt to pay the fee from change outputs, returning any fee remaining
Expand Down