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

Use evaluateTransactionFee in calculate-min-fee #642

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Mar 11, 2024

Changelog

- description: |
    Fix calculate-min-fee by switching to use Cardano.Api.evaluateTransactionFee
    instead of the deprecated Cardano.Api.estimateTransactionFee.
    This also deprecates the --mainnet, --testnet-magic, --tx-in-count,
    and --tx-out-count command args, which are no longer necessary.
    They can still be provided, but have no effect.
# uncomment types applicable to the change:
  type:
  - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  - improvement    # QoL changes e.g. refactoring
  - bugfix         # fixes a defect
  - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Original PR: #534

Cardano.Api.estimateTransactionFee (currently used by Cardano CLI calculate-min-fee) tends to overestimate transaction fees. Furthermore, inspecting the code, estimateTransactionFee appears to only apply the old a * x + b fee formula to the transaction size (augmented to take into account the user-provided Shelley and Byron witness counts). This is not how fees should be calculated for later eras.

A deprecation note is currently attached to Cardano.Api.estimateTransactionFee, saying that Cardano.Api.evaluateTransactionFee should be used instead. Inspecting the code, evaluateTransactionFee calls the Cardano Ledger's era-aware fee calculation function Cardano.Ledger.Shelley.API.Wallet.evaluateTransactionFee, which in turn calls the EraTx class function getMinFeeTx that is instantiated for every Cardano Ledger era.

https://github.com/IntersectMBO/cardano-api/blob/1c10a287093ea1396c4c0ef0c92460f60bab690f/cardano-api/internal/Cardano/Api/Fees.hs#L190

Therefore, cardano-cli should be updated to use Cardano.Api.evaluateTransactionFee in the handler for calculate-min-fee.

https://twitter.com/amw7/status/1740008349905928326

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

New golden test to calculate the min fee of a Babbage transaction body with Babbage protocol parameters, comparing against the known minimum fee (below which the transaction gets rejected when submitted to node).

https://github.com/IntersectMBO/cardano-cli/pull/534/files#diff-8d95bc46c5ef4bbab74b92c6b29635f1c3792d1e084d8c7668b0ce75bf9d2b52

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@newhoggy newhoggy force-pushed the newhoggy/use-evaluateTransactionFee-in-calculateMinFee branch from d6fbf0b to 93890cb Compare March 16, 2024 05:23
Cardano.Api.estimateTransactionFee is deprecated and tends to
over-estimate transaction fees.

There is a deprecation note currently attached to
Cardano.Api.estimateTransactionFee, saying that
Cardano.Api.evaluateTransactionFee should be used instead.

https://github.com/IntersectMBO/cardano-api/blob/1c10a287093ea1396c4c0ef0c92460f60bab690f/cardano-api/internal/Cardano/Api/Fees.hs#L190

Therefore, cardano-cli should be updated to use
Cardano.Api.evaluateTransactionFee in the handler for calculate-min-fee.

The existing golden test for calculate-min-fee was adjusted
and another golden test with a babbage transaction was added.
@newhoggy newhoggy force-pushed the newhoggy/use-evaluateTransactionFee-in-calculateMinFee branch from 93890cb to 32c2d75 Compare March 16, 2024 05:26
@newhoggy newhoggy requested a review from lehins March 16, 2024 05:36
@newhoggy
Copy link
Contributor Author

CC @GeorgeFlerovsky @gitmachtl

@newhoggy newhoggy requested a review from smelc March 16, 2024 05:47
@newhoggy newhoggy added this pull request to the merge queue Mar 18, 2024
Merged via the queue into main with commit a68f9f7 Mar 18, 2024
17 checks passed
@newhoggy newhoggy deleted the newhoggy/use-evaluateTransactionFee-in-calculateMinFee branch March 18, 2024 09:07
@GeorgeFlerovsky
Copy link
Contributor

🎉

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM. We can now use calcMinFeeTx for a more accurate fee calculation.


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

UnwitnessedCliFormattedTxBody anyTxBody -> do
InAnyShelleyBasedEra sbe txbody <- pure anyTxBody
UnwitnessedCliFormattedTxBody (InAnyShelleyBasedEra sbe txbody) -> do
Copy link
Contributor

Choose a reason for hiding this comment

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

We can get rid UnwitnessedCliFormattedTxBody

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants