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

Adjust chain-following code to account for collateral inputs #2856

Merged
merged 32 commits into from
Sep 9, 2021

Conversation

sevanspowell
Copy link
Contributor

@sevanspowell sevanspowell commented Aug 27, 2021

The purpose of this PR is to modify the the code that follows the blockchain to adjust the Wallet's UTxO when collateral inputs are lost due to a transaction that fails phase-2 script validation (Plutus scripts).

I have:

  • Add valid script field to MkApiTransactionParams, ApiTransaction, TxMeta, Tx, and TransactionInfo.
  • Modify state transition logic (when a Tx is applied to a UTxO) to account for collateral inputs.
  • Add integration test for collateral inputs.
  • Add property tests for state transition logic.
  • Add is_valid_script field to API specification.

Issue Number

ADP-1036

@sevanspowell sevanspowell self-assigned this Aug 27, 2021
@sevanspowell sevanspowell force-pushed the sevanspowell/adp-1036/collateral-chain-following branch 5 times, most recently from e13795b to 10958ce Compare September 2, 2021 03:23
Base automatically changed from sevanspowell/adp-1092/apply-tx-testable to master September 3, 2021 10:07
@sevanspowell sevanspowell force-pushed the sevanspowell/adp-1036/collateral-chain-following branch 2 times, most recently from 062528a to f7d9dd8 Compare September 6, 2021 07:35
lib/core/src/Cardano/Wallet/DB/Sqlite.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/DB/Sqlite/TH.hs Show resolved Hide resolved
lib/core/src/Cardano/Wallet/DB/Sqlite/Types.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/DB/Sqlite/Types.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/DB/Sqlite/Types.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/Primitive/Model.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/Primitive/Model.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

@sevanspowell and I had a discussion about the representation of a transaction's script validity (currently represented by the Tx.txScriptValidity :: TxScriptValidity field).

After consideration, we both feel that it doesn't make sense to talk about the script validation status of a transaction that is still in the pending set: after all, transactions in this state have not yet been subjected to phase-2 validation, so we can't yet know what the outcome will be.

Therefore, we propose to:

  • Change the type of Tx.txScriptValidity to Maybe TxScriptValidity.
    (This makes it possible to assign pending transactions with a script validity of Nothing.)
  • Change the type of TxInfo.txInfoScriptValidity to Maybe TxScriptValidity.
  • Change the definition of TxScriptValidity to:
    data TxScriptValidity
        = TxScriptValid
        | TxScriptInvalid
  • Change the API definition of txScriptValidity to:
    x-txScriptValidity: &txScriptValidity
      type: string
      enum:
        - valid
        - invalid

The absence of a validity means that either:

  • script validity is not supported for this type of transaction (for example, if the transaction was created in an era prior to Alonzo); or
  • this transaction has not yet been subjected to script validation.

We both feel that this representation is still not completely satisfactory. Even under the revised representation, the Tx type still make it possible to represent a pending transaction that has a script validity, which of course doesn't make sense.

Ideally, we'd want to represent pending transactions with a type that doesn't have a script validity field, so it would not be possible to represent this invalid state.

However, the Tx type is deeply embedded in multiple places, and replacing this with a PendingTx type would likely be a lot of work, and almost certainly cause us to exceed our time budget for this task. So we propose to:

  • Create an item of technical debt: represent pending transactions with a type that doesn't include script validity.

@sevanspowell
Copy link
Contributor Author

sevanspowell commented Sep 7, 2021

https://input-output.atlassian.net/browse/ADP-1116

Technical debt ticket created 👍

@sevanspowell
Copy link
Contributor Author

sevanspowell commented Sep 7, 2021

@sevanspowell and I had a discussion about the representation of a transaction's script validity (currently represented by the Tx.txScriptValidity :: TxScriptValidity field).

...

3997bf9

@sevanspowell sevanspowell force-pushed the sevanspowell/adp-1036/collateral-chain-following branch from 755da3b to 097046a Compare September 7, 2021 06:37
@sevanspowell sevanspowell force-pushed the sevanspowell/adp-1036/collateral-chain-following branch from 25ee052 to e562bc1 Compare September 9, 2021 00:12
sevanspowell and others added 7 commits September 9, 2021 11:44
In this commit we:

- Remove variable indentation, occasionally using named variables to help.
- Wrap expressions that are significantly longer than the line limit.
- Use standard formatting for `where` clauses.
- Remove extraneous blank lines.
Use this function to simplify `changeUTxO`.
Here we don't need to manually use `forAllShrink`, because we're just
using the standard generators and shrinkers without modification. We can
save a bit of boilerplate by using `Arbitrary` instances.
The `failedScriptValidation` function is currently always called in a
context where the transaction itself is available.

Therefore, we can save callers the trouble of manually lensing into the
`scriptValidity` field, and move this lensing into the
`failedScriptValidation` function itself.
…collateral-chain-following

Additions to PR 2856
…ip-test-api-tx-validity

Add roundtrip JSON encoding test for `ApiT TxScriptValidity`
@jonathanknowles jonathanknowles self-requested a review September 9, 2021 05:15
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏻

@sevanspowell thanks again for all your hard work on this!

@jonathanknowles
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 9, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 4d47339 into master Sep 9, 2021
@iohk-bors iohk-bors bot deleted the sevanspowell/adp-1036/collateral-chain-following branch September 9, 2021 06:23
WilliamKingNoel-Bot pushed a commit that referenced this pull request Sep 9, 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.

2 participants