Skip to content

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented Apr 11, 2024

This change allows to configure sjcl and use it as a normal dependency, together with @types/sjcl.
Impacts on reducing the code of the library by half.

This change allows to configure sjcl and use it as a normal
dependency, together with @types/sjcl.
Impacts on reducing the code of the library.
@armfazh armfazh requested a review from thibmeu April 11, 2024 08:33
// 1. If the signature representative s is not between 0 and n - 1,
// output "signature representative out of range" and stop.
if (!s.greaterEquals(new sjcl.bn(0)) || s.greaterEquals(pkS.n) == 1) {
if (!s.greaterEquals(new sjcl.bn(0)) || s.greaterEquals(pkS.n)) {
Copy link

@lukevalenta lukevalenta Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be BigNumber here?

Suggested change
if (!s.greaterEquals(new sjcl.bn(0)) || s.greaterEquals(pkS.n)) {
if (!s.greaterEquals(new sjcl.BigNumber(0)) || s.greaterEquals(pkS.n)) {

EDIT: nevermind, I see the bn() function is defined in sjcl, and the BigNumber infterface in @types/sjcl.

Although, the greaterEquals() function also accepts a number as an argument (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/sjcl/index.d.ts#L42). Why not use this instead?

Suggested change
if (!s.greaterEquals(new sjcl.bn(0)) || s.greaterEquals(pkS.n)) {
if (!s.greaterEquals(0) || s.greaterEquals(pkS.n)) {

Copy link
Contributor Author

@armfazh armfazh Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when passing a number, the function internally creates a new object, then it becomes equivalent.
http://bitwiseshiftleft.github.io/sjcl/doc/bn.js.html#line75

Copy link
Contributor

@thibmeu thibmeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall ok.
did you confirm it works when imported?

"clean": "rimraf lib coverage",
"sjcl": "cd node_modules/sjcl && concat -o sjcl.js core/sjcl.js core/aes.js core/bitArray.js core/codecString.js core/codecBase64.js core/codecBytes.js core/sha256.js core/random.js core/bn.js core/exports.js",
"prepack": "npm run sjcl && npm run build",
"prepare": "npm run sjcl && npm run build"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is prepare always run when the library is imported? the issue I faced in the past was @cloudflare/blindrsa-ts would not work when imported indirectly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could use some help to achieve this.
Either prepare or postinstall can trigger the script, however, the path to sjcl needs to be resolved.

Copy link
Contributor

@thibmeu thibmeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there needs to be changes:

  • prepack and prepare run respectively before the folder is packaged, and before publishing respectively (see npm docs). What you're looking for is postinstall, because you'd like node_modules/* folder to be modified
  • Folder for npm scripts are not fixed. When installed as a dependency, the folder is the one of said dependency, when installed as a main package, it's the main folder. This creates something tricky to trace, not even mentioning having dependency x modify dependency y is a bad practice

If we want to keep @cloudflare/blindrsa-ts unbundled, I suggest:

  1. Declare @types/sjcl as a dependency (not a devDependency)
  2. Remove scripts sjcl, prepare, pack

The diff is

diff --git a/package-lock.json b/package-lock.json
index fd3a106..b14bc64 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -9,12 +9,12 @@
             "version": "0.4.2",
             "license": "Apache-2.0",
             "dependencies": {
+                "@types/sjcl": "1.0.34",
                 "sjcl": "1.0.8"
             },
             "devDependencies": {
                 "@types/benchmark": "2.1.5",
                 "@types/jest": "29.5.12",
-                "@types/sjcl": "1.0.34",
                 "@typescript-eslint/eslint-plugin": "7.2.0",
                 "@typescript-eslint/parser": "7.2.0",
                 "benchmark": "2.1.4",
@@ -1516,8 +1516,7 @@
         "node_modules/@types/sjcl": {
             "version": "1.0.34",
             "resolved": "https://registry.npmjs.org/@types/sjcl/-/sjcl-1.0.34.tgz",
-            "integrity": "sha512-bQHEeK5DTQRunIfQeUMgtpPsNNCcZyQ9MJuAfW1I7iN0LDunTc78Fu17STbLMd7KiEY/g2zHVApippa70h6HoQ==",
-            "dev": true
+            "integrity": "sha512-bQHEeK5DTQRunIfQeUMgtpPsNNCcZyQ9MJuAfW1I7iN0LDunTc78Fu17STbLMd7KiEY/g2zHVApippa70h6HoQ=="
         },
         "node_modules/@types/stack-utils": {
             "version": "2.0.1",
diff --git a/package.json b/package.json
index 0121f80..0608410 100644
--- a/package.json
+++ b/package.json
@@ -34,18 +34,15 @@
         "format": "prettier './(src|test|examples)/**/!(*.d).ts' --write",
         "bench": "tsc -b bench && node ./lib/bench/index.js",
         "examples": "tsc -b examples && node ./lib/examples/index.js",
-        "clean": "rimraf lib coverage",
-        "sjcl": "cd node_modules/sjcl && concat -o sjcl.js core/sjcl.js core/aes.js core/bitArray.js core/codecString.js core/codecBase64.js core/codecBytes.js core/sha256.js core/random.js core/bn.js core/exports.js",
-        "prepack": "npm run sjcl && npm run build",
-        "prepare": "npm run sjcl && npm run build"
+        "clean": "rimraf lib coverage"
     },
     "dependencies": {
+        "@types/sjcl": "1.0.34",
         "sjcl": "1.0.8"
     },
     "devDependencies": {
         "@types/benchmark": "2.1.5",
         "@types/jest": "29.5.12",
-        "@types/sjcl": "1.0.34",
         "@typescript-eslint/eslint-plugin": "7.2.0",
         "@typescript-eslint/parser": "7.2.0",
         "benchmark": "2.1.4",

@armfazh armfazh added the help wanted Extra attention is needed label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants