load_env damages unquoted strings

[pro-wh]

Describe the bug I have a script like this:

export A=a6ed68a1cc964b430e1e40254347367f08e4eb5eeaf0852d5648022873b50c07
echo "A is $A"
./rosetta-cli --configuration-file rosetta-cli-config.json check:construction

And my .ros file has this in a scenario:

a = load_env("A");
print_message({"a": "{{a}}"});

The output is this from the echo:

A is a6ed68a1cc964b430e1e40254347367f08e4eb5eeaf0852d5648022873b50c07

and this from the print_message:

2021/12/22 11:40:18 Message: {"a": "6ed68a1cc964b430e1e40254347367f08e4eb5eeaf0852d5648022873b50c07"}

Notice how the a before the 6 is missing when accessed from within the scenario.

To Reproduce

1. Create an environment variable that's not JSON, i.e. where strings aren't surrounded by quotes
2. Access it from a load_env action

Expected behavior Couple of weird things:

1. Something somewhere is cutting off the a at the beginning. My expectation is to see {"a": "a6ed...0c07"}
2. You need quotes around {{a}}, otherwise the {"a": 6ed...0c07} is not valid JSON. This kind of leaks the way this template stuff is done through string manipulation. Would it make sense to expect that load_env puts a value into a string and then JSON-serializes that string? Although this might break other people's use cases where they want a number or something.

Additional context

We use a local testnet to test our rosetta server, and each invocation of a local testnet has a unique network identifier. So we want to pass this in programmatically.

[shiatcb]

After some experiments and investigation, it seems the issue is in gjson.Get(): https://github.com/coinbase/rosetta-sdk-go/blob/master/constructor/worker/populator.go#L37.

Details: when print_message is parsed into rosetta-sdk-go, the input param will be populated as json object. Thus we will get {"a":a6ed68a1cc964b430e1e40254347367f08e4eb5eeaf0852d5648022873b50c07} as raw input. However, when we pass it to gjson.Get(), gjson will go through the value, find a digit '6', treat it as a number, and ignore the characters before '6', thus first character was truncated: https://github.com/tidwall/gjson/blob/master/gjson.go#L2183.

I've tried a few different inputs such as "abc6d" and observed the same symptom (aka the output is "6d"). We may need to report this issue to gjson developers, or work around it by writing a helper function in rosetta-sdk-go (maybe populator.go).

I've submitted an issue to gjson developer: https://github.com/tidwall/gjson/issues/256

[shrimalmadhur]

thanks @shiatcb for investigating, hopefully we get to hear from gjson.

@pro-wh on a separate not, I am quite curious as to why you are doing this each invocation of a local testnet has a unique network identifier ? Is there a particular reason for having different network identifier for each invocation?

[pro-wh]

They really are different networks, each one with its own independent network state. It actually seems like the correct way to do it :shrug:

[shrimalmadhur]

@pro-wh I see. but it's not that they are running in parallel right? Like if you running a node locally for testing and then kill it to run another one, you can still use the same network identifier right? Why do you want it to be different? (I can see different id if you want to run let's say two different testnets forever)

[pro-wh]

The second one would start over from a fresh state, perhaps even a different initial state, and it will process blocks and transactions unrelated to the ones processed on the first network. I must turn that question around--would you specifically want to make it impossible to distinguish between these two networks?

[shrimalmadhur]

@pro-wh yea agreed. but when it comes to testing, once you kill the first network you don't really need that state and when you start the new one you are starting from clean slate. My point is why would you need a different network identifier in that case, since the old network is irrelevant now for testing and has been shutdown so technically it's like that network never existed. It shouldn't impact the new network. Let me know if I am missing anything in terms of if the old network is relevant.

[pro-wh]

I think it's just that we don't have some special logic to force a fixed network ID. And without that, it's the usual story of "we must have a unique ID for each network to isolate transaction signatures."

[shrimalmadhur]

I see. that makes sense. Obviously for testnet and mainnet running in the network it will be different. But I was just curious about the local env. thanks for explaining though. We will find a solution to the json issue soon.

[shiatcb]

From sjson dev's response: https://github.com/tidwall/gjson/issues/256:

I recommend using sjson.Set instead of SetRaw. The Set takes care of converting the input, in the case of a string it adds quotations. SetRaw is for setting "raw" json.

@shrimalmadhur : do you know why we are using SetRaw from the beginning? Is there any scenario that can be handled by SetRaw but not Set?

[shrimalmadhur]

@shiatcb just to circle back on this, it was written before me but if Set is something we should use we should fix it and test it out to see if it works well with all the cases we expect.