Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect fees estimation when balancing transaction minting assets #622

Merged
merged 2 commits into from
Aug 29, 2024
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
2 changes: 2 additions & 0 deletions cardano-api/cardano-api.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,8 @@ test-suite cardano-api-test
cardano-ledger-api ^>=1.9,
cardano-ledger-binary,
cardano-ledger-core:{cardano-ledger-core, testlib} >=1.8,
cardano-ledger-mary,
cardano-ledger-shelley,
cardano-protocol-tpraos,
cardano-slotting,
cborg,
Expand Down
2 changes: 1 addition & 1 deletion cardano-api/gen/Test/Gen/Cardano/Api/Typed.hs
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ genPlutusScript _ =
genScriptDataSchema :: Gen ScriptDataJsonSchema
genScriptDataSchema = Gen.element [ScriptDataJsonNoSchema, ScriptDataJsonDetailedSchema]

genHashableScriptData :: Gen HashableScriptData
genHashableScriptData :: HasCallStack => Gen HashableScriptData
genHashableScriptData = do
sd <- genScriptData
case deserialiseFromCBOR AsHashableScriptData $ serialiseToCBOR sd of
Expand Down
4 changes: 2 additions & 2 deletions cardano-api/internal/Cardano/Api/Experimental/Tx.hs
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ import Cardano.Ledger.Hashes
import qualified Cardano.Ledger.Keys as L
import qualified Cardano.Ledger.SafeHash as L

import qualified Data.Map.Strict as Map
import qualified Data.Set as Set
import GHC.Exts (IsList (..))
import Lens.Micro

-- | A transaction that can contain everything
Expand Down Expand Up @@ -115,7 +115,7 @@ makeUnsignedTx era bc = obtainCommonConstraints era $ do
scriptWitnesses =
L.mkBasicTxWits
& L.scriptTxWitsL
.~ Map.fromList
.~ fromList
[ (L.hashScript sw, sw)
| sw <- scripts
]
Expand Down
22 changes: 10 additions & 12 deletions cardano-api/internal/Cardano/Api/Fees.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1062,6 +1062,12 @@ makeTransactionBodyAutoBalance
-- 2. figure out the overall min fees
-- 3. update tx with fees
-- 4. balance the transaction and update tx change output

let totalValueAtSpendableUTxO = fromLedgerValue sbe . calculateIncomingUTxOValue . Map.elems $ unUTxO utxo
change =
monoidForEraInEon (toCardanoEra sbe) $ \w ->
toLedgerValue w $ calculateChangeValue sbe totalValueAtSpendableUTxO txbodycontent

UnsignedTx unsignedTx0 <-
first TxBodyError
$ makeUnsignedTx
Expand All @@ -1070,9 +1076,7 @@ makeTransactionBodyAutoBalance
$ txbodycontent
{ txOuts =
txOuts txbodycontent
++ [TxOut changeaddr (lovelaceToTxOutValue sbe 0) TxOutDatumNone ReferenceScriptNone]
-- TODO: think about the size of the change output
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jimbo4350 What does this TODO mean? This PR changes the TxOut to include all assets from TxIns and minted ones, so I guess this TODO is fixed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Duncan left that there and I can't remember why.

-- 1,2,4 or 8 bytes?
<> [TxOut changeaddr (TxOutValueShelleyBased sbe change) TxOutDatumNone ReferenceScriptNone]
}
exUnitsMapWithLogs <-
first TxBodyErrorValidityInterval
Expand Down Expand Up @@ -1109,12 +1113,6 @@ makeTransactionBodyAutoBalance
let maxLovelaceChange = L.Coin (2 ^ (64 :: Integer)) - 1
let maxLovelaceFee = L.Coin (2 ^ (32 :: Integer) - 1)

