Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(scope): move unstringification of BigInts to Library from client #146

Merged
merged 3 commits into from
May 17, 2022

Conversation

bajpai244
Copy link
Contributor

The CLI uses the unstringifyBigInts utility on Objects and then passes
it to the library functions. When working on a webclient or using the library
without the cli this leads to production of errors, one example is
generating callData for Solidity Verifier, since unstringification of
BigInts haven’t happened, the callData generated is corrupt, anyone
who wants to fix this would have to read the cli to figure out what is
going wrong.

Two files: plonk_verify.js & groth16_verify.js are doing this
unstringification in themselves already, and the CLI is doing it again
making the unstringification reduntant.

This PR moves the unstringification of BigInts to library files, which saves the
effort for anyone consuming the library to dig deep in the CLI and then
figure out what is going wrong, it also solves the redundancy mentioned
in the section above.

The PR also includes the updated builds and has also passed all
described library tests.

…ient

The CLI uses the `unstringifyBigInts` utility on Objects and then passes
it to the library functions. When working on a webclient or using the library
without the cli this leads to production of errors, one example is
generating callData for Solidity Verifier, since unstringification of
BigBigInts haven’t happened, the callData generated is corrupt, anyone
who wants to fix this would have to read the cli to figure out what is
going wrong.

Two files: `plonk_verify.js` & `groth16_verify.js` are doing this
unstringification in themselves already, and the CLI is doing it again
making the unstringification reduntant.

This PR moves the unstringification of BigInts to library files, which saves the
effort for anyone consuming the library to dig deep in the CLI and then
figure our what is going wrong, it also solves the reduntancy mentioned
in the section above.

The PR also includes the updated builds and has also passed all
described library tests.
@phated
Copy link
Contributor

phated commented May 16, 2022

@bajpai244 I generally like this PR! However, I'm unclear if this is a breaking change for anyone that is already using any of the APIs in something like node or the browser. For example, if they were already passing a BigInt to the functions (is that supported currently?) the unstringifyBigInts function would mangle it.

@bajpai244
Copy link
Contributor Author

bajpai244 commented May 16, 2022

@phated glad that u like it! So, this is a non-breaking change, if we look at the description of unstringifyBigInts, it is:

export function unstringifyBigInts(o) {
    if ((typeof(o) == "string") && (/^[0-9]+$/.test(o) ))  {
        return bigInt(o);
    } else if ((typeof(o) == "string") && (/^0x[0-9a-fA-F]+$/.test(o) ))  {
        return bigInt(o);
    } else if (Array.isArray(o)) {
        return o.map(unstringifyBigInts);
    } else if (typeof o == "object") {
        const res = {};
        const keys = Object.keys(o);
        keys.forEach( (k) => {
            res[k] = unstringifyBigInts(o[k]);
        });
        return res;
    } else {
        return o;
    }
}

As we can see that if a particular field is not of type string or object it simply returns it { the last else case! }, Hence if you will pass it an object that has unstringified bigInts, the function would have no effect on it! Therefore u can apply this utility as many times as u want.

Here is a quick example to test this out:

const { utils } = require("ffjavascript");
const crypto = require('crypto');
const assert = require('assert');

const { unstringifyBigInts, leBuff2int } = utils;

const randomBigInt = (nbytes) => leBuff2int(crypto.randomBytes(nbytes));

const main = () => {

  // the num field is a BigInt
  const obj = {
    num: randomBigInt(31)
  };

  // we are unstrigifying an object with BigInts, it still works like a charm!

  const unstringifiedObj = unstringifyBigInts(obj);

  console.log("obj =>", obj);
  console.log("unstringifiedObj =>", unstringifiedObj);
  
  // assertion passes
   assert.equal(obj.num === unstringifiedObj.num, true);
}

main();

p.s: This is already happening in the current version in plonk_verify.js & groth16_verify.js, i.e the CLI is passing them objects after unstringification and they are applying this function again in them, it still works like a charm!

@phated
Copy link
Contributor

phated commented May 16, 2022

@bajpai244 typeof the "big-integer" shim will be object. I'm guessing you ran your test on a platform that has native BigInt in javascript.

@phated
Copy link
Contributor

phated commented May 16, 2022

I'm checking with the team on whether we should drop the "big-integer" shim.

@bajpai244
Copy link
Contributor Author

bajpai244 commented May 16, 2022

@phated even on platforms that don't have native support this will work!

Let's take a look at how "big-integer" represents BigInt if there is no native support.

{ 
value: number | array[number], 
sign: boolean, 
isSmall: boolean 
}

Since no field of this object is of type string, hence unstringifyBigInts will have no effect on them! { it will run the last else on each of these fields! }. Hence I don't think there is any need to drop this shim!

Here is a repl that I have made: https://replit.com/@HARSHBAJPAI1/UnstringifyBigIntDemo#index.js

I have manually switched off support for native bigInt in this case! You can see that even if I am applying unstringifyBigInts on this Object representation of BigInt, it is still working and retaining its original form!

@phated
Copy link
Contributor

phated commented May 16, 2022

This is very helpful! I wonder if the "big-integer" library is storing the value as a number because it is within the valid JS range. I thought the trick for BigInts was to store them as strings? Maybe it has an optimization for small numbers.

@bajpai244
Copy link
Contributor Author

@phated so, big-integer it will store it as a number if the number is small, in that case, isSmall flag will be true, when the number is big { i.e isSmall flag is not true }, then the value will be store as an array of numbers.

for example let's take:

const obj = new bigInt("75643564363473453456342378564387956906736546456235345"); 

It will store obj as:

{
  value: [
    6235345, 3654645,
    9569067, 8564387,
    5634237, 4734534,
    3564363,    7564
  ],
  sign: false,
  isSmall: false
} 

Hence, none of its field will ever be of type string!

@phated
Copy link
Contributor

phated commented May 17, 2022

Excellent. Thanks. Can you remove the build/ changes? Those are generated during release and it'll make this PR easier for review by the rest of the team.

@bajpai244
Copy link
Contributor Author

@phated, I have removed all the build files from this PR 🙌

@jbaylina jbaylina merged commit 42d96f6 into iden3:master May 17, 2022
This was referenced May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants