Adjust fee estimation to account for minting and burning. #2741

sevanspowell · 2021-06-25T08:44:17Z

Issue Number

Overview

This PR:

adjusts fee estimation to account for minted assets and scripts.
adds property and unit tests to cover changes to estimateTxSize.
makes small adjustments to integration test expectations for transaction fees.

sevanspowell · 2021-07-12T07:48:41Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

@@ -183,7 +197,7 @@ spec = do
        prop "roundtrip for Byron witnesses" prop_decodeSignedByronTxRoundtrip

    estimateMaxInputsTests @ShelleyKey
-        [(1,115),(5,106),(10,101),(20,85),(50,32)]
+        [(1,114),(5,106),(10,101),(20,85),(50,32)]


I've changed the expected number of inputs, because my change to the fee estimation caused this test to fail, but I'll admit I don't quite understand the test.

A small difference such as this in the expected result doesn't seem to be a cause for concern, but if anyone has more knowledge on this I'd love to hear it.

I guess that the max inputs should decrease if you have caused all fee estimations to increase.
I wonder why only the first test case needed adjustment though.

rvl

Nice work - tests look good.
I have some change requests, so please merge once they're done.

P.S. Squash commits together where it make sense

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

rvl · 2021-07-13T06:07:28Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+        sizeOf_ValidityIntervalStart
+          = sizeOf_UInt


About this one - is it for the validity interval of the transaction as a whole or, is it an interval which is part of a script?
Are the validity interval bounds optional? What about the size of the validity interval end?

This is the validity interval of the transaction as a whole, separate validity intervals exist for the scripts (see here).

Shelley and later eras support an upper bound on the validity range, this is known as the TTL, so it is already part of the fee estimation (https://github.com/input-output-hk/cardano-wallet/blob/5517e3f699aaeeb78fe3abd5bf7cc41042dc83fb/lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs#L866). Allegra and later eras support a lower bound on the validity range, and that's what I'm adding here.

So both ends of the validity bound are present, just one end is aliased as "TTL".

rvl · 2021-07-13T06:11:43Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+        -- multiasset<a> = { * policy_id => { * asset_name => a } }
+        -- policy_id = scripthash
+        -- asset_name = bytes .size (0..32)
+        sizeOf_Mint AssetId{tokenName}


Could sizeOf_Mint and sizeOf_NativeAsset be refactored and parameterised by the inner type?
Like how it's done in the shelley-ma.cddl file with multiasset<a>.

Addressed in 5dbf25e.

https://github.com/input-output-hk/cardano-wallet/pull/2741/files#diff-6218e625cd8c0f10e842ef1c10eb04bebf307ccecad42038e3b12b287a763690R948

rvl · 2021-07-13T06:13:39Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+    --   , ? 8 : uint ; validity interval start
+    --   , ? 9 : mint


These are things added in shelley-ma.
There is a link to the source shelley.cddl file above.
Can we also add a link to shelley-ma.cddl?

Thanks :) Addressed in b8cbd8a

https://github.com/input-output-hk/cardano-wallet/pull/2741/files#diff-6218e625cd8c0f10e842ef1c10eb04bebf307ccecad42038e3b12b287a763690R787

rvl · 2021-07-13T06:14:50Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

@@ -1107,6 +1178,11 @@ estimateTxSize skeleton =
    sizeOf_SmallArray = 1
    sizeOf_Array = 3

+-- Small helper function for summing values. Given a list of values, get the sum
+-- of the values, after the given function has been applied to each value.
+sumVia :: (Foldable t, Num m) => (a -> m) -> t a -> m


rvl · 2021-07-13T07:03:11Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+    --      ; Timelock validity intervals are half-open intervals [a, b).
+    --      ; This field specifies the right (excluded) endpoint b.
+    --   ]
+    sizeOf_NativeScript = \case


Good to add this into the size estimation, because I think it would also be needed for spending from a multi-sig wallet.

rvl · 2021-07-13T07:06:42Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+    -- A CBOR Int which is less than 23 in value fits on a single byte. Beyond,
+    -- the first byte is used to encode the number of bytes necessary to encode
+    -- the number, followed by the number itself.
+    sizeOf_LargeInt = 9


Would it be better to call this sizeOf_Int64? Because that's the max size for minting.

Addressed in 51d5f93.

https://github.com/input-output-hk/cardano-wallet/pull/2741/files#diff-6218e625cd8c0f10e842ef1c10eb04bebf307ccecad42038e3b12b287a763690R1184

rvl · 2021-07-13T07:09:18Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

@@ -183,7 +197,7 @@ spec = do
        prop "roundtrip for Byron witnesses" prop_decodeSignedByronTxRoundtrip

    estimateMaxInputsTests @ShelleyKey
-        [(1,115),(5,106),(10,101),(20,85),(50,32)]
+        [(1,114),(5,106),(10,101),(20,85),(50,32)]


I guess that the max inputs should decrease if you have caused all fee estimations to increase.
I wonder why only the first test case needed adjustment though.

jonathanknowles

Hi @sevanspowell

Thanks for making this PR!

My main question relates to burning: is support for burned assets included in this PR?

I've done an initial review. I still have a bit of work to do to understand the test suite additions. I'll have another look tomorrow morning!

jonathanknowles · 2021-07-13T07:43:57Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

@@ -714,6 +719,8 @@ data TxSkeleton = TxSkeleton
    , txInputCount :: !Int
    , txOutputs :: ![TxOut]
    , txChange :: ![Set AssetId]
+    , txScripts :: [Script KeyHash]
+    , txMintAssets :: [AssetId]


Hi @sevanspowell.

Does this field refer only to minted assets, or can it also contain burned assets?

If it only refers to minted assets, then will you need to add another field for burned assets?

If it also contains burned assets, then what happens if there are assets that are both minted and burned? (This is allowed by coin selection, but I'm not sure if it's planned to allow this in the API, so asking just in case!)

There is some somewhat confusing terminology here. cardano-node just refers to the minting and burning field as "mint". So this refers to both minted and burned assets.

When constructing a transaction manually, mints look something like +3 deadbeef.aaaaaa, whereas burns look something like -3 deadbeef.aaaaaa. If an asset is both minted and burned, it will just be added twice to the txMintAssets list. This is what we want because the Tx construction will do that same, add some data for the mint, add some data for the burn. From the perspective of tx size estimation, negative (burned) and positive (minted) quantities are no different, they are both represented with an int64. We don't have to do any fancy logic here, mints and burns are both treated as (qty, policyId, assetName), as they will be in the final Tx.

I've improved the terminology and added a comment to explain the usage of the field: c0de116.

https://github.com/input-output-hk/cardano-wallet/pull/2741/files#diff-6218e625cd8c0f10e842ef1c10eb04bebf307ccecad42038e3b12b287a763690R721

jonathanknowles · 2021-07-13T07:44:21Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+    numberOf_ScriptVkeyWitnesses
+      = sumVia scriptRequiredKeySigs txScripts


Suggested change

numberOf_ScriptVkeyWitnesses

= sumVia scriptRequiredKeySigs txScripts

numberOf_ScriptVkeyWitnesses

= sumVia scriptRequiredKeySigs txScripts

Addressed in 73caf08.

https://github.com/input-output-hk/cardano-wallet/pull/2741/files#diff-6218e625cd8c0f10e842ef1c10eb04bebf307ccecad42038e3b12b287a763690R816

jonathanknowles · 2021-07-13T07:49:00Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+    scriptRequiredKeySigs :: Num num => Script KeyHash -> num
+    scriptRequiredKeySigs (RequireSignatureOf _) = 1
+    scriptRequiredKeySigs (RequireAllOf ss)      = sumVia scriptRequiredKeySigs ss
+    scriptRequiredKeySigs (RequireAnyOf ss)      = sumVia scriptRequiredKeySigs ss
+    scriptRequiredKeySigs (ActiveFromSlot _)     = 0
+    scriptRequiredKeySigs (ActiveUntilSlot _)    = 0
+    -- We don't know how many the user will sign with, so we just assume the worst case of "signs with all".
+    scriptRequiredKeySigs (RequireSomeOf _ ss)   = sumVia scriptRequiredKeySigs ss


Request: follow the coding standards here (line length + avoid vertical alignment).

Suggestion: use LambdaCase to resolve this:

