Skip to content
This repository was archived by the owner on Dec 2, 2024. It is now read-only.

PLT-670 Add MustValidateIn to tx constraints#665

Merged
berewt merged 14 commits intoIntersectMBO:mainfrom
berewt:PLT-670-add--MustValidateIn-to-tx-constraints
Aug 23, 2022
Merged

PLT-670 Add MustValidateIn to tx constraints#665
berewt merged 14 commits intoIntersectMBO:mainfrom
berewt:PLT-670-add--MustValidateIn-to-tx-constraints

Conversation

@berewt
Copy link
Contributor

@berewt berewt commented Aug 17, 2022

[WIP] The tests part is still missing but the validityRanges are now directly store in the Tx, not in an extra field.

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@koslambrou koslambrou self-requested a review August 17, 2022 13:46
@koslambrou
Copy link
Contributor

For this PR, I would recreate the same tests in Spec.TxConstraints.TimeValidity, but for plutus-tx-constraints instead of plutus-ledger-constraints (keep the old one until we finally remove it).

@berewt berewt changed the title [PLT-670] Add MustValidateIn to tx constraints PLT-670 Add MustValidateIn to tx constraints Aug 18, 2022
@berewt berewt marked this pull request as draft August 19, 2022 09:18
@berewt
Copy link
Contributor Author

berewt commented Aug 19, 2022

Got a failure I'm unable to explain in the protocolV5Cardano test.
The transaction pass despite the validityRange, not sure if it's normal due to how we build the transaction in Tx.Constraints or not.

@sjoerdvisscher sjoerdvisscher self-requested a review August 19, 2022 11:00
@berewt
Copy link
Contributor Author

berewt commented Aug 22, 2022

protocolV5 test keeps failing with Tx.Constraints. I wonder if it can come from the way we test in the two cases: we check that the transaction is validated within its validity range in the Tx.Constraints case, while we are checking the validity range that is stored in ScriptContext in the initial case. Unfortunately, I can't use the validator as not enough Tx.Constraints are available on main.

@sjoerdvisscher
Copy link
Contributor

I'm fine with removing the V5 test, as long as we have a good negative test that shows the positive test doesn't succeed by accident.

@berewt
Copy link
Contributor Author

berewt commented Aug 22, 2022

It leads to the other issue with the PR at this stage: the failing test defines an unreachable range for a Tx and checks that the Tx isn't validated after some slot. Unfortunately, the transaction isn't failing either, it's just waiting for a valid slot that doesn't (and can't) come.
I think it's ok, but maybe we (I) should open an issue to investigate whether this behavior should be fixed.

@berewt berewt marked this pull request as ready for review August 22, 2022 10:45
@berewt berewt requested review from sjoerdvisscher and removed request for koslambrou August 22, 2022 11:15
@berewt berewt requested a review from sjoerdvisscher August 22, 2022 12:28
Copy link
Contributor

@sjoerdvisscher sjoerdvisscher left a comment

Choose a reason for hiding this comment

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

Thanks, that makes a lot of sense with the explanation!

@berewt berewt merged commit 749f231 into IntersectMBO:main Aug 23, 2022
@berewt berewt deleted the PLT-670-add--MustValidateIn-to-tx-constraints branch August 23, 2022 14:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants