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

Closes #92: validates transactions relying on the Ledger.Validator api #93

Merged
merged 23 commits into from
Apr 21, 2022

Conversation

VictorCMiraldo
Copy link
Contributor

@VictorCMiraldo VictorCMiraldo commented Apr 14, 2022

This is a PR closes #92. The most important change is that we now rely on the validation exactly as performed by the cardano node, not just on the simplified Ledger.Index validation. Consequently, we needed a proper fee computation, which was mostly taken from plutus-apps.

Moreover, since empty currency symbols are not permitted anymore, we had to make a simple QuickCurrency minting policy for quick values.

There's still work to be done with respect to collateral, which I'll tackle before marking the PR as ready.

@VictorCMiraldo
Copy link
Contributor Author

This PR is in a good shape. It just needs to provide txCollateral; the best option is likely to be initializing the test wallets with a number of outputs instead of just one, so we always have some collateral to give.

@VictorCMiraldo VictorCMiraldo changed the title Start tacking issue 92 Closes #92: validates transactions relying on the Ledger.Validator api Apr 14, 2022
@VictorCMiraldo VictorCMiraldo marked this pull request as ready for review April 16, 2022 10:41
@VictorCMiraldo
Copy link
Contributor Author

@sjoerdvisscher , if you have some time to look at this it would be greatly appreciated; the only important place to look would be the Cooked.MockChain.Monad.Direct module, where most of the hard work was done. No need to look at the rest.

Copy link
Contributor

@GuillaumeGen GuillaumeGen left a comment

Choose a reason for hiding this comment

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

The mechanisms to deal with signatures and fees seem crazy, but apparently we have no choice.

cooked-validators/src/Cooked/MockChain/Wallet.hs Outdated Show resolved Hide resolved
cooked-validators/src/Cooked/MockChain/Monad/Direct.hs Outdated Show resolved Hide resolved
@sjoerdvisscher
Copy link
Member

@GuillaumeGen The requiredSigners situation will improve in the future when we complete the switch from our own Tx data type to the cardano-ledger one. The fee calculation is what it is, it was design with security and network stability in mind, not ease of use...

@GuillaumeGen
Copy link
Contributor

@sjoerdvisscher It is great to learn that the handling of signature will be simplified in the future.
Regarding the fee calculation, I have no doubt that setting it in order both to make Cardano a cheap blockchain and to guarantee the stability of the network is an hard enough task not try additionally to make it easy to compute manually. However I did not grasp yet why it is required to do a fixpoint computation and Plutus cannot provide a function computeMinimalFees :: Tx -> Value (or equivalently setMinimalFees :: Tx -> Tx) which given a transaction with an irrelevant value in the fee field outputs the minimal fee amount for this transaction to validate. (or the same transaction with the fee field updated).
If you could elaborate on this, I would be super interested.

@sjoerdvisscher
Copy link
Member

@GuillaumeGen The fee depends on the amount of inputs and outputs, so it depends on balancing, and since you do your own balancing you can just call a function to calculate the fees. Obviously balancing also depends on the fees in turn, hence the need for a fixed point. And then customers reported that the fixed point was not always reached, a bigger fee can cause a bigger input to be chosen during balancing, leading to fewer inputs, leading to lower fees, cycling ad infinitum. So it's annoyingly complicated.

cooked-validators/src/Cooked/Currencies.hs Show resolved Hide resolved
cooked-validators/src/Cooked/MockChain/Monad/Direct.hs Outdated Show resolved Hide resolved
cooked-validators/tests/Cooked/QuickValueSpec.hs Outdated Show resolved Hide resolved
cooked-validators/src/Cooked/MockChain/Wallet.hs Outdated Show resolved Hide resolved
cooked-validators/src/Cooked/Tx/Constraints/Type.hs Outdated Show resolved Hide resolved
cooked-validators/tests/Cooked/BalanceSpec.hs Outdated Show resolved Hide resolved
Copy link
Member

@florentc florentc left a comment

Choose a reason for hiding this comment

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

A lot of side improvements besides the main matters of the hope-for-a-fixpoint fee calculation/balancing daunting matter and support for collaterals and signatures the right way. Still not sure I understand why we start with high fees and switch to little fees if that's not possible. Is it just an empirical optimization hack that tries to avoid many iterations if possible or is there more to it in terms of correctness?
I have no use case in mind for the True minting policy but I conceptually like a lot how the new Currency module distinguishes between quick and permanent (old quicks) values. Some old code for pretty printing old quick values is no longer relevant. I left a more detailed comment.

@@ -5,7 +5,9 @@

module Cooked.MockChain.Wallet where

import qualified Cardano.Api as C
Copy link
Member

Choose a reason for hiding this comment

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

I think the qualified names in the module are becoming confusing. Maybe we could refactor:

  • avoiding 1-2 letters names apart from Pl, including M which could be imported as Map along with a non qualified import for the type Map
  • making it clear what comes from Plutus and what comes from Cardano.
    There are two Crypto modules here, one from Plutus, one from Cardano.
  • being consistent whether we name the submodule (e.g. Validation/Crypto/CW), give them the lib name (Pl/Cardano), or both (e.g. PlCrypto/CardanoCrypto).

cooked-validators/src/Cooked/MockChain/Wallet.hs Outdated Show resolved Hide resolved
@VictorCMiraldo VictorCMiraldo merged commit 13e9d57 into main Apr 21, 2022
@VictorCMiraldo VictorCMiraldo deleted the vcm/issue-92 branch April 21, 2022 12:19
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.

Rely on Ledger.Validation instead of Ledger.Index for more faithful transaction validation
6 participants