scriptRequiredKeySigs :: Num num => Script KeyHash -> num scriptRequiredKeySigs = \case RequireSignatureOf _ -> 1 RequireAllOf ss -> sumVia scriptRequiredKeySigs ss RequireAnyOf ss -> sumVia scriptRequiredKeySigs ss ActiveFromSlot _ -> 0 ActiveUntilSlot _ -> 0 RequireSomeOf _ ss -> -- We don't know how many the user will sign with, so we just assume -- the worst case of "signs with all". sumVia scriptRequiredKeySigs ss

Addressed in 73caf08.

This suggestion makes a small function consume a quite unnecessary amount of vertical space. 😛

You could also keep it under 80 characters, and vertically compact too, just by renaming the ss variable to s, and/or by removing the column alignment on =.

Easier to scroll down than sideways? :P

https://github.com/input-output-hk/cardano-wallet/pull/2741/files#diff-6218e625cd8c0f10e842ef1c10eb04bebf307ccecad42038e3b12b287a763690R818

jonathanknowles · 2021-07-13T07:50:51Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+        sizeOf_ValidityIntervalStart
+          = sizeOf_UInt


Suggested change

sizeOf_ValidityIntervalStart

= sizeOf_UInt

sizeOf_ValidityIntervalStart

= sizeOf_UInt

Addressed in 73caf08.

https://github.com/input-output-hk/cardano-wallet/pull/2741/files#diff-6218e625cd8c0f10e842ef1c10eb04bebf307ccecad42038e3b12b287a763690R944

jonathanknowles · 2021-07-13T07:52:49Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+        -- multiasset<a> = { * policy_id => { * asset_name => a } }
+        -- policy_id = scripthash
+        -- asset_name = bytes .size (0..32)
+        sizeOf_Mint AssetId{tokenName}


@sevanspowell is there a sizeOf_Burn function to accompany sizeOf_Mint?

Or is it the intention to put everything into the same field?

See #2741 (comment)

jonathanknowles · 2021-07-13T08:20:17Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

+                & counterexample ("cost with: " <> show costWith)
+                & counterexample ("cost without: " <> show costWithout)
+
+        it "increasing mint increases tx size at least proportionate to asset names"


Suggested change

it "increasing mint increases tx size at least proportionate to asset names"

it "increasing mint increases tx size at least proportionally to asset names"

Addressed in 73caf08.

jonathanknowles · 2021-07-13T08:24:51Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

+                (if null assets
+                 then property $ marginalCost == 0
+                 else property $ marginalCost > 0
+                ) & classify (null assets) "null minting assets"
+                & counterexample ("marginal cost: " <> show marginalCost)


Suggested change

(if null assets

then property $ marginalCost == 0

else property $ marginalCost > 0

) & classify (null assets) "null minting assets"

& counterexample ("marginal cost: " <> show marginalCost)

(if null assets

then property $ marginalCost == 0

else property $ marginalCost > 0

)

& classify (null assets) "null minting assets"

& counterexample ("marginal cost: " <> show marginalCost)

Addressed in 73caf08.

jonathanknowles · 2021-07-13T08:25:35Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

+                (if null scripts
+                 then property $ marginalCost == 0
+                 else property $ marginalCost > 0
+                ) & classify (null scripts) "null scripts"
+                & counterexample ("marginal cost: " <> show marginalCost)


Suggested change

(if null scripts

then property $ marginalCost == 0

else property $ marginalCost > 0

) & classify (null scripts) "null scripts"

& counterexample ("marginal cost: " <> show marginalCost)

(if null scripts

then property $ marginalCost == 0

else property $ marginalCost > 0

)

& classify (null scripts) "null scripts"

& counterexample ("marginal cost: " <> show marginalCost)

Addressed in 73caf08.

jonathanknowles · 2021-07-13T08:26:12Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

+                & counterexample ("asset names length: "
+                                  <> show lengthAssetNames)


Suggested change

& counterexample ("asset names length: "

<> show lengthAssetNames)

& counterexample

("asset names length: " <> show lengthAssetNames)

Addressed in 73caf08.

jonathanknowles · 2021-07-13T08:26:58Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

+                & counterexample ("witness size: "
+                                  <> show (numWitnesses * sizeWitness))


Suggested change

& counterexample ("witness size: "

<> show (numWitnesses * sizeWitness))

