Check if wasm/zkey/vkey exist on RLN initialization

[AtHeartEngineer]

Wasm/zkey isn't getting fetched until a proof is generated; this is bad developer experience since they won't know something is wrong until they actually go to generate a proof, instead of on RLN instantiation.

[mhchia]

Agreed. I'd like to add that if we add a check in the constructor, we should also make sure the check is working in the browser.

[s-ekai]

@AtHeartEngineer

I'm considering working on this issue. Would it be okay to add the following code to rln.ts?

    if (typeof args.wasmFilePath === 'string' && !fs.existsSync(args.wasmFilePath)) {
      throw new Error(
        'the file does not exist at the path for `wasmFilePath`'
      )
    }

    if (typeof args.finalZkeyPath === 'string' && !fs.existsSync(args.finalZkeyPath)) {
      throw new Error(
        'the file does not exist at the path for `finalZkeyPath`'
      )
    }

I find it challenging to add conditions for verificationKey since it's not a file path but an actual verification key. Also, I believe that when the data types for wasmFilePath and finalZkeyPath are Uint8Array, they already contain the file contents. Therefore, I think there's no need to add conditions in those cases.

[mhchia]

Hey @s-ekai , thanks for working on this and the clarification. I think it's good to have two ifs to check, but I think there might be issues in the browser if we use fs.existsSync. Or, can it possibly be polyfiiled by the rollup bundler so that we can support both the browser and NodeJS?

[s-ekai]

@mhchia

Thank you for your response. I hadn't considered running it in a browser. I'll think about whether there's a good way to do it. Thank you also for telling me about the library!

[mhchia]

Hey @s-ekai no problem :) Actually we've already used roIlup to bundle the package. I'm thinking if polyfill can automatically replace fs.existsSync with a browser-compatible function, that's great. But, if polyfill can't, we can also use a if-else to check. Something like the following

if (typeof process !== 'undefined') {
    // In NodeJS
    // import fs and check fs.existsSync
} else {
    // In browser, we need to use another API to check if a resource at the URL exists.
}

I'm not sure if it's this is the correct way to check though. Let me know your thoughts on this 🙏

[s-ekai]

@mhchia

Thank you for explaining so clearly. I also suspect that it is probably being loaded automatically.

I have a question about debugging methods.
I replaced the code of the rlnjs library under node_modules within the examples/browser/ of this repository as shown below link and built it. However, the changes are not reflected. (I confirmed that the changes haven't been saved by checking inside examples/browser/dist/index.mjs and not seeing the changes there.) Could it be caching or something similar?

If you know of any better ways to verify this, I would appreciate your guidance.

https://github.com/s-ekai/rlnjs/pull/1/files

[s-ekai]

I realized that I should modify the code below. I'll check it out.
examples/browser/node_modules/rlnjs/dist/index.mjs

[mhchia]

Yeah you're right. If you wanted to modify code rlnjs and test it in the examples/browser. You could also do something like the followings

1. Change examples/browser to use the local rlnjs

In examples/browser/package.json

{
  "name": "example-browser",
  "version": "1.0.0",
  "description": "",
  "main": "dist/index.mjs",
  "module": "dist/index.mjs",
  "scripts": {
    "build": "rollup --config rollup.config.mjs",
    "test": "npm run build && mkdir -p public && cp dist/index.mjs index.html public && cd public && npx http-server",
    "clean": "rm -rf dist"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "@rollup/plugin-commonjs": "^24.1.0",
    "@rollup/plugin-json": "^6.0.0",
    "@rollup/plugin-node-resolve": "^15.0.2",
    "@rollup/plugin-replace": "^5.0.2",
    "rollup": "^3.20.2",
    "rollup-plugin-cleaner": "^1.0.0",
    "rollup-plugin-polyfill-node": "^0.12.0",
    "rollup-plugin-typescript2": "^0.34.1",
    "rollup-plugin-visualizer": "^5.9.0",
    "rlnjs": "file:../../"
  }
}

2. Modify rlnjs. Build the rlnjs package again with npm run build

# Go to rlnjs project root
cd ../../
npm run build

3. Install and build the browser example again

cd examples/browser
npm install && npm test

[s-ekai]

@mhchia Thank you very much!

And there is no way to load(read) local files from a browser.
(If a browser can load(read) local files, there are security concerns. However, if the file is under the management of an HTTP server, it can be loaded.)
Therefore, I thought it was unnecessary to use libraries like fs in the browser to check whether a file exists.

In APIs that communicate with local files on the browser side, as far as I can see, they use the input tag to load the file.
In this case, there is no need to verify whether the file path is correct.

For this implementation, if the data type of args.finalZkeyPath is a string, I thought it might be considered as running in a node environment rather than in a browser. (It might also be good to check if the process is defined for safety reasons.)

Could you tell me your opinion?

[mhchia]

Hey @s-ekai, sorry for the late response, and thanks for your input. You're right that browsers cannot access local files. However, args.finalZkeyPath and args.wasmFilePath can be URLs when using RLNjs in browsers. In this case, we can use something like an HTTP HEAD request to check if a resource exists without downloading the whole file. Something like axiom.head should be able to do this. Please let me know if I get something wrong here 🙏

[s-ekai]

@mhchia

Thank you for reaching out!
I hadn't fully considered the case of downloading from the web.
I'll start working on it based on the advice you've provided so far!

[s-ekai]

@mhchia

I had forgotten that we can't perform asynchronous operations in a class constructor. Therefore, I feel that if I need to check whether the URL is valid, I can only do it within the create function. Is this approach okay? I'm also thinking of moving the conditions that are already in the constructor to the create function as much as possible.

[mhchia]

Thanks for bringing this up. Yes, we can do it in RLN.create

I'm also thinking of moving the conditions that are already in the constructor to the create function as much as possible

Can you elaborate on what conditions are you moving? or we can discuss it in your pull request too :)

[s-ekai]

@mhchia

Thank you.
I’ve tried creating the code snippet as envisioned here.
https://github.com/s-ekai/rlnjs/pull/1/files
(Note: I haven’t tested or verified this code yet.If the approach of this code is acceptable, I plan to add tests and clean up the code.)

• Initially, I considered moving the following code into the create function. However, since wasmFilePath and other variables are initialized within the create function, I decided against it.

    if ((args.wasmFilePath === undefined || args.finalZkeyPath === undefined) && args.verificationKey === undefined) {
      throw new Error(
        ‘Either both `wasmFilePath` and `finalZkeyPath` must be supplied to generate proofs, ’ +
        ‘or `verificationKey` must be provided to verify proofs.’,
      )
    }

• To check if the runtime environment is a browser, I added the code typeof window !== 'undefined' && typeof window.document !== 'undefined'. This is not perfect as it doesn’t account for cases where the window variable is defined within the code, but I couldn’t think of a better way.

[mhchia]

@s-ekai Great work! I just left some comments in the pull request in your repo. Let me know your thoughts on them :)

To check if the runtime environment is a browser, I added the code typeof window !== 'undefined' && typeof window.document !== 'undefined'. This is not perfect as it doesn’t account for cases where the window variable is defined within the code, but I couldn’t think of a better way.

What if we check process and if it exists it is in NodeJS, or is there any issue doing this?

[s-ekai]

@mhchia
Thank you for your comment!
I've updated the PR and added more tests.

https://github.com/s-ekai/rlnjs/pull/1/files

I believe that just checking the 'process' variable should be sufficient, but I can't say for certain. I looked into discussions on Stack Overflow, and some people are checking with 'require' or 'global' variables. For this case, just to be safe, I'm checking for the existence of both 'process' and 'window' variables to determine if it's a browser environment.

[mhchia]

Hey @s-ekai , awesome work! Thanks for the investigation and also for your tests. Can you also do the same check for withdrawWasmFilePath and withdrawFinalZkeyPath in createWithContractRegistry? After this one I think it'll be all set. And would you mind opening a PR in this repo?

[s-ekai]

Thank you. I will make the corrections and create a PR (Pull Request)!

On a different note, while I was testing in the browser, I encountered the following error. It might be a good idea to change the CORS settings on the S3 side. (If you're not expecting this file to be accessed from the browser, please disregard this comment.)

Access to XMLHttpRequest at 'https://rln-trusted-setup-ceremony-pse-p0tion-production.s3.eu-central-1.amazonaws.com/circuits/rln-20/RLN-20.wasm' from origin 'http://127.0.0.1:8080' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

I've modified and executed the code inside examples/browser as follows.

    async function createRLNInstance() {
        return await RLN.createWithContractRegistry({
            /* Required */
            rlnIdentifier,
            provider,
            contractAddress: rlnContractAddress,
            /* Optional */
            contractAtBlock: rlnContractAtBlock,
            signer,
            wasmFilePath: "https://rln-trusted-setup-ceremony-pse-p0tion-production.s3.eu-central-1.amazonaws.com/circuits/rln-20/RLN-20.wasm",
            finalZkeyPath:  "https://rln-trusted-setup-ceremony-pse-p0tion-production.s3.eu-central-1.amazonaws.com/contributions/rln-20_final.zkey"
        })
    }

[s-ekai]

@mhchia I've created new PR!
I would appreciate it if you could check it when you have some time.

#109