fix: addressing Huberts PR comments

rebuild/binding.gyp

@@ -27,6 +27,7 @@
           '-Wpedantic',
       ],
       'cflags_cc': [
+          '-std=c++17',
           '-fexceptions',
           '-Werror',
           '-Wall',
@@ -41,6 +42,7 @@
               'VCCLCompilerTool': {
                 'ExceptionHandling': 1,
                 'EnablePREfast': 'true',
+                'AdditionalOptions': [ '/std:c++17' ], 
               },
             },
           }
@@ -52,6 +54,7 @@
         ['OS=="mac"', {
           'xcode_settings': {
             'OTHER_CFLAGS': ['-fvisibility=hidden'],
+            'CLANG_CXX_LANGUAGE_STANDARD': 'c++17',
             'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',
             'CLANG_CXX_LIBRARY': 'libc++',
             'MACOSX_DEPLOYMENT_TARGET': '12',

rebuild/src/addon.cc

@@ -1,61 +1,63 @@
 #include "addon.h"
 
+namespace blst_ts {
 bool is_zero_bytes(
-    const uint8_t *data, const size_t start_byte, const size_t byte_length) {
-    for (size_t i = start_byte; i < byte_length; i++) {
-        if (data[i] != 0) {
-            return false;
-        }
+    const uint8_t *data,
+    const size_t starting_index,
+    const size_t byte_length) noexcept {
+    if (starting_index < byte_length) {
+        return std::all_of(
+            data + starting_index, data + byte_length, [](uint8_t x) {
+                return x == 0x00;
+            });
     }
-    return true;
+    return false;
 }
 
-bool is_valid_length(
-    std::string &error_out,
-    size_t byte_length,
-    size_t length1,
-    size_t length2) {
+[[nodiscard]] std::optional<std::string> is_valid_length(
+    size_t byte_length, size_t length1, size_t length2) noexcept {
     if (byte_length == length1 || (length2 != 0 && byte_length == length2)) {
-        return true;
+        return std::nullopt;
     }
-    error_out.append(" must be ");
+    std::string err_msg{" must be "};
     if (length1 != 0) {
-        error_out.append(std::to_string(length1));
+        err_msg.append(std::to_string(length1));
     };
     if (length2 != 0) {
         if (length1 != 0) {
-            error_out.append(" or ");
+            err_msg.append(" or ");
         }
-        error_out.append(std::to_string(length2));
+        err_msg.append(std::to_string(length2));
     };
-    error_out.append(" bytes long");
-    return false;
+    err_msg.append(" bytes long");
+    return err_msg;
 }
+}  // namespace blst_ts
 
 BlstTsAddon::BlstTsAddon(Napi::Env env, Napi::Object exports)
-    : _dst{"BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_POP_"},
-      _blst_error_strings{
-          "BLST_SUCCESS",
-          "BLST_ERROR::BLST_BAD_ENCODING",
-          "BLST_ERROR::BLST_POINT_NOT_ON_CURVE",
-          "BLST_ERROR::BLST_POINT_NOT_IN_GROUP",
-          "BLST_ERROR::BLST_AGGR_TYPE_MISMATCH",
-          "BLST_ERROR::BLST_VERIFY_FAIL",
-          "BLST_ERROR::BLST_PK_IS_INFINITY",
-          "BLST_ERROR::BLST_BAD_SCALAR",
-      } {
+    : _blst_error_strings{
+        "BLST_SUCCESS",
+        "BLST_ERROR::BLST_BAD_ENCODING",
+        "BLST_ERROR::BLST_POINT_NOT_ON_CURVE",
+        "BLST_ERROR::BLST_POINT_NOT_IN_GROUP",
+        "BLST_ERROR::BLST_AGGR_TYPE_MISMATCH",
+        "BLST_ERROR::BLST_VERIFY_FAIL",
+        "BLST_ERROR::BLST_PK_IS_INFINITY",
+        "BLST_ERROR::BLST_BAD_SCALAR",
+    },
+    dst{"BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_POP_"} {
     Napi::Object js_constants = Napi::Object::New(env);
     js_constants.Set(
-        Napi::String::New(env, "DST"), Napi::String::New(env, _dst));
+        Napi::String::New(env, "DST"), Napi::String::New(env, dst));
     DefineAddon(
         exports,
         {
             InstanceValue("BLST_CONSTANTS", js_constants, napi_enumerable),
         });
-    SecretKey::Init(env, exports, this);
-    PublicKey::Init(env, exports, this);
-    Signature::Init(env, exports, this);
-    Functions::Init(env, exports);
+    blst_ts::SecretKey::Init(env, exports, this);
+    blst_ts::PublicKey::Init(env, exports, this);
+    blst_ts::Signature::Init(env, exports, this);
+    blst_ts_functions::init(env, exports);
     env.SetInstanceData(this);
 
     // Check that openssl PRNG is seeded
@@ -68,6 +70,14 @@ BlstTsAddon::BlstTsAddon(Napi::Env env, Napi::Object exports)
 }
 
 std::string BlstTsAddon::GetBlstErrorString(const blst::BLST_ERROR &err) {
+    size_t err_index = static_cast<size_t>(err);
+    // size of total array divided by size of one element minus 1 for 0 index
+    // basis
+    size_t max_index =
+        sizeof(_blst_error_strings) / sizeof(_blst_error_strings[0]) - 1;
+    if (err_index > max_index) {
+        return "BLST_ERROR::UNKNOWN_ERROR_CODE";
+    }
     return _blst_error_strings[err];
 }
 
@@ -77,10 +87,8 @@ bool BlstTsAddon::GetRandomBytes(blst::byte *bytes, size_t length) {
     // [RandomBytesTraits::DeriveBits](https://github.com/nodejs/node/blob/4166d40d0873b6d8a0c7291872c8d20dc680b1d7/src/crypto/crypto_random.cc#L65)
     // [CSPRNG](https://github.com/nodejs/node/blob/4166d40d0873b6d8a0c7291872c8d20dc680b1d7/src/crypto/crypto_util.cc#L63)
     do {
-        if (1 == RAND_status()) {
-            if (1 == RAND_bytes(bytes, length)) {
-                return true;
-            }
+        if ((1 == RAND_status()) && (1 == RAND_bytes(bytes, length))) {
+            return true;
         }
     } while (1 == RAND_poll());
 

rebuild/src/addon.h

@@ -1,34 +1,26 @@
-#ifndef BLST_TS_ADDON_H__
-#define BLST_TS_ADDON_H__
+#pragma once
 
 #include <openssl/rand.h>
 
+#include <algorithm>
 #include <iostream>
 #include <memory>
-#include <sstream>
+#include <optional>
 #include <string>
 #include <string_view>
 
 #include "blst.hpp"
 #include "napi.h"
 
-// TODO: these should come out post PR review
-using std::cout;
-using std::endl;
-
+namespace blst_ts {
 #define BLST_TS_RANDOM_BYTES_LENGTH 8U
 
 #define BLST_TS_FUNCTION_PREAMBLE(info, env, module)                           \
     Napi::Env env = info.Env();                                                \
     Napi::EscapableHandleScope scope(env);                                     \
     BlstTsAddon *module = env.GetInstanceData<BlstTsAddon>();
 
-#define BLST_TS_IS_INFINITY                                                \
-    Napi::Env env = info.Env();                                                \
-    Napi::EscapableHandleScope scope(env);                                     \
-    return scope.Escape(Napi::Boolean::New(env, _point->IsInfinite()));
-
-#define BLST_TS_SERIALIZE_POINT(macro_name)                                \
+#define BLST_TS_SERIALIZE_POINT(snake_case_name)                               \
     Napi::Env env = info.Env();                                                \
     Napi::EscapableHandleScope scope(env);                                     \
     bool compressed{true};                                                     \
@@ -37,12 +29,12 @@ using std::endl;
     }                                                                          \
     Napi::Buffer<uint8_t> serialized = Napi::Buffer<uint8_t>::New(             \
         env,                                                                   \
-        compressed ? BLST_TS_##macro_name##_LENGTH_COMPRESSED                  \
-                   : BLST_TS_##macro_name##_LENGTH_UNCOMPRESSED);              \
-    _point->Serialize(compressed, serialized.Data());                          \
+        compressed ? snake_case_name##_length_compressed                       \
+                   : snake_case_name##_length_uncompressed);                   \
+    point->Serialize(compressed, serialized.Data());                           \
     return scope.Escape(serialized);
 
-#define BLST_TS_UNWRAP_UINT_8_ARRAY(value_name, arr_name, js_name)         \
+#define BLST_TS_UNWRAP_UINT_8_ARRAY(value_name, arr_name, js_name)             \
     if (!value_name.IsTypedArray()) {                                          \
         Napi::TypeError::New(                                                  \
             env, "BLST_ERROR: " js_name " must be a BlstBuffer")               \
@@ -59,79 +51,95 @@ using std::endl;
     Napi::Uint8Array arr_name =                                                \
         arr_name##_array.As<Napi::TypedArrayOf<uint8_t>>();
 
-#define BLST_TS_CLASS_UNWRAP_UINT_8_ARRAY(value_name, arr_name, js_name)   \
+#define BLST_TS_CLASS_UNWRAP_UINT_8_ARRAY(value_name, arr_name, js_name)       \
     if (!value_name.IsTypedArray()) {                                          \
         Napi::TypeError::New(                                                  \
             env, "BLST_ERROR: " js_name " must be a BlstBuffer")               \
             .ThrowAsJavaScriptException();                                     \
-        m_has_error = true;                                                    \
+        has_error = true;                                                      \
         return;                                                                \
     }                                                                          \
     Napi::TypedArray arr_name##_array = value_name.As<Napi::TypedArray>();     \
     if (arr_name##_array.TypedArrayType() != napi_uint8_array) {               \
         Napi::TypeError::New(                                                  \
             env, "BLST_ERROR: " js_name " must be a BlstBuffer")               \
             .ThrowAsJavaScriptException();                                     \
-        m_has_error = true;                                                    \
+        has_error = true;                                                      \
         return;                                                                \
     }                                                                          \
     Napi::Uint8Array arr_name =                                                \
         arr_name##_array.As<Napi::TypedArrayOf<uint8_t>>();
 
-class BlstTsAddon;
-
 typedef enum { Affine, Jacobian } CoordType;
 
 /**
- * Checks a byte array to see if it is all zeros. Can pass start byte for the
- * cases where the first byte is the tag (infinity point and
- * compress/uncompressed).
+ * Checks if a specified range of bytes within a byte array consists only of
+ * zeros.
  *
- * @param data uint8_t*
- * @param start_byte size_t
- * @param byte_length size_t
+ * @param data A pointer to the first element of the byte array to be checked.
+ * @param starting_index The offset (index) from the beginning of the array
+ * where the check should start. (0 starts at *data)
+ * @param byte_length The total length of the data array
  *
- * @return bool
+ * @return Returns true if all bytes from start_byte to the end of the array are
+ * zeros. Returns false if the starting offset is beyond the array length or if
+ * any byte in the specified range is not zero.
  */
 bool is_zero_bytes(
-    const uint8_t *data, const size_t start_byte, const size_t byte_length);
+    const uint8_t *data,
+    const size_t start_byte,
+    const size_t byte_length) noexcept;
 
 /**
- * Checks if a byte array is a valid length. If not, sets the error message and
- * returns false.  If valid returns true for use in conditional statements.
+ * Validates that a given byte length matches one of two specified lengths,
+ * returning an optional error message if the validation fails.
  *
- * @param[out] error_out &std::string - error message to set if invalid
- * @param[in] byte_length size_t - length of the byte array to validate
- * @param[in] length1 size_t - first valid length
- * @param[in] length2 size_t - second valid length (optional)
+ * @param byte_length The length to be validated against length1 and length2.
+ * @param length1 The first valid length that byte_length can be. A value of 0
+ * is considered as not set and thus not compared.
+ * @param length2 The second valid length that byte_length can be. A value of 0
+ * is considered as not set and thus not compared.
  *
- * @return bool
+ * @return An std::optional<std::string> that contains an error message if
+ * byte_length does not match either length1 or (if length2 is not 0) length2.
+ * If byte_length is valid, std::nullopt is returned. The error message, if
+ * returned, specifies the valid lengths byte_length must match.
+ *
+ * @note If both length1 and length2 are provided (non-zero), and byte_length
+ * does not match, the error message indicates that the valid byte_length must
+ * be either length1 or length2. If only one length is provided (the other being
+ * 0), the error message will only reference the provided length. This function
+ * is marked with [[nodiscard]] to ensure that the caller handles the potential
+ * error message, promoting safer and more intentional error checking.
  */
-bool is_valid_length(
-    std::string &error_out,
-    size_t byte_length,
-    size_t length1,
-    size_t length2 = 0);
+[[nodiscard]] std::optional<std::string> is_valid_length(
+    size_t byte_length, size_t length1, size_t length2 = 0) noexcept;
+}  // namespace blst_ts
 
+class BlstTsAddon;
 /**
  * Circular dependency if these are moved up to the top of the file.
  */
-#include "functions.h"
 #include "public_key.h"
 #include "secret_key.h"
 #include "signature.h"
+namespace blst_ts_functions {
+void init(const Napi::Env &env, Napi::Object &exports);
+}
 
 /**
  * BlstTsAddon is the main entry point for the library. It is responsible
  * for initialization and holding global values.
  */
 class BlstTsAddon : public Napi::Addon<BlstTsAddon> {
-   public:
-    std::string _dst;
+   private:
     std::string _blst_error_strings[8];
-    Napi::FunctionReference _secret_key_ctr;
-    Napi::FunctionReference _public_key_ctr;
-    Napi::FunctionReference _signature_ctr;
+
+   public:
+    std::string dst;
+    Napi::FunctionReference secret_key_ctr;
+    Napi::FunctionReference public_key_ctr;
+    Napi::FunctionReference signature_ctr;
 
     /**
      * BlstTsAddon::BlstTsAddon constructor used by Node.js to create an
@@ -173,5 +181,3 @@ class BlstTsAddon : public Napi::Addon<BlstTsAddon> {
      */
     bool GetRandomBytes(blst::byte *ikm, size_t length);
 };
-
-#endif /* BLST_TS_ADDON_H__ */