& counterexample

("witness size: " <> show (numWitnesses * sizeWitness))

Addressed in 73caf08.

rvl

Thanks @sevanspowell

sevanspowell · 2021-07-14T06:45:05Z

bors r+

iohk-bors · 2021-07-14T06:45:08Z

👎 Rejected by code reviews

#expected (not flaky)

jonathanknowles · 2021-07-14T07:06:04Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+    -- ^ Assets that have been both minted and burned, or minted or burned
+    -- multiple times, will appear multiple times in this list, once for each
+    -- mint or burn. For example if the caller "mints 3" of asset id "A", and
+    -- "burns 3" of asset id "A", "A" will appear twice in the list.


Thanks, this is a really nice and clear explanation. 👍🏻

jonathanknowles · 2021-07-14T07:08:07Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+        RequireSignatureOf _ -> sizeOf_SmallUInt + sizeOf_Hash28
+        RequireAllOf ss      -> sizeOf_SmallUInt + sizeOf_Array + sumVia sizeOf_NativeScript ss
+        RequireAnyOf ss      -> sizeOf_SmallUInt + sizeOf_Array + sumVia sizeOf_NativeScript ss
+        RequireSomeOf _ ss   -> sizeOf_SmallUInt + sizeOf_UInt + sizeOf_Array + sumVia sizeOf_NativeScript ss
+        ActiveFromSlot _     -> sizeOf_SmallUInt + sizeOf_UInt
+        ActiveUntilSlot _    -> sizeOf_SmallUInt + sizeOf_UInt


Addressed in 73caf08.

Hi Sam, I think this commit may have been dropped, perhaps accidentally?

jonathanknowles · 2021-07-14T08:45:56Z

bors r+

2741: Adjust fee estimation to account for minting and burning. r=jonathanknowles a=sevanspowell # Issue Number ADP-998 ADP-955 # Overview I've adjusted fee estimation to account for minted assets and scripts. And also added some tests to cover it. This PR is still a draft as I am not confident that the tests sufficiently assert the expected behaviour of fee estimation. I'm testing the estimation with more estimation, although I'm not sure how we typically test heuristics like this. Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-07-14T09:28:30Z

Build failed:

buildkite/cardano-wallet

Failure log:

               , totalFee = Quantity
                   { getQuantity = 3121400 }
               , balanceLeftover = ApiWalletMigrationBalance
                   { ada = Quantity
                       { getQuantity = 0 }
                   , assets = ApiT
                       { getApiT = TokenMap
                           ( fromList [] )
                       }
                   }
               , balanceSelected = ApiWalletMigrationBalance
                   { ada = Quantity
                       { getQuantity = 10000100000000 }
                   , assets = ApiT
                       { getApiT = TokenMap
                           ( fromList [] )
                       }
                   }
               }
           )
       )
     expected: 3120400
      but got: 3121400

To rerun use: --match "/API Specifications/SHELLEY_MIGRATIONS/SHELLEY_MIGRATE_02 - Can migrate a large wallet requiring more than one transaction./"

It looks like the fee estimation has gone up for ordinary transactions, even those that don't require minting and burning.

#expected

jonathanknowles · 2021-07-14T13:12:49Z

bors r+

2741: Adjust fee estimation to account for minting and burning. r=jonathanknowles a=sevanspowell # Issue Number ADP-998 ADP-955 # Overview I've adjusted fee estimation to account for minted assets and scripts. And also added some tests to cover it. This PR is still a draft as I am not confident that the tests sufficiently assert the expected behaviour of fee estimation. I'm testing the estimation with more estimation, although I'm not sure how we typically test heuristics like this. Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-07-14T13:35:55Z

Build failed:

ci/hydra-build:required

Similar failures to previous bors run. For example, w.r.t. expected fees:

       expected: 1129000
        but got: 1129500

  To rerun use: --match "/API Specifications/NEW_SHELLEY_TRANSACTIONS/TRANS_NEW_CREATE_02 - Only metadata/"

Again, it does look like the fee estimations for ordinary transactions have increased, even in contexts where there are no minted or burned tokens.

#expected

jonathanknowles

Hi @sevanspowell

I think we might need to make further changes to this PR.

