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

ADP-1183 (Part 2): Balancing Plutus transactions #2967

Merged
merged 28 commits into from
Oct 15, 2021

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Oct 11, 2021

  • 📍 Add core type 'Redeemer' and compat functions with Cardano ledger.
    Certificates have been left out currently, to be done in a later commit / iteration.

  • 📍 Add util 'modifyM' function for state-monad.

  • 📍 Expose constructor for TimeInterpreter
    Will be needed in order to evaluate Plutus scripts and assign execution units to redeemers.
    I originally wanted to expose only record accessors, but this isn't working for the 'interpreter' field because of the existential; eras would escape their scope. Hence the full constructor being exposed. Alternatively, we could expose the 'EpochInfo' from this module instead to keep the module's API somewhat sane.

  • 📍 Extend 'TransactionLayer' with 'assignScriptRedeemers'
    The goal is to move and replace the bunch of logic recently added into 'updateTx' to keep the latter only about adding inputs/outputs, and the former about handling redeemers. This makes it easier to follow and test down the line.

  • 📍 Implement 'assignScriptRedeemers' for Cardano.

  • 📍 Remove newExUnits from TxUpdate, moved as separate function.
    This was a quick-n-dirty hack to get started with the integration and testing. Adding witnesses is actually quite more involved in the end and it is therefore better done in a, far from trivial, separate function.

  • 📍 Remove 'certifying' type of redeemers in the API
    I've mistakenly messed up when introducing this one originally an the type is plain wrong (it's Byron's certificate where it should be Shelley's). For now, the focus is on getting the balancing endpoint to work, and the case of redeemers for certificates is quite an edge-case, so it can be addressed in subsequent PRs.

  • 📍 Wire updateTx and assignScriptRedeemers errors into the API.

  • 📍 Move 'mapFirst' local function to Cardano.Wallet.Util
    For re-use across other modules.

  • 📍 Add txOutAddCoin function to primitives for transaction.

  • 📍 Finalize implementation of 'balanceTransaction'
    This should now work properly for any redeemers and, with proper execution units & fees. Not really heavily tested at this stage, but it's coming.

  • 📍 Make fee padding more robust to number (and content) of redeemers.

  • 📍 Also provide datum with external inputs.
    It is actually needed to lookup redeemer pointers of spending redeemers.

  • 📍 Resolve additional issues with execution units and clean up code.

  • 📍 Improve error reporting for redeemers error in the balancing endpoint.

  • 📍 Add new plutus scenario: GameStateMachine.
    Currently fails, because this scenario requires the wallet to handle minting/burning accordingly.

  • 📍 Support balancing minted and burnt tokens in pre-constructed transactions.

  • 📍 Enable 3rd step of the GameStateMachine contract. Currently fails.

  • 📍 Also provide input resolution for own inputs when evaluating scripts.
    Doh. This was a tough one. Turns out that, the 'UTXO' given to 'evaluateTransactionExecutionUnits' MUST contain ALL inputs referenced in the transaction. If some are missing, it silently ignore them when constructing the script context for the underlying validator causing the script to either fail, or evaluate with a wrong context. I raised the issue with the ledger team as I firmly believe the 'evaluateTransactionExecutionUnits' ought to fail loudly when the UTXO given to it is incomplete (easily verifiable).

  • 📍 Move epochInfo creation to TimeInterpreter module
    To create the 'EpochInfo' needed to evaluate scripts, I had to expose the constructor of TimeInterpreter, making the abstraction leaky. That's not good, especially when it is quite easy to keep the abstraction boundary closed by providing ways of doing the conversions from the module itself.

  • 📍 Move back extra outputs at the end
    For rapid testing, I originally change the way extra outputs (i.e. the change output) were added to the transaction to match the same convention used by the PAB. That is, the PAB have them by default as the first output of every transaction, and contract traces are built with this assumption in mind.
    However, having them at the beginning is really not handy, since there may be in practice more than one change output! It is thus easier to append them to existing outputs, and have contracts works from a 0-based index instead.

  • 📍 Remove script evaluation scaling
    I believe the issue was due to the missing internal inputs not present in the UTXO given to the evaluator. So, fixed in 1530e19

  • 📍 Add a 3rd Plutus end-to-end scenario
    This 3rd scenarios exercises two components of the transaction workflow: minting and burning of tokens (automatically detected from the transaction context), as well as, required signatures from a transaction (field 14), that are necessary here to execute the minting / burning policy. The policy itself is a template which requires a verification key hash and validates if the minting/burning transaction is signed by the sk corresponding to that key hash. When executing the scenario, the vk is pulled from the wallet known keys, and added as required signers to the transaction so that the wallet produces a signature for that key regardless of whether it is needed for an input.

  • 📍 Add some extra notes after self-review.

  • 📍 Add a new function to integration tests' context to inject rewards into a script address.

  • 📍 Use 'StakeAddress' instead of 'RewardAccount' for withdrawal redeemers
    RewardAccount are associated with stake keys in all boundary interfaces; that is, they are serialised and parsed as type 08 addresses, whereas they could in principle be type 09 (and this is precisely what withdrawal e2e plutus scenario is testing. The wallet codebase does not have a good abstraction for representing stake addresses in general, but cardano-api does, and it's quite handy since it's merely a wrapper around the cardano-ledger-specs' types, so easily re-used in 'assignNullRedeemers'. It also has existing conversions from and to bech32, so it is actually a low hanging fruit.

  • 📍 Add 4th e2e Plutus scenario for trying out withdrawal from script address.

  • 📍 Regenerate nix

Comments

N/A

Issue Number

ADP-1183

@KtorZ KtorZ self-assigned this Oct 11, 2021
TimeInterpreter{interpreter,blockchainStartTime} -> do
let systemStart = toSystemStart blockchainStartTime
epochInfo <- lift (interpreterToEpochInfo <$> interpreter)
pure (systemStart, epochInfo)
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: This could be moved to the Slotting module, to avoid leaking TimeInterpreter abstraction's internals.

@KtorZ KtorZ force-pushed the KtorZ/ADP-1183/balancing-real-execution-units branch from cb02011 to 48aa861 Compare October 12, 2021 09:28
@KtorZ KtorZ marked this pull request as ready for review October 12, 2021 18:45
@KtorZ KtorZ force-pushed the KtorZ/ADP-1183/balancing-real-execution-units branch 3 times, most recently from f16132c to ecfdb7d Compare October 14, 2021 12:06
KtorZ added 23 commits October 14, 2021 18:10
  Certificates have been left out currently, to be done in a later commit / iteration.
  Will be needed in order to evaluate Plutus scripts and assign execution units to redeemers.
  I originally wanted to expose only record accessors, but this isn't working for the 'interpreter' field because of the existential; eras would escape their scope. Hence the full constructor being exposed. Alternatively, we could expose the 'EpochInfo' from this module instead to keep the module's API somewhat sane.
  The goal is to move and replace the bunch of logic recently added into 'updateTx' to keep the latter only about adding inputs/outputs, and the former about handling redeemers. This makes it easier to follow and test down the line.
  This was a quick-n-dirty hack to get started with the integration and testing. Adding witnesses is actually quite more involved in the end and it is therefore better done in a, far from trivial, separate function.
  I've mistakenly messed up when introducing this one originally an the type is plain wrong (it's Byron's certificate where it should be Shelley's). For now, the focus is on getting the balancing endpoint to work, and the case of redeemers for certificates is quite an edge-case, so it can be addressed in subsequent PRs.
  This should now work properly for any redeemers and, with proper execution units & fees. Not really heavily tested at this stage, but it's coming.
  It is actually needed to lookup redeemer pointers of spending redeemers.
  Currently fails, because this scenario requires the wallet to handle minting/burning accordingly.
  Doh. This was a tough one. Turns out that, the 'UTXO' given to 'evaluateTransactionExecutionUnits' MUST contain ALL inputs referenced in the transaction. If some are missing, it silently ignore them when constructing the script context for the underlying validator causing the script to either fail, or evaluate with a wrong context. I raised the issue with the ledger team as I firmly believe the 'evaluateTransactionExecutionUnits' ought to fail loudly when the UTXO given to it is incomplete (easily verifiable).
  To create the 'EpochInfo' needed to evaluate scripts, I had to expose the constructor of `TimeInterpreter`, making the abstraction leaky. That's not good, especially when it is quite easy to keep the abstraction boundary closed by providing ways of doing the conversions from the module itself.
  For rapid testing, I originally change the way extra outputs (i.e. the change output) were added to the transaction to match the same convention used by the PAB. That is, the PAB have them by default as the first output of every transaction, and contract traces are built with this assumption in mind.
  However, having them at the beginning is really not handy, since there may be in practice more than one change output! It is thus easier to append them to existing outputs, and have contracts works from a 0-based index instead.
  I believe the issue was due to the missing internal inputs not present in the UTXO given to the evaluator. So, fixed in 1530e19
  This 3rd scenarios exercises two components of the transaction workflow: minting and burning of tokens (automatically detected from the transaction context), as well as, required signatures from a transaction (field #14), that are necessary here to execute the minting / burning policy. The policy itself is a template which requires a verification key hash and validates if the minting/burning transaction is signed by the sk corresponding to that key hash. When executing the scenario, the vk is pulled from the wallet known keys, and added as required signers to the transaction so that the wallet produces a signature for that key regardless of whether it is needed for an input.
@KtorZ KtorZ force-pushed the KtorZ/ADP-1183/balancing-real-execution-units branch from ecfdb7d to e6fd97a Compare October 14, 2021 16:10
@KtorZ
Copy link
Member Author

KtorZ commented Oct 14, 2021

bors try

iohk-bors bot added a commit that referenced this pull request Oct 14, 2021
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 14, 2021

try

Build failed:

@KtorZ KtorZ force-pushed the KtorZ/ADP-1183/balancing-real-execution-units branch from e6fd97a to 1d336e6 Compare October 15, 2021 17:06
@KtorZ
Copy link
Member Author

KtorZ commented Oct 15, 2021

bors try

iohk-bors bot added a commit that referenced this pull request Oct 15, 2021
@KtorZ
Copy link
Member Author

KtorZ commented Oct 15, 2021

bors r-

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 15, 2021

try

Build failed:

@KtorZ KtorZ force-pushed the KtorZ/ADP-1183/balancing-real-execution-units branch 3 times, most recently from 398b736 to fc86190 Compare October 15, 2021 18:19
@KtorZ
Copy link
Member Author

KtorZ commented Oct 15, 2021

bors try

iohk-bors bot added a commit that referenced this pull request Oct 15, 2021
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 15, 2021

try

Build failed:

KtorZ and others added 4 commits October 15, 2021 20:55
  RewardAccount are associated with stake keys in all boundary interfaces; that is, they are serialised and parsed as type 08 addresses, whereas they could in principle be type 09 (and this is precisely what withdrawal e2e plutus scenario is testing. The wallet codebase does not have a good abstraction for representing stake addresses in general, but cardano-api does, and it's quite handy since it's merely a wrapper around the cardano-ledger-specs' types, so easily re-used in 'assignNullRedeemers'. It also has existing conversions from and to bech32, so it is actually a low hanging fruit.
@KtorZ KtorZ force-pushed the KtorZ/ADP-1183/balancing-real-execution-units branch from fc86190 to 158cde1 Compare October 15, 2021 19:13
@KtorZ
Copy link
Member Author

KtorZ commented Oct 15, 2021

bors try

iohk-bors bot added a commit that referenced this pull request Oct 15, 2021
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 15, 2021

try

Build succeeded:

@KtorZ KtorZ merged commit 54326ed into master Oct 15, 2021
@KtorZ KtorZ deleted the KtorZ/ADP-1183/balancing-real-execution-units branch October 15, 2021 20:42
WilliamKingNoel-Bot pushed a commit that referenced this pull request Oct 15, 2021
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.

1 participant