Serialization of Infinity is null

[mttrbrts]

Describe the bug
When an expression sets a value in the state to Infinity, this gets serialized (through JSON.stringify?) to null. This has the effect of invalidating the state instance and rendering the contract unable to accept new requests.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Description

The numbers Infinity and NaN, as well as the value null, are all considered null.

To Reproduce
Steps to reproduce the behavior:
infinity.ergo

namespace matt.inf

import org.accordproject.cicero.contract.*
import org.accordproject.cicero.runtime.*

contract Inf over InfContract {
    clause infTest(request: Request): Response {
        set state InfContractState {
            stateId: "inf0",
            value: 1.0 / 0.0
        };
        return Response{}
    }
}

model.cto

namespace matt.inf

import org.accordproject.cicero.contract.* from https://models.accordproject.org/cicero/contract.cto
import org.accordproject.cicero.runtime.* from https://models.accordproject.org/cicero/runtime.cto

asset InfContractState extends AccordContractState {
    o Double value
}

asset InfContract extends AccordContract {}

state.json

{
    "$class": "matt.inf.InfContractState",
    "stateId": "state1",
    "value": 0.0
}

contract.json

{
    "$class": "matt.inf.InfContract",
    "contractId": "contract1"
}

request.json

{
    "$class": "org.accordproject.cicero.runtime.Request"
}

When you run ...

mattmbp:minimal matt$ ergorun execute --contract contract.json --request request.json --state state.json  model.cto infinity.ergo
09:05:31 - info:
{
  "clause": "mattXinfXInf",
  "request": {
    "$class": "org.accordproject.cicero.runtime.Request",
    "transactionId": "54280e38-bfda-43f1-98d1-9048a64d7aeb",
    "timestamp": "2019-08-16T08:05:31.493Z"
  },
  "response": {
    "$class": "org.accordproject.cicero.runtime.Response",
    "transactionId": "994be5fe-39b9-451e-9aa1-5e6a0fc90e3e",
    "timestamp": "2019-08-16T08:05:31.504Z"
  },
  "state": {
    "$class": "matt.inf.InfContractState",
    "value": null,
    "stateId": "inf0"
  },
  "emit": []
}

Notice the value of value in the state response.

If you then update the value of state.json with the new state, it will fail to pass validation.

mattmbp:minimal matt$ ergorun execute --contract contract.json --request request.json --state state.json  model.cto infinity.ergo
09:05:41 - error: Instance matt.inf.InfContractState#state1 missing required field value

Expected behavior
The engine should throw a runtime error, perhaps similar to an enforce response, to alert the client to the overflow and protect corruption of the state.

[rajatrawataku1]

Can i work on this, if its not assigned to anyone else?

[mttrbrts]

@rajatrawataku1 by all means.

What I expect to happen is that the outputs of execution (contract state, response and emits) should all be validated, and a runtime error thrown if validation fails. Perhaps @jeromesimeon has some pointers?

[jeromesimeon]

Thanks @rajatrawataku1 and @mttrbrts I would actually first look at this exact same issue in Concerto (https://github.com/accordproject/concerto). I wouldn't be surprised that serialising from a Concerto object with +/-Infinity or NaN yields a JSON with a null. If so that would be the right place to fix this IMO.

[jeromesimeon]

A bit of a review of concerto about this. The internals seem sound, using parseFloat and should round trip properly. It's possible that the best approach might be to delay the check all the way to JSON.stringify.

This could be done along the lines of: https://stackoverflow.com/questions/21896792/force-json-stringify-to-emit-nan-infinity-or-js-json-lib-that-does-so

I would simply raise an error for NaN Infinity and -Infinity, something like:

bash-3.2$ cat test.js 
const json1 = {
    'foo': 'hello!',
    'bar': [ 1, 2, 3 ]
};
const json2 = {
    'foo': 'hello!',
    'bar': [ 1, 2, NaN ]
};

function safeStringify(input) {
    return JSON.stringify(input, function (key, value) {
        if (value !== value) {
            throw new Error('Cannot export NaN to JSON')
        }

        if (value === Infinity) {
            throw new Error('Cannot export Infinity to JSON');
        }

        if (value === -Infinity) {
            throw new Error('Cannot export -Infinity to JSON');
        }
        
        return value;
    });
}

console.log(JSON.stringify(json1));
console.log(safeStringify(json1));
console.log(JSON.stringify(json2));
console.log(safeStringify(json2));
bash-3.2$ node test.js
{"foo":"hello!","bar":[1,2,3]}
{"foo":"hello!","bar":[1,2,3]}
{"foo":"hello!","bar":[1,2,null]}
/Users/jeromesimeon/git/concerto/test.js:13
            throw new Error('Cannot export NaN to JSON')
            ^

Error: Cannot export NaN to JSON
    at Array.<anonymous> (/Users/jeromesimeon/git/concerto/test.js:13:19)
    at JSON.stringify (<anonymous>)
    at safeStringify (/Users/jeromesimeon/git/concerto/test.js:11:17)
    at Object.<anonymous> (/Users/jeromesimeon/git/concerto/test.js:31:13)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Function.Module.runMain (module.js:694:10)

The benefit of that approach (and delaying the check to JSON.stringify) is that applications using the API would be able to handle results or inputs that include the full range of IEEE 754 values.

[jeromesimeon]

@rajatrawataku1 would you still feel up to the task?

This should include a review of everywhere we use JSON.stringify and maybe to put the variant above in a common utility file e.g., https://github.com/accordproject/ergo/blob/master/packages/ergo-compiler/lib/util.js

[jolanglinais]

Moving this back to open after 7 days.

[jeromesimeon]

@mttrbrts Any thoughts on my alternative suggestion? Would you see any benefit in doing this at validation time rather than serialization time?

[coderkalyan]

Could I take a shot at this? I'm new to this project, and would appreciate some guidance!

[jeromesimeon]

Could I take a shot at this? I'm new to this project, and would appreciate some guidance!

Sure @coderkalyan ! There is already quite a lot of details in this thread. Does it make sense to you, or which part is unclear?

[mttrbrts]

@coderkalyan yes, please feel free to pick this up. @jeromesimeon's approach is correct.

IMO, the correct place to fix this is in the ResourceValidator in Concerto

concerto/packages/concerto-core/lib/serializer/resourcevalidator.js

Line 293 in bf155ae

if(field.isPrimitive()) {

If we raise validation errors for NaN, +Infinity, and -Infinity, we can detect these elsewhere in the stack.

[jeromesimeon]

No activity on this for a while, I'm releasing the ownership for now.

[jeromesimeon]

A fix for this issue, by changing toJSON call in the serializer to raise a validation error when the Double is NaN, Infinity or -Infinity is available for review in PR #161.

[jeromesimeon]

A fix for this issue, by changing toJSON call in the serializer to raise a validation error when the Double is NaN, Infinity or -Infinity is available for review in PR #161.

That fix has been refactored to be done in the resourcevalidator rather than the jsongenerator class, as suggested by @mttrbrts. This provides better and more consistent error messages.

[jeromesimeon]

Fixed based on proposal in #161 in the 1.0 branch