Vesting use case succeeds even if validator is set to True

[locallycompact]

Summary

applying this diff

@@ -131,21 +131,7 @@ remainingFrom t@VestingTranche{vestingTrancheAmount} range =
 {-# INLINABLE validate #-}
 validate :: VestingParams -> () -> () -> V2.ScriptContext -> Bool
 validate VestingParams{vestingTranche1, vestingTranche2, vestingOwner} () () [email protected]{scriptContextTxInfo=txInfo@TxInfo{txInfoValidRange}} =
-    let
-        remainingActual  = V2.valueLockedBy txInfo (V2.ownHash ctx)
-
-        remainingExpected =
-            remainingFrom vestingTranche1 txInfoValidRange
-            + remainingFrom vestingTranche2 txInfoValidRange
-
-    in remainingActual `Value.geq` remainingExpected
-            -- The policy encoded in this contract
-            -- is "vestingOwner can do with the funds what they want" (as opposed
-            -- to "the funds must be paid to vestingOwner"). This is enforcey by
-            -- the following condition:
-            && V2.txSignedBy txInfo (unPaymentPubKeyHash vestingOwner)
-            -- That way the recipient of the funds can pay them to whatever address they
-            -- please, potentially saving one transaction.
+       True

 vestingScript :: VestingParams -> Validator
 vestingScript = Scripts.validatorScript . typedValidator

still makes all the tests for this use case pass

Steps to reproduce the behavior

apply the diff

Actual Result

doesn't work

Expected Result

should work

Describe the approach you would take to fix this

No response

System info

nixos

[locallycompact]

Can anyone shed any light on this? It's very confusing.

[nielstron]

Are there any negative test cases for this contract i.e. cases where the contract is expected to fail?

[berewt]

I had a look at it. The Vesting test aren't unit test about the validators, but integration tests that test both the OffChain and the OnChain code at the same time. The retrieve endpoint do an off chain of the avaible founds before we try and submit the Tx, so the too permissive enchain code isn't checked in the end.

We should have add a test on the enchain code itself though, you're right.

[locallycompact]

A negative test case would not be a test of the on-chain code directly but an adversarial action that expects the model not to change.

[berewt]

Both are valid test scenarios actually, you want to check the behaviour of the onchain code and to protect yourself against malicious transaction.

[locallycompact]

I really don't see how. The validator only makes sense in the context you intend it to form a functioning contract. A validator by itself is not correct or incorrect, it just is what it is.

[berewt]

The validator itself is a function, you can use test to specify it and validates that it behaves as specified by the tests.
You can then check if this specification is correct in the context of a functioning contract by providing adversarial attacks.

Your validator can respect the specification and the specification can be too weak to ensure properties of a functioning contract, or it can be what is needed.
Checking both allows you to know if the validator is incorrect or if your specifications are too weak.

[locallycompact]

The validator is just a content-addressed predicate. The same validator in different contexts can be used for completely different behaviour. There is nothing to say by testing the validator in isolation except to say that it is that predicate. Everything that is testable is defined by what actions you perform and what should the result be.

[berewt]

We're derailing out of the scope of the issue and it's probably the best place to talk about It (not that I wouldn't like to, but I prefer to keep the comments focused on the initial issue). The issue is tracked. We'll see when and how we'll be able to address it.