Fixes for balanceTransaction in presence of scripts

[Anviking]

• Don't multiply with numberOfScripts in _maxScriptExecutionCost
• Fix mismatch of prMem and PrSteps in fromAlonzoPrices

Comments

• Thread: https://input-output-rnd.slack.com/archives/GBT05825V/p1634825136008200
• I believe our the fee minimisation using node functions otherwise hid the price mismatch. This is somewhat alarming, and it will be nice to both property test balanceTransaction as well as simplifying the implementation down the line.

Behaviour after fix, which I don't quite understand:

With manual w <- fixtureWalletWith @n ctx [15_000_000]

Failures:

  src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs:1341:54:
  1) API Specifications, NEW_SHELLEY_TRANSACTIONS, Plutus scenarios, game state-machine
       uncaught exception: RequestException
       ClientError (Object (fromList [("code",String "insufficient_collateral"),("message",String "I'm unable to create this transaction because the balance of pure ada UTxOs in your wallet is insufficient to cover the minimum amount of collateral required. I need an ada amount of at least: 2.262900 The largest combination of pure ada UTxOs I could find is: [] To fix this, you'll need to add one or more pure ada UTxOs to your wallet that can cover the minimum amount required.")]))

  To rerun use: --match "/API Specifications/NEW_SHELLEY_TRANSACTIONS/Plutus scenarios/game state-machine/"

  src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs:1341:54:
  2) API Specifications, NEW_SHELLEY_TRANSACTIONS, Plutus scenarios, mint-burn
       uncaught exception: RequestException
       ClientError (Object (fromList [("code",String "insufficient_collateral"),("message",String "I'm unable to create this transaction because the balance of pure ada UTxOs in your wallet is insufficient to cover the minimum amount of collateral required. I need an ada amount of at least: 2.214450 The largest combination of pure ada UTxOs I could find is: [] To fix this, you'll need to add one or more pure ada UTxOs to your wallet that can cover the minimum amount required.")]))

However I think it would be better to investigate while creating property tests and/or with simplified implementation.

Issue Number

ADP-1227

[jonathanknowles]

Just curious, do you know why it wouldn't be possible to use pp ^. #_prices here?

(Apologies if I'm missing something.)

[Anviking]

Not sure. The getField + HasField seems more straight-forward than the more magical ^. + OverloadedLabels. I tried some time ago but couldn't get ^. working.

[jonathanknowles]

Yeah, I just saw the same error. 🤷🏻‍♂️

Oh well, I guess it's something we can save for another day!

[jonathanknowles]

I found this code a little hard to understand, since the field names used in this function are quite different from the field names of the original record types. I think it would be good to use consistent terminology everywhere, to help people trying to understand the code in future.

Looking at the types themselves, I think I prefer your names!

Can we change the field names of ExecutionUnitPrices to something like:

data ExecutionUnitPrices = ExecutionUnitPrices
-     { priceExecutionSteps
+     { pricePerStep
          :: Rational
-     , priceExecutionMemory
+     , pricePerMemoryUnit
          :: Rational
      } deriving (Eq, Generic, Show)

I think calling it perMemoryUnit is also clearer from a plain English perspective (since "per memory" is a bit ambiguous).

Secondly, could we rewrite this function to use labels? Attempting to pattern match these records on a single line makes that line rather long. (This approach doesn't really scale if you have more parameters.)

So perhaps:

executionCost :: ExecutionUnitPrices -> ExecutionUnits -> Coin
executionCost ps us = Coin.unsafeNaturalToCoin . ceiling
    $ (ps ^. #pricePerStep)       * toRational (us ^. #steps)
    + (ps ^. #pricePerMemoryUnit) * toRational (us ^. #memoryUnits)

[jonathanknowles]

Update: I made a commit with suggested changes, which you could cherry-pick if you prefer:

20273eb

[Anviking]

Thanks, I think the renaming definitely makes sense.

Secondly, could we rewrite this function to use labels?

You do lose the ability to spot unused fields… but sure, its neater in other ways

[jonathanknowles]

LGTM!

[Anviking]

bors r+

[iohk-bors]

Build failed:

• buildkite/cardano-wallet

#expected

[Anviking]

bors r+

[iohk-bors]

Build succeeded:

• buildkite/cardano-wallet
• ci/hydra-build:required