According to the integration test failure log (example), it seems that the fee estimates for ordinary transactions have increased, even for those that don't involve minting or burning.

I'm not sure if this is intentional or not.

Perhaps we need to look at the estimateTxSize function again, to see why estimates are increasing even when there are no minted or burned tokens.

OTOH, if this is intentional, then we'll probably need to adjust the expectations of the failing integration tests. This isn't necessarily a bad thing, as it gives us an opportunity to check (with a human pair of eyes) that the fees still remain reasonable.

Let me know if I can be of any assistance!

sevanspowell · 2021-07-15T07:31:24Z

Hi @sevanspowell

I think we might need to make further changes to this PR.

According to the integration test failure log (example), it seems that the fee estimates for ordinary transactions have increased, even for those that don't involve minting or burning.

I'm not sure if this is intentional or not.

Perhaps we need to look at the estimateTxSize function again, to see why estimates are increasing even when there are no minted or burned tokens.

OTOH, if this is intentional, then we'll probably need to adjust the expectations of the failing integration tests. This isn't necessarily a bad thing, as it gives us an opportunity to check (with a human pair of eyes) that the fees still remain reasonable.

Let me know if I can be of any assistance!

Yes, the base fee estimation will increase somewhat, due to the presence of the new "validity interval start" field. The size of the field is 5 bytes. I'm verifying now that this is the only cause for the increase in fees.

EDIT: I've verified that the cardano-wallet integration test passes when the validity interval is commented out:

    sizeOf_TransactionBody
        = sizeOf_SmallMap
        + sizeOf_Inputs
        + sizeOf_Outputs
        + sizeOf_Fee
        + sizeOf_Ttl
        + sizeOf_Certificates
        + sizeOf_Withdrawals
        + sizeOf_Update
        + sizeOf_MetadataHash
        -- + sizeOf_ValidityIntervalStart
        + sumVia sizeOf_Mint txMintBurnAssets

I think this verifies that the cause of the increase in fees is solely due to the presence of the new validity interval start, for which we'd expect an increase in fees. I'll modify the test expectations accordingly.

Anviking · 2021-07-15T12:14:58Z

I added #expected annotations to the last bors failures as it sounded like they were caused by the PR itself. Feel free to change if incorrect.

- Update fee estimation function to provide fee estimates for minted/burned assets and for scripts. - Move Cardano.Wallet.Gen to cardano-wallet-core (with the other test modules) so it can be re-used across cardano-wallet-core and cardano-wallet tests. - Both "NativeAsset" and "Mint" are "multiasset"s from the perspective of the CDDL, refactor their size definitions to use a new "multiasset" size function. - The base estimate for the size of transactions has increased due to the presence of the "Tx is valid from this point forward" field (validity interval start). - Adjust the test fee expectations to match this.

sevanspowell · 2021-07-16T01:32:23Z

bors r+

iohk-bors · 2021-07-16T01:32:26Z

👎 Rejected by code reviews

#expected

jonathanknowles

LGTM!

sevanspowell · 2021-07-16T03:17:58Z

bors r+

2741: Adjust fee estimation to account for minting and burning. r=sevanspowell a=sevanspowell # Issue Number ADP-998 ADP-955 # Overview This PR: - [x] adjusts fee estimation to account for minted assets and scripts. - [x] adds property and unit tests to cover changes to `estimateTxSize`. - [x] makes small adjustments to integration test expectations for transaction fees. Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-07-16T03:46:30Z

Build failed:

ci/hydra-build:required

Looks like there are still a couple of failing integration tests:

Failures:

  src/Test/Integration/Scenario/API/Shelley/StakePools.hs:850:19:
  1) API Specifications, SHELLEY_STAKE_POOLS, STAKE_POOLS_JOIN_01x - Fee boundary values, STAKE_POOLS_JOIN_01x - I can join if I have just the right amount
       expected: Status {statusCode = 202, statusMessage = "Accepted"}
        but got: Status {statusCode = 403, statusMessage = "Forbidden"}

       from the following response: Left (DecodeFailure "Error in $: parsing Cardano.Wallet.Api.Types.ApiTransaction(ApiTransaction) failed, key \"id\" not found: Response {responseStatus = Status {statusCode = 403, statusMessage = \"Forbidden\"}, responseVersion = HTTP/1.1, responseHeaders = [(\"Transfer-Encoding\",\"chunked\"),(\"Date\",\"Fri, 16 Jul 2021 03:40:18 GMT\"),(\"Server\",\"Warp/3.3.14\"),(\"Content-Type\",\"application/json;charset=utf-8\")], responseBody = \"{\\\"code\\\":\\\"cannot_cover_fee\\\",\\\"message\\\":\\\"I am unable to finalize the transaction, as there is not enough ada I can use to pay for fees, or to satisfy the minimum ada quantities of change outputs. I need about pretty 1.000500 ada to proceed; try increasing your wallet balance as such, or try sending a different, smaller payment.\\\"}\", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}")

       While verifying value:
         ( Status
             { statusCode = 403
             , statusMessage = "Forbidden"
             }
         , Left
             ( DecodeFailure "Error in $: parsing Cardano.Wallet.Api.Types.ApiTransaction(ApiTransaction) failed, key "id" not found: Response {responseStatus = Status {statusCode = 403, statusMessage = "Forbidden"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Fri, 16 Jul 2021 03:40:18 GMT"),("Server","Warp/3.3.14"),("Content-Type","application/json;charset=utf-8")], responseBody = "{\"code\":\"cannot_cover_fee\",\"message\":\"I am unable to finalize the transaction, as there is not enough ada I can use to pay for fees, or to satisfy the minimum ada quantities of change outputs. I need about pretty 1.000500 ada to proceed; try increasing your wallet balance as such, or try sending a different, smaller payment.\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}" )
         )

  To rerun use: --match "/API Specifications/SHELLEY_STAKE_POOLS/STAKE_POOLS_JOIN_01x - Fee boundary values/STAKE_POOLS_JOIN_01x - I can join if I have just the right amount/"

  src/Test/Integration/Scenario/API/Shelley/StakePools.hs:938:19:
  2) API Specifications, SHELLEY_STAKE_POOLS, STAKE_POOLS_QUIT_01x - Fee boundary values, STAKE_POOLS_QUIT_01x - I cannot quit if I have not enough to cover fees
       expected: Status {statusCode = 202, statusMessage = "Accepted"}
        but got: Status {statusCode = 403, statusMessage = "Forbidden"}

       from the following response: Left (DecodeFailure "Error in $: parsing Cardano.Wallet.Api.Types.ApiTransaction(ApiTransaction) failed, key \"id\" not found: Response {responseStatus = Status {statusCode = 403, statusMessage = \"Forbidden\"}, responseVersion = HTTP/1.1, responseHeaders = [(\"Transfer-Encoding\",\"chunked\"),(\"Date\",\"Fri, 16 Jul 2021 03:40:58 GMT\"),(\"Server\",\"Warp/3.3.14\"),(\"Content-Type\",\"application/json;charset=utf-8\")], responseBody = \"{\\\"code\\\":\\\"cannot_cover_fee\\\",\\\"message\\\":\\\"I am unable to finalize the transaction, as there is not enough ada I can use to pay for fees, or to satisfy the minimum ada quantities of change outputs. I need about pretty 1.000500 ada to proceed; try increasing your wallet balance as such, or try sending a different, smaller payment.\\\"}\", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}")

       While verifying value:
         ( Status
             { statusCode = 403
             , statusMessage = "Forbidden"
             }
         , Left
             ( DecodeFailure "Error in $: parsing Cardano.Wallet.Api.Types.ApiTransaction(ApiTransaction) failed, key "id" not found: Response {responseStatus = Status {statusCode = 403, statusMessage = "Forbidden"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Fri, 16 Jul 2021 03:40:58 GMT"),("Server","Warp/3.3.14"),("Content-Type","application/json;charset=utf-8")], responseBody = "{\"code\":\"cannot_cover_fee\",\"message\":\"I am unable to finalize the transaction, as there is not enough ada I can use to pay for fees, or to satisfy the minimum ada quantities of change outputs. I need about pretty 1.000500 ada to proceed; try increasing your wallet balance as such, or try sending a different, smaller payment.\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}" )
         )

  To rerun use: --match "/API Specifications/SHELLEY_STAKE_POOLS/STAKE_POOLS_QUIT_01x - Fee boundary values/STAKE_POOLS_QUIT_01x - I cannot quit if I have not enough to cover fees/"

