Do not set calcNewSig to false in request #1056
Conversation
| stat: "instructions:u", | ||
| }, state); | ||
| values["calcNewSig"] = values.calcNewSig === 'true'; | ||
| // Make sure we pass a boolean. | ||
| if (typeof values.calcNewSig === "string") { |
There was a problem hiding this comment.
Can you explain why we need to do this?
There was a problem hiding this comment.
I haven't verified, but I believe serde will fail to deserialize calcNewSig:"true" as a bool, so we need to force the conversion to boolean in JS for the case where it's set from the query parameters
There was a problem hiding this comment.
Yea that seems true, but we were already setting calcNewSig to a bool. This seems to just conditionally set if it were previously a string. I'm just confused why we need to add this conditional.
There was a problem hiding this comment.
Without this patch, values.calcNewSig is undefined (since it's not mentioned on the query string), and that is !== "true", so we get false and don't enable the new alg.
If it is mentioned, we need to "deserialize" it with the === "true" but if we do that unconditionally we'll end up setting it to false (since we've put a boolean in the map). I guess an alternative could be to put "true" in the default object here.
Missed another spot where this needs adjusting...