let totalValueAtSpendableUTxO = fromLedgerValue sbe $ calculateIncomingUTxOValue $ Map.elems $ unUTxO utxo
let change =
forShelleyBasedEraInEon
sbe
mempty
(\w -> toLedgerValue w $ calculateChangeValue sbe totalValueAtSpendableUTxO txbodycontent1)
let changeWithMaxLovelace = change & A.adaAssetL sbe .~ maxLovelaceChange
let changeTxOut =
forShelleyBasedEraInEon
Expand All @@ -1131,8 +1129,8 @@ makeTransactionBodyAutoBalance
$ txbodycontent1
{ txFee = TxFeeExplicit sbe maxLovelaceFee
, txOuts =
TxOut changeaddr changeTxOut TxOutDatumNone ReferenceScriptNone
: txOuts txbodycontent
txOuts txbodycontent
<> [TxOut changeaddr changeTxOut TxOutDatumNone ReferenceScriptNone]
, txReturnCollateral = dummyCollRet
, txTotalCollateral = dummyTotColl
}
Expand Down Expand Up @@ -1278,7 +1276,7 @@ isNotAda AdaAssetId = False
isNotAda _ = True

onlyAda :: Value -> Bool
onlyAda = null . valueToList . filterValue isNotAda
onlyAda = null . toList . filterValue isNotAda

calculateIncomingUTxOValue
:: Monoid (Ledger.Value (ShelleyLedgerEra era))
Expand Down
15 changes: 9 additions & 6 deletions cardano-api/internal/Cardano/Api/Tx/Body.hs
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ import qualified Data.Text as Text
import Data.Type.Equality (TestEquality (..), (:~:) (Refl))
import Data.Word (Word16, Word32, Word64)
import GHC.Exts (IsList (..))
import GHC.Stack
import Lens.Micro hiding (ix)
import Lens.Micro.Extras (view)
import qualified Text.Parsec as Parsec
Expand Down Expand Up @@ -709,7 +710,8 @@ toByronTxOut = \case

toShelleyTxOut
:: forall era ledgerera
. ShelleyLedgerEra era ~ ledgerera
. HasCallStack
=> ShelleyLedgerEra era ~ ledgerera
=> ShelleyBasedEra era
-> TxOut CtxUTxO era
-> Ledger.TxOut ledgerera
Expand Down Expand Up @@ -959,12 +961,12 @@ instance IsShelleyBasedEra era => FromJSON (TxOutValue era) where
decodeAssetId (polid, Aeson.Object assetNameHm) = do
let polId = fromString . Text.unpack $ Aeson.toText polid
aNameQuantity <- decodeAssets assetNameHm
pure . valueFromList $
pure . fromList $
map (first $ AssetId polId) aNameQuantity
decodeAssetId ("lovelace", Aeson.Number sci) =
case toBoundedInteger sci of
Just (ll :: Word64) ->
pure $ valueFromList [(AdaAssetId, Quantity $ toInteger ll)]
pure $ fromList [(AdaAssetId, Quantity $ toInteger ll)]
Nothing ->
fail $ "Expected a Bounded number but got: " <> show sci
decodeAssetId wrong = fail $ "Expected a policy id and a JSON object but got: " <> show wrong
Expand Down Expand Up @@ -1829,7 +1831,7 @@ outputDoesNotExceedMax
-> TxOut CtxTx era
-> Either TxBodyError ()
outputDoesNotExceedMax era v txout =
case [q | (_, q) <- valueToList v, q > maxTxOut] of
case [q | (_, q) <- toList v, q > maxTxOut] of
[] -> Right ()
q : _ -> Left (TxBodyOutputOverflow q (txOutInAnyEra era txout))

Expand All @@ -1840,7 +1842,7 @@ positiveOutput
-> TxOut CtxTx era
-> Either TxBodyError ()
positiveOutput era v txout =
case [q | (_, q) <- valueToList v, q < 0] of
case [q | (_, q) <- toList v, q < 0] of
[] -> Right ()
q : _ -> Left (TxBodyOutputNegative q (txOutInAnyEra era txout))

Expand Down Expand Up @@ -3057,7 +3059,8 @@ makeShelleyTransactionBody
-- embedded datums (taking only their hash).
toShelleyTxOutAny
:: forall ctx era ledgerera
. ShelleyLedgerEra era ~ ledgerera
. HasCallStack
=> ShelleyLedgerEra era ~ ledgerera
=> ShelleyBasedEra era
-> TxOut ctx era
-> Ledger.TxOut ledgerera
Expand Down
19 changes: 13 additions & 6 deletions cardano-api/internal/Cardano/Api/Value.hs
Original file line number Diff line number Diff line change
Expand Up @@ -189,14 +189,22 @@ newtype Value = Value (Map AssetId Quantity)
instance Show Value where
showsPrec d v =
showParen (d > 10) $
showString "valueFromList " . shows (valueToList v)
showString "valueFromList " . shows (toList v)

instance Semigroup Value where
Value a <> Value b = Value (mergeAssetMaps a b)

instance Monoid Value where
mempty = Value Map.empty

instance IsList Value where
type Item Value = (AssetId, Quantity)
fromList =
Value
. Map.filter (/= 0)
. Map.fromListWith (<>)
toList (Value m) = toList m

{-# NOINLINE mergeAssetMaps #-} -- as per advice in Data.Map.Merge docs
mergeAssetMaps
:: Map AssetId Quantity
Expand All @@ -223,14 +231,13 @@ instance FromJSON Value where
selectAsset :: Value -> (AssetId -> Quantity)
selectAsset (Value m) a = Map.findWithDefault mempty a m

{-# DEPRECATED valueFromList "Use 'fromList' instead." #-}
valueFromList :: [(AssetId, Quantity)] -> Value
valueFromList =
Value
. Map.filter (/= 0)
. Map.fromListWith (<>)
valueFromList = fromList

{-# DEPRECATED valueToList "Use 'toList' instead." #-}
valueToList :: Value -> [(AssetId, Quantity)]
valueToList (Value m) = toList m
valueToList = toList

-- | This lets you write @a - b@ as @a <> negateValue b@.
negateValue :: Value -> Value
Expand Down
7 changes: 4 additions & 3 deletions cardano-api/internal/Cardano/Api/ValueParser.hs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
{-# LANGUAGE OverloadedLists #-}

module Cardano.Api.ValueParser
( parseValue
, assetName
Expand Down Expand Up @@ -34,9 +36,8 @@ evalValueExpr vExpr =
case vExpr of
ValueExprAdd x y -> evalValueExpr x <> evalValueExpr y
ValueExprNegate x -> negateValue (evalValueExpr x)
ValueExprLovelace quant -> valueFromList [(AdaAssetId, quant)]
ValueExprMultiAsset polId aName quant ->
valueFromList [(AssetId polId aName, quant)]
ValueExprLovelace quant -> [(AdaAssetId, quant)]
ValueExprMultiAsset polId aName quant -> [(AssetId polId aName, quant)]

------------------------------------------------------------------------------
-- Expression parser
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import Data.Aeson (eitherDecode, encode)
import Data.List (groupBy, sort)
import qualified Data.Map.Strict as Map
import qualified Data.Text as Text
import GHC.Exts (IsList (..))
import qualified Text.Parsec as Parsec (parse)

import Test.Gen.Cardano.Api.Typed (genAssetName, genValueDefault, genValueNestedRep)
Expand Down Expand Up @@ -47,7 +48,7 @@ hprop_goldenValue_1_lovelace :: Property
hprop_goldenValue_1_lovelace =
H.propertyOnce $ do
valueList <- pure [(Api.AdaAssetId, 1)]
value <- pure $ Text.unpack $ Api.renderValuePretty $ Api.valueFromList valueList
value <- pure $ Text.unpack $ Api.renderValuePretty $ fromList valueList

H.diffVsGoldenFile value "test/cardano-api-golden/files/golden/Cardano/Api/Value/value-ada-1.json"

Expand All @@ -57,7 +58,7 @@ hprop_goldenValue1 =
policyId <- pure $ Api.PolicyId "a0000000000000000000000000000000000000000000000000000000"
assetName <- pure $ Api.AssetName "asset1"
valueList <- pure [(Api.AssetId policyId assetName, 1)]
value <- pure $ Text.unpack $ Api.renderValuePretty $ Api.valueFromList valueList
value <- pure $ Text.unpack $ Api.renderValuePretty $ fromList valueList

H.diffVsGoldenFile
value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ test_TxBodyErrorAutoBalance =
, ("TxBodyErrorMinUTxONotMet", TxBodyErrorMinUTxONotMet txOutInAnyEra1 1)
,
( "TxBodyErrorNonAdaAssetsUnbalanced"
, TxBodyErrorNonAdaAssetsUnbalanced (valueFromList [(AdaAssetId, Quantity 1)])
, TxBodyErrorNonAdaAssetsUnbalanced (fromList [(AdaAssetId, Quantity 1)])
)
,
( "TxBodyErrorScriptWitnessIndexMissingFromExecUnitsMap"
Expand Down
Loading
Loading