Randomized with seed 1727163690

Finished in 1025.0644 seconds, used 538.3794 seconds of CPU time
837 examples, 2 failures, 25 pending

Error text:

"I am unable to finalize the transaction, as there is not enough ada I can use to pay for fees, or to satisfy the minimum ada quantities of change outputs. I need about 1.000500 ada to proceed; try increasing your wallet balance as such, or try sending a different, smaller payment."

#expected

jonathanknowles

Hi @sevanspowell!

It looks as though these tests are still failing:

#2741 (comment)

I can also reliably reproduce these failures locally, with:

stack build --fast --test cardano-wallet:test:integration --test-arguments '--match "STAKE_POOLS"'

From a quick look, these tests appear to probe the boundaries of fee calculations. So it's somewhat expected that these might fail if our fee calculations change.

Let me know if I can provide any assistance!

2762: Quieten `show` output for API types. r=rvl a=jonathanknowles # Issue Number None. (Produced while debugging test failures for #2741) # Overview Integration test failures occasionally produce very verbose output, similar to the following (an extract from a [multi-page error](https://hydra.iohk.io/build/6925587/nixlog/1)): ```hs , derivationPath = ApiT { getApiT = DerivationIndex { getDerivationIndex = 2147485500 } } :| [ ApiT { getApiT = DerivationIndex { getDerivationIndex = 2147485463 } } , ApiT { getApiT = DerivationIndex { getDerivationIndex = 2147483648 } } , ApiT { getApiT = DerivationIndex { getDerivationIndex = 0 } } , ApiT { getApiT = DerivationIndex { getDerivationIndex = 4 } } ] , amount = Quantity { getQuantity = 100000000000 } , assets = ApiT { getApiT = TokenMap ( fromList [] ) } ``` Most of the above output is **boilerplate**: the `getApiT` and `getDerivationIndex` deconstructor field names are not necessary to understand the output, as each of these types is just a wrapper type with exactly one field. We can remove this boilerplate by deriving our `Show` instances for newtypes via `Quiet`, which produces output similar to the following: ```hs , derivationPath = ApiT (DerivationIndex 2147485500) :| [ ApiT (DerivationIndex 2147485463) , ApiT (DerivationIndex 2147483648) , ApiT (DerivationIndex 0) , ApiT (DerivationIndex 4) ] , amount = Quantity 100000000000 , assets = ApiT (TokenMap (fromList [])) ``` Real log output, demonstrating the difference in verbosity: * [verbose.log](https://github.com/input-output-hk/cardano-wallet/files/6827653/verbose.log) `451 1451 23191 verbose.log` * [concise.log](https://github.com/input-output-hk/cardano-wallet/files/6827652/concise.log) `337 1027 16641 concise.log` Co-authored-by: Jonathan Knowles <[email protected]>

- We have increased the base tx size by 5 bytes with the inclusion of the validity interval start for a tx. Increase the magic constant in costOfJoining by 5 to match this. Note that increasing it by 4 causes the test to fail so I am confident this value is correct.

jonathanknowles

👍🏻

jonathanknowles · 2021-07-16T07:54:35Z

bors r+

iohk-bors · 2021-07-16T08:59:32Z

Build succeeded:

sevanspowell requested review from jonathanknowles and rvl June 25, 2021 08:44

sevanspowell self-assigned this Jun 25, 2021

sevanspowell commented Jul 12, 2021

View reviewed changes

sevanspowell force-pushed the sevanspowell/ADP-998/mint-burn-fee-estimation branch from 730347d to 5517e3f Compare July 13, 2021 02:28

sevanspowell marked this pull request as ready for review July 13, 2021 03:48

rvl approved these changes Jul 13, 2021

View reviewed changes

jonathanknowles suggested changes Jul 13, 2021

View reviewed changes

rvl approved these changes Jul 14, 2021

View reviewed changes

sevanspowell force-pushed the sevanspowell/ADP-998/mint-burn-fee-estimation branch from 8517e91 to 3e589c5 Compare July 14, 2021 06:44

jonathanknowles suggested changes Jul 14, 2021

View reviewed changes

jonathanknowles force-pushed the sevanspowell/ADP-998/mint-burn-fee-estimation branch from 0848d02 to f5788f5 Compare July 14, 2021 07:51

jonathanknowles approved these changes Jul 14, 2021

View reviewed changes

jonathanknowles changed the title ~~Initial commit of fee estimation work~~ Adjust fee estimation to account for minting and burning. Jul 14, 2021

jonathanknowles mentioned this pull request Jul 15, 2021

Quieten show output for API types. #2762

Merged

jonathanknowles suggested changes Jul 15, 2021

View reviewed changes

sevanspowell force-pushed the sevanspowell/ADP-998/mint-burn-fee-estimation branch from 7846243 to 1c3c5f9 Compare July 16, 2021 01:30

sevanspowell and others added 2 commits July 16, 2021 09:31

Fix formatting of estimateTxSize.

f82d8c5

sevanspowell force-pushed the sevanspowell/ADP-998/mint-burn-fee-estimation branch from 1c3c5f9 to f82d8c5 Compare July 16, 2021 01:32

jonathanknowles self-requested a review July 16, 2021 02:45

jonathanknowles approved these changes Jul 16, 2021

View reviewed changes

jonathanknowles self-requested a review July 16, 2021 04:03

jonathanknowles suggested changes Jul 16, 2021

View reviewed changes

jonathanknowles requested review from jonathanknowles and rvl July 16, 2021 07:51

jonathanknowles approved these changes Jul 16, 2021

View reviewed changes

iohk-bors bot merged commit a38c5e7 into master Jul 16, 2021

iohk-bors bot deleted the sevanspowell/ADP-998/mint-burn-fee-estimation branch July 16, 2021 08:59

		numberOf_ScriptVkeyWitnesses
		= sumVia scriptRequiredKeySigs txScripts

	it "increasing mint increases tx size at least proportionate to asset names"
	it "increasing mint increases tx size at least proportionally to asset names"

		& counterexample ("asset names length: "
		<> show lengthAssetNames)

		& counterexample ("witness size: "
		<> show (numWitnesses * sizeWitness))

Adjust fee estimation to account for minting and burning. #2741

Adjust fee estimation to account for minting and burning. #2741

Conversation

sevanspowell commented Jun 25, 2021 • edited by jonathanknowles Loading

Issue Number

Overview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl left a comment • edited Loading

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

jonathanknowles left a 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

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

rvl left a comment

Choose a reason for hiding this comment

sevanspowell commented Jul 14, 2021

iohk-bors bot commented Jul 14, 2021 • edited by Anviking Loading

jonathanknowles Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles commented Jul 14, 2021

iohk-bors bot commented Jul 14, 2021 • edited by Anviking Loading

jonathanknowles commented Jul 14, 2021

iohk-bors bot commented Jul 14, 2021 • edited by Anviking Loading

jonathanknowles left a comment • edited Loading

Choose a reason for hiding this comment

sevanspowell commented Jul 15, 2021 • edited Loading

Anviking commented Jul 15, 2021

sevanspowell commented Jul 16, 2021

iohk-bors bot commented Jul 16, 2021 • edited by Anviking Loading

jonathanknowles left a comment

Choose a reason for hiding this comment

sevanspowell commented Jul 16, 2021

iohk-bors bot commented Jul 16, 2021 • edited by Anviking Loading

jonathanknowles left a comment • edited Loading

Choose a reason for hiding this comment

jonathanknowles left a comment

Choose a reason for hiding this comment

sevanspowell commented Jun 25, 2021 •

edited by jonathanknowles

Loading

rvl left a comment •

edited

Loading

iohk-bors bot commented Jul 14, 2021 •

edited by Anviking

Loading

jonathanknowles Jul 14, 2021 •

edited

Loading

iohk-bors bot commented Jul 14, 2021 •

edited by Anviking

Loading

iohk-bors bot commented Jul 14, 2021 •

edited by Anviking

Loading

jonathanknowles left a comment •

edited

Loading

sevanspowell commented Jul 15, 2021 •

edited

Loading

iohk-bors bot commented Jul 16, 2021 •

edited by Anviking

Loading

iohk-bors bot commented Jul 16, 2021 •

edited by Anviking

Loading

jonathanknowles left a comment •

edited

Loading