-
Notifications
You must be signed in to change notification settings - Fork 214
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 cardano-ledger
for all minimum UTxO calculations
#3456
Use cardano-ledger
for all minimum UTxO calculations
#3456
Conversation
9d84e68
to
5d8f504
Compare
9566e7a
to
acbfaad
Compare
bors try |
acbfaad
to
43a604a
Compare
tryBuild succeeded: |
Internal.computeMinimumCoinForUTxO_CardanoLedger | ||
minimumUTxO txOut | ||
where | ||
txOut = TxOut address (TokenBundle coin tokenMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Why not take a TokenBundle
directly like in other properties? It seems you needed to define Arbitrary Coin
specially for this property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Why not take a
TokenBundle
directly like in other properties? It seems you needed to defineArbitrary Coin
specially for this property.
Good question! I've simplified this in 9f37edc.
-- This function should /only/ be used to validate existing 'Coin' values that | ||
-- do not need to be modified in any way. | ||
-- | ||
-- Increasing the 'Coin' value of an output can lead to an increase in the | ||
-- serialized length of that output, which can in turn lead to an increase in | ||
-- the minimum required 'Coin' value, since the minimum required 'Coin' value | ||
-- is dependent on an output's serialized length. | ||
-- | ||
-- Therefore, even if this function indicates that a given value 'Coin' value | ||
-- 'c' satisfies the minimum UTxO rule, it should not be taken to imply that | ||
-- all values greater than 'c' will also satisfy the minimum UTxO rule. | ||
-- | ||
-- If you need to generate a value that can always safely be increased, use | ||
-- the 'computeMinimumCoinForUTxO' function instead. | ||
-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
$ toAlonzoTxOut txOut Nothing | ||
Cardano.ShelleyBasedEraBabbage -> | ||
evaluateMinLovelaceOutput pp | ||
$ toBabbageTxOut txOut Nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly we could move the case era of
to a toLedger :: ShelleyEra era => W.TxOut -> Ledger.TxOut era
? but it doesn't matter now; we'll see how things turn out in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It'd be nice to factor out this pattern match as you suggest. It's something we could do in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did give this a try, but couldn't get the types to line up in a reasonable amount of time.
The ledger code has so many re-exports that I just gave up trying to figure out which things to import...
bors r+ |
3456: Use `cardano-ledger` for all minimum UTxO calculations r=Anviking a=jonathanknowles ## Issue Number ADP-2144 ## Summary This PR: - uses the ledger function [`evaluateMinLovelaceOutput`](https://github.com/input-output-hk/cardano-ledger/blob/68a535603c80b7cf53425fd34558e23f9983dfe8/eras/shelley/impl/src/Cardano/Ledger/Shelley/API/Wallet.hs#L506) to replace the use of Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) in all implementation code, for all eras. - repurposes the Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) for use as an oracle, to compare against when testing. - removes all special-casing for the Babbage era, and replaces it with a simpler implementation that works in all eras. - revises documentation for the `{compute,isBelow}minimumCoinForUTxO` functions to explain their intended purpose more clearly. ## Context - The ledger function [`evaluateMinLovelaceOutput`](https://github.com/input-output-hk/cardano-ledger/blob/68a535603c80b7cf53425fd34558e23f9983dfe8/eras/shelley/impl/src/Cardano/Ledger/Shelley/API/Wallet.hs#L506) accepts an _era-specific_ protocol parameters object and, unlike like the Cardano API, does _not_ require that we convert to an era-agnostic protocol parameters object. Since values of the `MinimumUTxO` type already contain era-specific protocol parameters, this means we can avoid the extra complication of record conversion whenever we call: - `isBelowMinimumCoinForUTxO` - `computeMinimumCoinForUTxO` ## Performance Mainnet (Alonzo): ```sh $ time curl -X POST http://localhost:8091/v2/wallets/042094088ed439a7b14e811e0781a00185b921c2/payment-fees -d '{"payments":[{"address":"addr1qylgm2dh3vpv07cjfcyuu6vhaqhf8998qcx6s8ucpkly6f8l0dw5r75vk42mv3ykq8vyjeaanvpytg79xqzymqy5acmqgyxuyr","amount":{"quantity":100000000,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":187633,"unit":"lovelace"},"estimated_min":{"quantity":169021,"unit":"lovelace"},"minimum_coins":[{"quantity":999978,"unit":"lovelace"}]} real 0m0,029s user 0m0,006s sys 0m0,000s ``` Preprod (Alonzo): ```sh $ time curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/payment-fees \ > -d '{"payments":[{"address":"addr_test1qrtez7vn0d8xp495ggypmu2kyt7tt6qyva2spm0f5a3ewn0v474mcs4q8e9g55yknx3729kyg5dl69x5596ee9tvnynq7ffety","amount":{"quantity":1000000,"unit":"lovelace"}}]}' \ > -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":169021,"unit":"lovelace"},"estimated_min":{"quantity":169021,"unit":"lovelace"},"minimum_coins":[{"quantity":999978,"unit":"lovelace"}]} real 0m0,029s user 0m0,006s sys 0m0,000s ``` Preview (Babbage): ```sh $ time curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/payment-fees -d '{"payments":[{"address":"addr_test1qrtez7vn0d8xp495ggypmu2kyt7tt6qyva2spm0f5a3ewn0v474mcs4q8e9g55yknx3729kyg5dl69x5596ee9tvnynq7ffety","amount":{"quantity":100000000,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":187809,"unit":"lovelace"},"estimated_min":{"quantity":169197,"unit":"lovelace"},"minimum_coins":[{"quantity":995610,"unit":"lovelace"}]} real 0m0,036s user 0m0,003s sys 0m0,003s ``` Co-authored-by: Jonathan Knowles <[email protected]>
Build failed:
|
bors r+ |
3456: Use `cardano-ledger` for all minimum UTxO calculations r=jonathanknowles a=jonathanknowles ## Issue Number ADP-2144 ## Summary This PR: - uses the ledger function [`evaluateMinLovelaceOutput`](https://github.com/input-output-hk/cardano-ledger/blob/68a535603c80b7cf53425fd34558e23f9983dfe8/eras/shelley/impl/src/Cardano/Ledger/Shelley/API/Wallet.hs#L506) to replace the use of Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) in all implementation code, for all eras. - repurposes the Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) for use as an oracle, to compare against when testing. - removes all special-casing for the Babbage era, and replaces it with a simpler implementation that works in all eras. - revises documentation for the `{compute,isBelow}minimumCoinForUTxO` functions to explain their intended purpose more clearly. ## Context - The ledger function [`evaluateMinLovelaceOutput`](https://github.com/input-output-hk/cardano-ledger/blob/68a535603c80b7cf53425fd34558e23f9983dfe8/eras/shelley/impl/src/Cardano/Ledger/Shelley/API/Wallet.hs#L506) accepts an _era-specific_ protocol parameters object and, unlike like the Cardano API, does _not_ require that we convert to an era-agnostic protocol parameters object. Since values of the `MinimumUTxO` type already contain era-specific protocol parameters, this means we can avoid the extra complication of record conversion whenever we call: - `isBelowMinimumCoinForUTxO` - `computeMinimumCoinForUTxO` ## Performance Mainnet (Alonzo): ```sh $ time curl -X POST http://localhost:8091/v2/wallets/042094088ed439a7b14e811e0781a00185b921c2/payment-fees -d '{"payments":[{"address":"addr1qylgm2dh3vpv07cjfcyuu6vhaqhf8998qcx6s8ucpkly6f8l0dw5r75vk42mv3ykq8vyjeaanvpytg79xqzymqy5acmqgyxuyr","amount":{"quantity":100000000,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":187633,"unit":"lovelace"},"estimated_min":{"quantity":169021,"unit":"lovelace"},"minimum_coins":[{"quantity":999978,"unit":"lovelace"}]} real 0m0,029s user 0m0,006s sys 0m0,000s ``` Preprod (Alonzo): ```sh $ time curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/payment-fees \ > -d '{"payments":[{"address":"addr_test1qrtez7vn0d8xp495ggypmu2kyt7tt6qyva2spm0f5a3ewn0v474mcs4q8e9g55yknx3729kyg5dl69x5596ee9tvnynq7ffety","amount":{"quantity":1000000,"unit":"lovelace"}}]}' \ > -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":169021,"unit":"lovelace"},"estimated_min":{"quantity":169021,"unit":"lovelace"},"minimum_coins":[{"quantity":999978,"unit":"lovelace"}]} real 0m0,029s user 0m0,006s sys 0m0,000s ``` Preview (Babbage): ```sh $ time curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/payment-fees -d '{"payments":[{"address":"addr_test1qrtez7vn0d8xp495ggypmu2kyt7tt6qyva2spm0f5a3ewn0v474mcs4q8e9g55yknx3729kyg5dl69x5596ee9tvnynq7ffety","amount":{"quantity":100000000,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":187809,"unit":"lovelace"},"estimated_min":{"quantity":169197,"unit":"lovelace"},"minimum_coins":[{"quantity":995610,"unit":"lovelace"}]} real 0m0,036s user 0m0,003s sys 0m0,003s ``` Co-authored-by: Jonathan Knowles <[email protected]>
This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried. |
3456: Use `cardano-ledger` for all minimum UTxO calculations r=jonathanknowles a=jonathanknowles ## Issue Number ADP-2144 ## Summary This PR: - uses the ledger function [`evaluateMinLovelaceOutput`](https://github.com/input-output-hk/cardano-ledger/blob/68a535603c80b7cf53425fd34558e23f9983dfe8/eras/shelley/impl/src/Cardano/Ledger/Shelley/API/Wallet.hs#L506) to replace the use of Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) in all implementation code, for all eras. - repurposes the Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) for use as an oracle, to compare against when testing. - removes all special-casing for the Babbage era, and replaces it with a simpler implementation that works in all eras. - revises documentation for the `{compute,isBelow}minimumCoinForUTxO` functions to explain their intended purpose more clearly. ## Context - The ledger function [`evaluateMinLovelaceOutput`](https://github.com/input-output-hk/cardano-ledger/blob/68a535603c80b7cf53425fd34558e23f9983dfe8/eras/shelley/impl/src/Cardano/Ledger/Shelley/API/Wallet.hs#L506) accepts an _era-specific_ protocol parameters object and, unlike like the Cardano API, does _not_ require that we convert to an era-agnostic protocol parameters object. Since values of the `MinimumUTxO` type already contain era-specific protocol parameters, this means we can avoid the extra complication of record conversion whenever we call: - `isBelowMinimumCoinForUTxO` - `computeMinimumCoinForUTxO` ## Performance Mainnet (Alonzo): ```sh $ time curl -X POST http://localhost:8091/v2/wallets/042094088ed439a7b14e811e0781a00185b921c2/payment-fees -d '{"payments":[{"address":"addr1qylgm2dh3vpv07cjfcyuu6vhaqhf8998qcx6s8ucpkly6f8l0dw5r75vk42mv3ykq8vyjeaanvpytg79xqzymqy5acmqgyxuyr","amount":{"quantity":100000000,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":187633,"unit":"lovelace"},"estimated_min":{"quantity":169021,"unit":"lovelace"},"minimum_coins":[{"quantity":999978,"unit":"lovelace"}]} real 0m0,029s user 0m0,006s sys 0m0,000s ``` Preprod (Alonzo): ```sh $ time curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/payment-fees \ > -d '{"payments":[{"address":"addr_test1qrtez7vn0d8xp495ggypmu2kyt7tt6qyva2spm0f5a3ewn0v474mcs4q8e9g55yknx3729kyg5dl69x5596ee9tvnynq7ffety","amount":{"quantity":1000000,"unit":"lovelace"}}]}' \ > -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":169021,"unit":"lovelace"},"estimated_min":{"quantity":169021,"unit":"lovelace"},"minimum_coins":[{"quantity":999978,"unit":"lovelace"}]} real 0m0,029s user 0m0,006s sys 0m0,000s ``` Preview (Babbage): ```sh $ time curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/payment-fees -d '{"payments":[{"address":"addr_test1qrtez7vn0d8xp495ggypmu2kyt7tt6qyva2spm0f5a3ewn0v474mcs4q8e9g55yknx3729kyg5dl69x5596ee9tvnynq7ffety","amount":{"quantity":100000000,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":187809,"unit":"lovelace"},"estimated_min":{"quantity":169197,"unit":"lovelace"},"minimum_coins":[{"quantity":995610,"unit":"lovelace"}]} real 0m0,036s user 0m0,003s sys 0m0,003s ``` Co-authored-by: Jonathan Knowles <[email protected]>
Build failed:
|
bors r+ |
3456: Use `cardano-ledger` for all minimum UTxO calculations r=jonathanknowles a=jonathanknowles ## Issue Number ADP-2144 ## Summary This PR: - uses the ledger function [`evaluateMinLovelaceOutput`](https://github.com/input-output-hk/cardano-ledger/blob/68a535603c80b7cf53425fd34558e23f9983dfe8/eras/shelley/impl/src/Cardano/Ledger/Shelley/API/Wallet.hs#L506) to replace the use of Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) in all implementation code, for all eras. - repurposes the Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) for use as an oracle, to compare against when testing. - removes all special-casing for the Babbage era, and replaces it with a simpler implementation that works in all eras. - revises documentation for the `{compute,isBelow}minimumCoinForUTxO` functions to explain their intended purpose more clearly. ## Context - The ledger function [`evaluateMinLovelaceOutput`](https://github.com/input-output-hk/cardano-ledger/blob/68a535603c80b7cf53425fd34558e23f9983dfe8/eras/shelley/impl/src/Cardano/Ledger/Shelley/API/Wallet.hs#L506) accepts an _era-specific_ protocol parameters object and, unlike like the Cardano API, does _not_ require that we convert to an era-agnostic protocol parameters object. Since values of the `MinimumUTxO` type already contain era-specific protocol parameters, this means we can avoid the extra complication of record conversion whenever we call: - `isBelowMinimumCoinForUTxO` - `computeMinimumCoinForUTxO` ## Performance Mainnet (Alonzo): ```sh $ time curl -X POST http://localhost:8091/v2/wallets/042094088ed439a7b14e811e0781a00185b921c2/payment-fees -d '{"payments":[{"address":"addr1qylgm2dh3vpv07cjfcyuu6vhaqhf8998qcx6s8ucpkly6f8l0dw5r75vk42mv3ykq8vyjeaanvpytg79xqzymqy5acmqgyxuyr","amount":{"quantity":100000000,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":187633,"unit":"lovelace"},"estimated_min":{"quantity":169021,"unit":"lovelace"},"minimum_coins":[{"quantity":999978,"unit":"lovelace"}]} real 0m0,029s user 0m0,006s sys 0m0,000s ``` Preprod (Alonzo): ```sh $ time curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/payment-fees \ > -d '{"payments":[{"address":"addr_test1qrtez7vn0d8xp495ggypmu2kyt7tt6qyva2spm0f5a3ewn0v474mcs4q8e9g55yknx3729kyg5dl69x5596ee9tvnynq7ffety","amount":{"quantity":1000000,"unit":"lovelace"}}]}' \ > -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":169021,"unit":"lovelace"},"estimated_min":{"quantity":169021,"unit":"lovelace"},"minimum_coins":[{"quantity":999978,"unit":"lovelace"}]} real 0m0,029s user 0m0,006s sys 0m0,000s ``` Preview (Babbage): ```sh $ time curl -X POST http://localhost:8090/v2/wallets/1f82e83772b7579fc0854bd13db6a9cce21ccd95/payment-fees -d '{"payments":[{"address":"addr_test1qrtez7vn0d8xp495ggypmu2kyt7tt6qyva2spm0f5a3ewn0v474mcs4q8e9g55yknx3729kyg5dl69x5596ee9tvnynq7ffety","amount":{"quantity":100000000,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"deposit":{"quantity":0,"unit":"lovelace"},"estimated_max":{"quantity":187809,"unit":"lovelace"},"estimated_min":{"quantity":169197,"unit":"lovelace"},"minimum_coins":[{"quantity":995610,"unit":"lovelace"}]} real 0m0,036s user 0m0,003s sys 0m0,003s ``` Co-authored-by: Jonathan Knowles <[email protected]>
Build failed:
|
This function simply encapsulates the Cardano API minimum UTxO function in a wallet-friendly interface, acceping and returning wallet-friendly types.
…nction. This function is now only used by `computeMinimumCoinForUTxOCardanoApi`. We can safely make it an inner function.
This function simply encapsulates the Cardano Ledger minimum UTxO function in a wallet-friendly interface, acceping and returning wallet-friendly types.
…quivalent. We eventually wish to switch from the Cardano API implementation of the minimum UTXO calculaion to the Cardano Ledger implementation. But before we do that, we test that the implementations are equivalent.
This commit switches from the Cardano API implementation of the minimum UTxO calculation to the Cardano Ledger implementation.
Now that we are using the Cardano Ledger implementation of the minimum UTxO calculation, we no longer need to treat the Babbage era specially in our public implementation.
These functions are no longer used.
Since our minimum UTxO calculation now relies on Cardano Ledger, we no longer need to convert from era-specific protocol parameters to the era-agnostic protocol parameters required by the Cardano API. Therefore, this property test (which checked that conversions would never cause run-time errors) is no longer necessary.
The juxtaposition of "UTxO" followed by another capitalised word is rather hard to read. This commit introduces underscores to aid readability.
…oLedger`. It's simpler for this property to accept a `TokenBundle` instead of a `TokenMap` and `Coin`. The `Arbitrary` instance for `TokenBundle` uses the `genTxOutTokenBundle` generator, which already has good coverage of minimum and maximum values specifically in the context of a transaction output. This allows us to get rid of the `Arbitrary` instance for `Coin` (which in hindsight would have been better to implement with the `genTxOutCoin` generator). In response to review feedback: #3456 (comment)
We can use the `Standard{Alonzo,Babbage}` synonyms for greater concision.
43a604a
to
3866692
Compare
I'm going to merge this manually, as:
|
…dp-2144/use-ledger-minimum-utxo-function
Issue Number
ADP-2144
Summary
This PR:
evaluateMinLovelaceOutput
to replace the use of Cardano API functioncalculateMinimumUTxO
in all implementation code, for all eras.calculateMinimumUTxO
for use as an oracle, to compare against when testing.{compute,isBelow}minimumCoinForUTxO
functions to explain their intended purpose more clearly.Context
evaluateMinLovelaceOutput
accepts an era-specific protocol parameters object and, unlike like the Cardano API, does not require that we convert to an era-agnostic protocol parameters object. Since values of theMinimumUTxO
type already contain era-specific protocol parameters, this means we can avoid the extra complication of record conversion whenever we call:isBelowMinimumCoinForUTxO
computeMinimumCoinForUTxO
Performance
Mainnet (Alonzo):
Preprod (Alonzo):
Preview (Babbage):