RealMath.fromReal truncates, and would be more usable if it rounded

[clexmond]

The following fails with RealMath.fromReal(hundred) being 99. And in fact the result is always off by int(1) (i.e. 4000 * 2/40 = 199).

function testRealMath() public {
    var exp = RealMath.div(RealMath.toReal(1), RealMath.toReal(40));
    var hundred = RealMath.mul(exp, RealMath.toReal(4000));

    Assert.equal(RealMath.fromReal(hundred), 100, "Should equal 100");
}

[interfect]

I think this might be "expected" behavior.

1/40 is a repeating decimal when expressed in binary, and the math library uses a binary fixed point representation. So the value in exp is necessarily truncated, and is ever so slightly less than 1/40.

Then when you multiply by 4000, you get a result that is 99.999something, and then fromReal truncates the fractional part when converting, just like a C cast of double to int, because (IIRC) it just integer divides by REAL_ONE.

The end result is of course not what any normal human would expect. I think the right solution might be to make fromReal round to the nearest integer, rather than truncate. Alternatively, everyone could remember to call a round function before converting back to an integer.

[clexmond]

That makes sense, and calling round prior to calling fromReal is a good workaround in the meantime. Are you open to contributions if I make the change?

[interfect]

Yes, sure, go ahead.

Just make sure to have a look at the LICENSE file before contributing. It's not quite an MIT license; it has an extra condition that I put in there to basically prohibit competing mainnet deployments of the entire Macroverse system as a whole. It's not really particularly relevant to just the math library, but you should have a look to see if you like it before you contribute code under it.

On Thu, Jan 25, 2018 at 7:13 PM, Chris Lexmond [email protected] wrote:

That makes sense, and calling round prior to calling fromReal is a good workaround in the meantime. Are you open to contributions if I make the change?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/NovakDistributed/macroverse/issues/2#issuecomment-360672776, or mute the thread https://github.com/notifications/unsubscribe-auth/AAt5tpprmM2Px5HV-lkam45xbaAQuqJnks5tOUL0gaJpZM4RtZb- .

[clexmond]

Not a problem, I gave it a look before starting to use the RNG and RealMath libs and noticed they didn't carry the AccessControl restrictions. Would you ever consider breaking those out into a different project with a clean MIT license?

[interfect]

I could license those files under MIT. I don't know if it would make sense to break those files out into their own repo/module; did you want to pull them in as a dependency for another project?

On Jan 26, 2018 06:25, "Chris Lexmond" [email protected] wrote:

Not a problem, I gave it a look before starting to use the RNG and RealMath libs and noticed they didn't carry the AccessControl restrictions. Would you ever consider breaking those out into a different project with a clean MIT license?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/NovakDistributed/macroverse/issues/2#issuecomment-360798086, or mute the thread https://github.com/notifications/unsubscribe-auth/AAt5tk-Cr6Hd2BEDmUrvoHvkcJZDTUj8ks5tOeBfgaJpZM4RtZb- .

[interfect]

I've updated the LICENSE to say that contracts/RNG.sol and contracts/RealMath.sol are MIT-licensed.

If you want to just copy-paste the files you are welcome to. If you want them published as a separate NPM module, open another issue for that.

I'm going to retitle this issue to be more on topic.

[clexmond]

That works great, thank you.