Use sensible maths for from_rational

[gavofyork]

Old maths had all sorts of small edge cases with rounding which broke expectations. Now we just use the stuff which is already proven.

Note one test in staking was relying on the old buggy maths. It had to be corrected with some dirty magic numbers. Not sure if @kianenigma wants to take a look. A test was added in per_things (from_rational_with_rounding_breakage_2) to demonstrate that the old numbers for which this tests behaviour relied on are in fact wrong and do definitely break expectations.

Possibly fix for paritytech/polkadot-sdk#200

[gavofyork]

Division by zero should panic if there is no way to return an Err. It panics in regular arithmetic. Returning some finite, strictly incorrect, value could mask major errors (tests would no longer panic) and lead to unintended consequences.

It's reasonable to introduce alternative versions of these functions which return Result and thus need not panic for division-by-zero. If we really want to make the infallible functions not panic on division by zero, I would at least use defensive! to ensure the error does not go unnoticed.

[ggwpez]

Division by zero should panic if there is no way to return an Err. It panics in regular arithmetic. Returning some finite, strictly incorrect, value could mask major errors (tests would no longer panic) and lead to unintended consequences.

It's reasonable to introduce alternative versions of these functions which return Result and thus need not panic for division-by-zero. If we really want to make the infallible functions not panic on division by zero, I would at least use defensive! to ensure the error does not go unnoticed.

Yea the case that it wont show up in tests anymore is indeed bad. So having Result functions is probably the only proper way since the code in sp-* cannot use the defensive! from frame-support. Should I create an issue for it?

[gavofyork]

Division by zero should panic if there is no way to return an Err. It panics in regular arithmetic. Returning some finite, strictly incorrect, value could mask major errors (tests would no longer panic) and lead to unintended consequences.
It's reasonable to introduce alternative versions of these functions which return Result and thus need not panic for division-by-zero. If we really want to make the infallible functions not panic on division by zero, I would at least use defensive! to ensure the error does not go unnoticed.

Yea the case that it wont show up in tests anymore is indeed bad. So having Result functions is probably the only proper way since the code in sp-* cannot use the defensive! from frame-support. Should I create an issue for it?

Yes, think so.

[ggwpez]

% the comment on multiply_rational.
Still need to figure out how to run these tests in the CI.

[ruseinov]

the logic looks good to me, however we should probably fix the staking test to actually reflect this properly and not via magic numbers.

[ruseinov]

approving this as fixing the staking test properly could be done as part of another issue

[gavofyork]

bot merge