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.

Sign up for GitHub

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

query protocol-parameters: use ledger JSON encoding, not API one #758

Merged
merged 2 commits into from
May 28, 2024
Merged

query protocol-parameters: use ledger JSON encoding, not API one #758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa7044c
query protocol parameters: use ledger JSON encoding, not API one
smelc May 13, 2024
87134c3
readProtocolParameters: return Ledger PParams instead of API ones
smelc May 15, 2024
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
1 change: 1 addition & 0 deletions cardano-cli/cardano-cli.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ library
, cardano-crypto-wrapper ^>= 1.5.1
, cardano-data >= 1.1
, cardano-git-rev ^>= 0.2.2
, cardano-ledger-api
, cardano-ledger-byron >= 1.0.1.0
, cardano-ping ^>= 0.2.0.13
, cardano-prelude
Expand Down
10 changes: 6 additions & 4 deletions cardano-cli/src/Cardano/CLI/EraBased/Run/Genesis.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1176,9 +1176,11 @@ readConwayGenesis fpath = do

--TODO: eliminate this and get only the necessary params, and get them in a more
-- helpful way rather than requiring them as a local file.
readProtocolParameters :: ProtocolParamsFile
-> ExceptT ProtocolParamsError IO ProtocolParameters
readProtocolParameters (ProtocolParamsFile fpath) = do
readProtocolParameters :: ()
=> ShelleyBasedEra era
-> ProtocolParamsFile
-> ExceptT ProtocolParamsError IO (L.PParams (ShelleyLedgerEra era))
readProtocolParameters sbe (ProtocolParamsFile fpath) = do
pparams <- handleIOExceptT (ProtocolParamsErrorFile . FileIOError fpath) $ LBS.readFile fpath
firstExceptT (ProtocolParamsErrorJSON fpath . Text.pack) . hoistEither $
Aeson.eitherDecode' pparams
shelleyBasedEraConstraints sbe $ Aeson.eitherDecode' pparams
5 changes: 3 additions & 2 deletions cardano-cli/src/Cardano/CLI/EraBased/Run/Query.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,15 +177,16 @@ runQueryProtocolParametersCmd
. newExceptT $ executeQueryAnyMode localNodeConnInfo qInMode
writeProtocolParameters sbe mOutFile pp
where
-- TODO: Conway era - use ledger PParams JSON
writeProtocolParameters
:: ShelleyBasedEra era
-> Maybe (File () Out)
-> L.PParams (ShelleyLedgerEra era)
-> ExceptT QueryCmdError IO ()
writeProtocolParameters sbe mOutFile' pparams =
firstExceptT QueryCmdWriteFileError . newExceptT
$ writeLazyByteStringOutput mOutFile' $ encodePretty $ fromLedgerPParams sbe pparams
$ writeLazyByteStringOutput mOutFile'
$ shelleyBasedEraConstraints sbe
$ encodePretty pparams
Copy link
Contributor

Choose a reason for hiding this comment

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

What about this TODO: https://github.com/IntersectMBO/cardano-cli/blob/main/cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs#L506
Are we able to round-trip i.e. read the parameters into the transaction build after your change? Afair there was a testnet test for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notes to myself:

  • On one hand, transaction build doesn't read the parameters by using the CLI's code: it requests them from the node
  • On the other hand, transaction build-raw does read parameters from the command line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carbolymer> actually there are multiple usages of the API JSON encoding:

  1. In build-estimate: https://github.com/IntersectMBO/cardano-cli/blob/main/cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs#L286
  2. In build-raw (the one you pointed out)
  3. In calculate-min-fee: https://github.com/IntersectMBO/cardano-cli/blob/main/cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs#L1261
  4. In calculate-min-value: https://github.com/IntersectMBO/cardano-cli/blob/main/cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs#L1342

Should we change all of them?

@gitmachtl> would you be able to test this change before we merge it? I'm afraid we are not the best ones to test this change exhaustively.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, also those are really important. let me compile the cli for testing...

Copy link
Contributor

@gitmachtl gitmachtl May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@smelc so first issue i see is, that cli is complaining about the missing parameter key "minUTxOValue" not found

so the parameters with null value that were sorted out are causing an issue here. cli expects at least that the key is in there.

afaik only those parameters are automatically "trimmed" out right now:

    "decentralization": null,
    "extraPraosEntropy": null,
    "minUTxOValue": null,

not sure if "decentralization" and "extraPraosEntropy" are actually ever read back for some processes? if not, only the "minUTxoValue" one is an issue. that parameter was only used pre babbage era, so would it be an option to only read that in if the used era expects it? utxoCostPerWord is also not present anymore in latest eras. but was used before, that might also be an issue during bootstrap phase to get thru the eras.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gitmachtl> is there a way I can reproduce your tests myself? So that then I can investigate how to fix the issues in autonomy.

Copy link
Contributor

@gitmachtl gitmachtl May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@smelc Sure, you can simply copy&paste those lines on the cli.

dummyTx='{"type":"Unwitnessed Tx ConwayEra","description":"Ledger Cddl Format","cborHex":"84a400d901028182582090ebfd31ec657d52eecf489871ecabefdce3753877aee7ee9e3200bd55fba9b800018282581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc745099441821a002f9d38ac581c01a7db2a2a897c4a84ca4b75b17281df80cc8607413644d21a9e020da14b73746172666f726765313002581c01c2435381c8362192bce939c002a99ce79c7bb7ba399cc55e2af07da14b73746172666f726765373901581c02f917e4ebe2f3474a713ec76e45e51f1ac72d5b053624d3ee124c12a14a73746172666f7267653201581c07e346cdcb11cff8048cf9ae2166a54c3324cf09b04c7554d6e94391a14b73746172666f726765373301581c0b6f1f1220140053fe1a9e44f4d77c12a5909f599970cd9b65e638e6a14b73746172666f726765343301581c0f28da1e6892acb5973ec68aab4e4718fec8761673e9d725343d97a9a14b73746172666f726765393701581c10ccfab9710c68068f3cd2bc86a96420c97b6d363a68b9232097b86aa14b73746172666f726765383801581c11b6c9e65d8e1e1bd51743a8599bfc8d70e1b1ccebb8902e32538c55a14b73746172666f726765313201581c12df514d7cd52225e39a7fe27ee0652dd2f1bfab05139deadda2a7caa14b73746172666f726765383901581c14deabc3a38365004e31a8a865a425e187fe4776e84c626c2c822344a148373536376130363301581cb2e796da6ef0fc4b427ca94f400691f22c949ed09ddd61592611e015a14b73746172666f726765343201581ce6911f8b6bae62fc20b96e55131bbf189334baab80d07f2964ca3b6da14a73746172666f726765300382581d6052e63f22c5107ed776b70f7b92248b02552fd08f3e747bc7450994411b000000058503a08a021a00030d40031a01ba9424a0f5f6"}'

protocolParametersJSON=$(cardano-cli conway query protocol-parameters)

cardano-cli conway transaction calculate-min-fee --tx-body-file <(echo ${dummyTx}) --protocol-params-file <(echo ${protocolParametersJSON}) --witness-count 1 --reference-script-size 0

Error: "Error in $: key \"extraPraosEntropy\" not found"

So the cli complains about the missing extrapraosEntropy parameter. To simulate the presence of the extraPraosEntropy we can do:

protocolParametersJSON=$(./cardano-cli conway query protocol-parameters | jq -r ". += {\"extraPraosEntropy\": null}")

cardano-cli conway transaction calculate-min-fee --tx-body-file <(echo ${dummyTx}) --protocol-params-file <(echo ${protocolParametersJSON}) --witness-count 1 --reference-script-size 0

Error: "Error in $: key \"minUTxOValue\" not found"

Now its complaining about the missing minUTxOVallue

If we also add this with the null value again, the fee calculation works:

protocolParametersJSON=$(./cardano-cli conway query protocol-parameters | jq -r ". += {\"extraPraosEntropy\": null, \"minUTxOValue\": null}")

cardano-cli conway transaction calculate-min-fee --tx-body-file <(echo ${dummyTx}) --protocol-params-file <(echo ${protocolParametersJSON}) --witness-count 1 --reference-script-size 0

189173 Lovelace

Same errors are for the calculate-min-required-utxo function.

Copy link
Contributor

Choose a reason for hiding this comment

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

@smelc yes, we should only use one encoding, so in this case ledger's.


-- | Calculate the percentage sync rendered as text.
percentage
Expand Down
43 changes: 13 additions & 30 deletions cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import Cardano.CLI.Types.Errors.TxCmdError
import Cardano.CLI.Types.Errors.TxValidationError
import Cardano.CLI.Types.Output (renderScriptCosts)
import Cardano.CLI.Types.TxFeature
import qualified Cardano.Ledger.Api.PParams as L
import qualified Ouroboros.Network.Protocol.LocalStateQuery.Type as Consensus
import qualified Ouroboros.Network.Protocol.LocalTxSubmission.Client as Net.Tx

Expand Down Expand Up @@ -283,9 +284,7 @@ runTransactionBuildEstimateCmd
, txBodyOutFile
} = do
let sbe = maryEraOnwardsToShelleyBasedEra eon
legacyPParams <- firstExceptT TxCmdProtocolParamsError $ readProtocolParameters protocolParamsFile
ledgerPParams <- hoistEither . first TxCmdProtocolParamsConverstionError
$ convertToLedgerProtocolParameters sbe legacyPParams
ledgerPParams <- firstExceptT TxCmdProtocolParamsError $ readProtocolParameters sbe protocolParamsFile
inputsAndMaybeScriptWits <- firstExceptT TxCmdScriptWitnessError
$ readScriptWitnessFiles sbe txins
certFilesAndMaybeScriptWits <- firstExceptT TxCmdScriptWitnessError
Expand Down Expand Up @@ -336,7 +335,7 @@ runTransactionBuildEstimateCmd

txBodyContent <- hoistEither $ constructTxBodyContent
sbe mScriptValidity
(Just $ unLedgerProtocolParameters ledgerPParams)
(Just ledgerPParams)
inputsAndMaybeScriptWits
readOnlyRefIns
filteredTxinsc
Expand Down Expand Up @@ -365,7 +364,7 @@ runTransactionBuildEstimateCmd

BalancedTxBody _ balancedTxBody _ _ <-
hoistEither $ first TxCmdFeeEstimationError $
estimateBalancedTxBody eon txBodyContent (unLedgerProtocolParameters ledgerPParams) poolsToDeregister
estimateBalancedTxBody eon txBodyContent ledgerPParams poolsToDeregister
stakeCredentialsToDeregisterMap drepsToDeregisterMap
pScriptExecUnits totCol shelleyWitnesses (fromMaybe 0 mByronWitnesses)
(maybe 0 unReferenceScriptSize totalReferenceScriptSize) (anyAddressInShelleyBasedEra sbe changeAddr)
Expand Down Expand Up @@ -503,15 +502,10 @@ runTransactionBuildRawCmd
mapM (readFileScriptInAnyLang . unFile) scriptFiles
txAuxScripts <- hoistEither $ first TxCmdAuxScriptsValidationError $ validateTxAuxScripts eon scripts

-- TODO: Conway era - update readProtocolParameters to rely on L.PParams JSON instances
pparams <- forM mProtocolParamsFile $ \ppf ->
firstExceptT TxCmdProtocolParamsError (readProtocolParameters ppf)
firstExceptT TxCmdProtocolParamsError (readProtocolParameters eon ppf)

mLedgerPParams <- case pparams of
Nothing -> return Nothing
Just pp -> do
ledgerpp <- firstExceptT TxCmdProtocolParamsConverstionError . hoistEither $ convertToLedgerProtocolParameters eon pp
return $ Just ledgerpp
let mLedgerPParams = LedgerProtocolParameters <$> pparams

txUpdateProposal <- case mUpdateProprosalFile of
Just (Featured w (Just updateProposalFile)) ->
Expand Down Expand Up @@ -1239,32 +1233,23 @@ runTransactionCalculateMinFeeCmd
unwitnessed <-
firstExceptT TxCmdTextEnvCddlError . newExceptT
$ readFileTxBody txbodyFile
pparams <-
firstExceptT TxCmdProtocolParamsError
$ readProtocolParameters protocolParamsFile

let nShelleyKeyWitW32 = fromIntegral nShelleyKeyWitnesses

InAnyShelleyBasedEra sbe txbody <- pure $ unIncompleteCddlTxBody unwitnessed

lpparams <- getLedgerPParams sbe pparams
lpparams <-
firstExceptT TxCmdProtocolParamsError
$ readProtocolParameters sbe protocolParamsFile

let shelleyfee = evaluateTransactionFee sbe lpparams txbody nShelleyKeyWitW32 0 sReferenceScript

let byronfee = calculateByronWitnessFees (protocolParamTxFeePerByte pparams) nByronKeyWitnesses
let byronfee = shelleyBasedEraConstraints sbe $ calculateByronWitnessFees (lpparams ^. L.ppMinFeeAL) nByronKeyWitnesses

let L.Coin fee = shelleyfee + byronfee

liftIO $ putStrLn $ (show fee :: String) <> " Lovelace"

getLedgerPParams :: forall era. ()
=> ShelleyBasedEra era
-> ProtocolParameters
-> ExceptT TxCmdError IO (L.PParams (ShelleyLedgerEra era))
getLedgerPParams sbe pparams =
firstExceptT TxCmdProtocolParamsConverstionError $
hoistEither $ toLedgerPParams sbe pparams

-- Extra logic to handle byron witnesses.
-- TODO: move this to Cardano.API.Fee.evaluateTransactionFee.
calculateByronWitnessFees :: ()
Expand Down Expand Up @@ -1317,14 +1302,12 @@ runTransactionCalculateMinValueCmd
, protocolParamsFile
, txOut
} = do
pp <- firstExceptT TxCmdProtocolParamsError (readProtocolParameters protocolParamsFile)
pp <- firstExceptT TxCmdProtocolParamsError (readProtocolParameters eon protocolParamsFile)
out <- toTxOutInShelleyBasedEra eon txOut
-- TODO: shouldn't we just require shelley based era here instead of error-ing for byron?

firstExceptT TxCmdPParamsErr . hoistEither
$ checkProtocolParameters eon pp
pp' <- hoistEither . first TxCmdProtocolParamsConverstionError $ toLedgerPParams eon pp
let minValue = calculateMinimumUTxO eon out pp'
$ checkProtocolParameters eon (fromLedgerPParams eon pp)
let minValue = calculateMinimumUTxO eon out pp
liftIO . IO.print $ minValue

runTransactionPolicyIdCmd :: ()
Expand Down
Loading