Delegate Rational invariant to PNonZero + optimizations

[TotallyNotChase]

The "fail on zero" invariant should be forced in the PlutusType instance - otherwise pcon is a trap for users.

The golden changes are purely within dev mode and only exist due to a slight change in the error trace.

[Geometer1729]

LGTM

[L-as]

I'm not sure this is the best solution. Shouldn't we just not export the constructors, or otherwise make the divisor (or dividend?) a Positive?

[TotallyNotChase]

I think pcon is a pretty good notion of smart constructor that already exists. Unless we require pcon to never error - I don't see a reason to not utilize it for this purpose.

We could indeed just not export the constructor but then the PlutusType instance becomes a bit useless - unless you go on to introduce "safe" helper functions to help with pmatch's handler and pcon's argument, which seems like unnecessary extra work.

[L-as]

The PlutusType instance would still be used internally.

in general, in Haskell, people expect constructors to be total. I don't think we should add partial constructors.

[TotallyNotChase]

Not wanting pcon to be partial is a valid concern - though I don't know how applicable it is for Plutarch usecases. Do you think we should unexport the constructor and just have 2 functions for building them (one returning PMaybe, the other being partial)?

And for deconstruction, I suppose the pnumerator and pdenominator will do? I feel like this might be a blocker for achieving maximal performance in userland (using pmatch directly in a utility function that is then hoisted will be more efficient) - but oh well.

[L-as]

Well that's why I suggested Positive.

[TotallyNotChase]

I assume you mean NonZero? Hmm, I could try that and see what it entails.

[L-as]

Shouldn't it be Positive and not NonZero? I don't see why it would be negative.

[TotallyNotChase]

Shouldn't it be Positive and not NonZero? I don't see why it would be negative.

The original invariant was NonZero so far so I preserved that behavior. I don't see anything technically wrong with allowing negative denominators but we can definitely change it to Positive if necessary.

[L-as]

We need to find a general solution to this.

[TotallyNotChase]

I removed the constructor export and kept the PlutusType instance "normal" (no forced invariant check), and added supplementary functions for construction and deconstruction

[L-as]

@TotallyNotChase Can you make this PR up to date?

[TotallyNotChase]

FYI this isn't ready yet - just syncing with staging.

[TotallyNotChase]

I'm trying to figure out optimization stops - where invariant checks should be skipped.

[L-as]

Just put them in everywhere, then we'll make it efficient later.

[TotallyNotChase]

Alright, will do optimizations later then

[TotallyNotChase]

Hmm, why did the PNonZero -> PPositive change make the benchmarks perform worse? There wasn't really any changes other than #== becoming #<= - should investigate.

[TotallyNotChase]

Hmm, why did the PNonZero -> PPositive change make the benchmarks perform worse? There wasn't really any changes other than #== becoming #<= - should investigate.

looks like the benchmarks are performed on scripts compiled with tracing mode on - that's not correct.

[L-as]

We mostly just need TODOs, great work, thanks!

[L-as]

LGTM