Implementation of fee adjustment #165

paweljakubas · 2019-04-09T10:58:57Z

Issue Number

Overview

I have added fee interface and corresponding types
I have added fee adjustment implementation
I have added unit tests
I have added property tests

Comments

KtorZ · 2019-04-09T13:06:34Z

src/Cardano/Wallet/CoinSelection/Fee.hs

+    -- of outputs (incl. change) was bigger than the total input value which is
+    -- by definition, impossible; unless we messed up real hard.
+    collapse []  _ =
+        invariant "outputs are bigger than inputs" (Coin 0) (const True)


I believe it should be const False here, and let's replace Coin 0 with undefined here ... this can never be reached.

KtorZ · 2019-04-09T13:07:21Z

src/Cardano/Wallet/CoinSelection/Fee.hs

+    -> UTxO
+    -> m (Maybe ((TxIn, TxOut), UTxO))
+pickUtxoRandom _ utxo =
+    runMaybeT $ pickRandom utxo


I think this function / indirection is unnecessary here.

KtorZ · 2019-04-09T13:09:14Z

src/Cardano/Wallet/CoinSelection/Fee.hs

+
+
+coinToDouble :: Coin -> Double
+coinToDouble = fromIntegral . getCoin


We can probably simplify those above and not actually require Double here. We do know the underlying type for coins (Word64), so let's use that and arithmetic on integer (e.g. div or quot instead of /)

KtorZ · 2019-04-09T13:11:10Z

src/Cardano/Wallet/CoinSelection/Fee.hs

+    -- the goal fixed by 'estimatedFee'. So, the actualFee just can't be lower
+    -- than the goal.
+    if (   actualFee < estimatedFee
+        || (coinToDouble actualFee) / 2.0 > (coinToDouble estimatedFee)) then


same remark here about Double ... We know the underlying type, so let's favor this one.

KtorZ · 2019-04-09T13:13:08Z

src/Cardano/Wallet/Fee.hs

+-- Provides the API of Fee calculation.
+
+
+module Cardano.Wallet.Fee


No need for an extra module here, let's have that in Cardano.Wallet.CoinSelection 👍

relocated FeeOptions and FeeErrors to Cardano.Wallet.CoinSelection

KtorZ · 2019-04-09T13:15:26Z

src/Cardano/Wallet/CoinSelection/Fee.hs

+        removeDust = L.filter (> (dustThreshold opt))
+
+        reduceChangeOutputs
+            :: ([Coin] -> [Coin])


No need for the abstraction here, we can use removeDust directly from above.

KtorZ · 2019-04-09T13:16:53Z

src/Cardano/Wallet/CoinSelection/Fee.hs

+                remFee <- lift $ runMaybeT $ coverRemainingFee remainingFee utxo
+                case remFee of
+                    Nothing ->
+                        throwE CannotCoverFee


Maybe we can have the error a bit more explicit here 🤔 ? Saying that we cannot cover for fee because there's not enough inputs? And actually communicate about the missing amount?

added missing amount arg

KtorZ · 2019-04-09T13:17:36Z

src/Cardano/Wallet/CoinSelection/Fee.hs

+    map (\a -> (feeForOut a, a)) outs
+  where
+    totalOut :: Coin
+    totalOut = (Coin . sum . map getCoin) outs


This seems like it could overflow 🤔 ...

outputs can add to more than upperBound of Coin. I used Word64 here, and checking in feeForOut just below overflowing

KtorZ · 2019-04-09T13:19:07Z

src/Cardano/Wallet/Fee.hs

+import GHC.Generics
+    ( Generic )
+
+newtype Fee = Fee { getFee :: Coin } deriving (Show, Eq)


I don't think we need the newtype here. Fee are just Coin

now no more Fee just Coin

paweljakubas · 2019-04-10T13:59:25Z

src/Cardano/Wallet/CoinSelection/Fee.hs

+    totalOuts :: Word64
+    totalOuts = (sum . map getCoin) outs
+
+    feeForOut :: Coin -> Coin


for the simplicity and error plucking I am backed with double

paweljakubas · 2019-04-10T14:01:23Z

test/unit/Cardano/Wallet/CoinSelection/FeeSpec.hs

+            , fExtraUtxo = [1]
+            , fFee = 5
+            , fDust = 0
+            }) (Left $ CannotCoverFee 2)


maybe should be CannotCoverFee 1 as fChngs and fExtraUtxo gives 4 ... now because of MaybeT usage signalling about missing coins before random picking started

will be corrected in coming version

paweljakubas · 2019-04-10T14:04:57Z

test/unit/Cardano/Wallet/CoinSelection/FeeSpec.hs

+            , fExtraUtxo = [2]
+            , fFee = 5
+            , fDust = 0
+            }) (Left $ OutOfBoundFee 0 5)


this one I do not understand. From senderPaysFee I have :

coinSel' = CoinSelection {inputs = [(TxIn {inputId = Hash {getHash = "\170\223\131K\130@\DLE2\202\159"}, inputIx = 0},TxOut {address = Address {getAddress = "ADDR03"}, coin = Coin {getCoin = 10}}),(TxIn {inputId = Hash {getHash = "\219\CANf\153\159\&9\189\154\&9\133"}, inputIx = 0},TxOut {address = Address {getAddress = "ADDR01"}, coin = Coin {getCoin = 2}})], outputs = [TxOut {address = Address {getAddress = "ADDR01"}, coin = Coin {getCoin = 7}}], change = [Coin {getCoin = 5}]}

and after computeFee coinSel' I have Coin 0 ... The computation is :

collapse : p>m : Coin {getCoin = 10} > Coin {getCoin = 7} ps:[Coin {getCoin = 2}] ms[Coin {getCoin = 5}] collapse : p<m : Coin {getCoin = 3} < Coin {getCoin = 5} ps:[Coin {getCoin = 2}] ms[] collapse : p==m : Coin {getCoin = 2} ps:[] ms[] collapse : plus : []

I need to think about it - though it should be Right

addressed in new commit

paweljakubas · 2019-04-10T14:06:39Z

added logs which I use in the comment to illustrate the point I do not understand yet. Will remove them before the last commit

KtorZ · 2019-04-09T17:18:20Z

cardano-wallet.cabal

@@ -69,6 +69,7 @@ library
      Cardano.Wallet.Binary
      Cardano.Wallet.Binary.Packfile
      Cardano.Wallet.CoinSelection
+      Cardano.Wallet.CoinSelection.Fee


Let's completely remove this module and have it merged with Cardano.Wallet.CoinSelection

KtorZ · 2019-04-09T17:19:50Z

src/Cardano/Wallet/CoinSelection/Random.hs

@@ -11,6 +11,8 @@

 module Cardano.Wallet.CoinSelection.Random
    ( random
+    , pickRandom
+    , distance


Maybe distance is something we want to have in Primitive.Types 🤔 ?

KtorZ · 2019-04-09T17:23:09Z

src/Cardano/Wallet/CoinSelection/Fee.hs

+computeFee
+    :: CoinSelection
+    -> Coin
+computeFee (CoinSelection inps outs chgs) =


Note that this whole algorithm here exists only because in the original implementation, we were in the generic realm, where there was no concrete instance for Coin. Here, we know that Coin = Coin Word64 with maxBound :: Coin being much much less than maxBound :: Word64; We could in practice makes the computation Word64 (or Natural) if we really really want the safety here, and at the end, convert it back to a Coin.

Also, we can pretty much safely assume (i.e. enforce with an invariant) that the resulting fee Coin is less than the max bound for Coin (otherwise that's a very very expensive transaction we have there!)

thanks for this comment

paweljakubas · 2019-04-11T11:53:30Z

added almost 25 unit tests and made sure they pass. Reshuffled the code as suggested by reviewer. Rebased. Next commit : properties tests

Remove pickUtxo argument and clean the code

Finetuning tests and adding rest properties Add estimate function pointers Improving on properties

KtorZ · 2019-04-16T01:40:30Z

There is a failing properties:

No fee gives back the same selection FAILED [1]

which doesn't fail all the time but only when small enough coins are generated. Seems like some dust remains in the final selection 🤔 ... I haven't much looked into that but, changing the coin arbitrary generator to choose (1, 10) would make the property fail at each run.

NOTE: I've removed / merged together some of the other properties to keep it simple and, only about properties that are rather useful to verify.

cc @paweljakubas

paweljakubas · 2019-04-16T14:20:22Z

as it is now I have not encountered failure when running:

stack test --ta '-m "Fee calculation"' cardano-wallet:unit

Moreover, I see in propSameSelection responsible for the property No fee ..:

let feeOpt = feeOptions 0 0

which means dust threshold is 0

Nevertheless, I confirm that when coin is between (1,10) the test fails

I see that the below is expected :

change = [Coin {getCoin = 0},Coin {getCoin = 0},Coin {getCoin = 3},Coin {getCoin = 5},Coin {getCoin = 7},Coin {getCoin = 7},Coin {getCoin = 7},Coin {getCoin = 8}]})

but after adjustForFee we obtained :

change = [Coin {getCoin = 3},Coin {getCoin = 5},Coin {getCoin = 7},Coin {getCoin = 7},Coin {getCoin = 7},Coin {getCoin = 8}]})

meaning because of small coins Coin 0 changes were produced. We should not do it because we wipe out Coin 0 after fee adjustment even when dust threshold is 0

Note. Of course, when Coin is (1,10) we cannot cover fees that are (100000, 500000) so third property will find satisfying case. As expected :

  test/unit/Cardano/Wallet/CoinSelectionSpec.hs:274:9: 
  1) Cardano.Wallet.CoinSelection, Fee calculation properties, Adjusting for fee (/= 0) reduces the change outputs or increase inputs
       Gave up after 0 tests; 1000 discarded!

KtorZ · 2019-04-16T16:38:56Z

I am not sure about this last commit @paweljakubas: why do we even have 0 coins as part of the selection 🤔 ?

paweljakubas · 2019-04-16T17:00:33Z

well I believe, largestFirst can create Coin 0 in change https://github.com/input-output-hk/cardano-wallet/blob/master/src/Cardano/Wallet/CoinSelection/LargestFirst.hs#L95 in contrast to random : https://github.com/input-output-hk/cardano-wallet/blob/master/src/Cardano/Wallet/CoinSelection/Random.hs#L219
and for small coins (1,10) we capture this case. So, because we create coin selection for adjustForFee using largestFirst it is possible that Coin 0 is in changes. ..

KtorZ · 2019-04-16T17:11:20Z

Indeed. I've moved the fix into the largest first implementation instead as it makes more sense to have it here and return valid coin selections in a first place!

paweljakubas self-assigned this Apr 9, 2019

paweljakubas requested a review from KtorZ April 9, 2019 10:58

paweljakubas force-pushed the paweljakubas/92/adding-fee-calculation branch from 010e51c to 4d4d18d Compare April 9, 2019 12:40

paweljakubas changed the title ~~First impl of Fee adjustment~~ Implementation of fee adjustment Apr 9, 2019

KtorZ mentioned this pull request Apr 9, 2019

Port coin selection and fee calculation from legacy #92

Closed

7 tasks

KtorZ reviewed Apr 9, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/92/adding-fee-calculation branch 2 times, most recently from ecd5b66 to fea97a0 Compare April 10, 2019 13:57

paweljakubas commented Apr 10, 2019

View reviewed changes

KtorZ reviewed Apr 11, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/92/adding-fee-calculation branch from fea97a0 to cde75b8 Compare April 11, 2019 11:51

paweljakubas force-pushed the paweljakubas/92/adding-fee-calculation branch 3 times, most recently from 6b85a3b to f06d2fa Compare April 11, 2019 16:23

KtorZ force-pushed the paweljakubas/92/adding-fee-calculation branch from 1acdbdb to c87b71d Compare April 14, 2019 19:23

paweljakubas force-pushed the paweljakubas/92/adding-fee-calculation branch from c87b71d to 40a761b Compare April 15, 2019 08:02

paweljakubas and others added 6 commits April 16, 2019 03:30

Implement first draft for fee adjustments

017fe75

Remove pickUtxo argument and clean the code

Adding tons of unit tests and revert to double conversion

e0d76c0

Adding property tests

8a862f5

Finetuning tests and adding rest properties Add estimate function pointers Improving on properties

style improvements & readability refactorings

4b86857

fix missing recursion after new inputs got selected to cover fee

141ccf1

review properties and unit tests for fee adjustment

356d7f8

KtorZ force-pushed the paweljakubas/92/adding-fee-calculation branch from 40a761b to 356d7f8 Compare April 16, 2019 01:37

add shrinkers & show error cases through Buildable instances

8d4c1b3

fix LargestFirst to not return zero coin values as change

215f429

KtorZ force-pushed the paweljakubas/92/adding-fee-calculation branch from 4b1d41f to 215f429 Compare April 16, 2019 17:13

KtorZ approved these changes Apr 17, 2019

View reviewed changes

KtorZ merged commit fde6196 into master Apr 17, 2019

KtorZ deleted the paweljakubas/92/adding-fee-calculation branch April 17, 2019 00:41



		coinToDouble :: Coin -> Double
		coinToDouble = fromIntegral . getCoin

		-- Provides the API of Fee calculation.


		module Cardano.Wallet.Fee

Implementation of fee adjustment #165

Implementation of fee adjustment #165

Conversation

paweljakubas commented Apr 9, 2019 • edited Loading

Issue Number

Overview

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Apr 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Apr 11, 2019

KtorZ commented Apr 16, 2019

paweljakubas commented Apr 16, 2019 • edited Loading

KtorZ commented Apr 16, 2019

paweljakubas commented Apr 16, 2019 • edited Loading

KtorZ commented Apr 16, 2019

paweljakubas commented Apr 9, 2019 •

edited

Loading

paweljakubas commented Apr 16, 2019 •

edited

Loading

paweljakubas commented Apr 16, 2019 •

edited

Loading