You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Looks good. I would suggest to keep it a bit more simple.
extra_info related functionality should be dropped
remove getResponse method, error message is available via error.message that comes form the base class.
remove equals method
I am not so sure about having code and errorType attributes together. They seem to overlap a little. Are there any rules on how they are defined? Wouldn't just having one be enough? Also, shouldn't code be a unique value?
Why: This suggestion correctly identifies a critical typo in the method call validateHahserConfig that would cause a runtime error since the method doesn't exist. This is a high-impact bug fix that prevents the code from running.
Medium
Fix unreliable object comparison method
Using JSON.stringify for object comparison is unreliable due to property ordering differences. This can cause false negatives when comparing objects with identical content but different key orders.
Why: The suggestion correctly identifies that using JSON.stringify for object comparison is unreliable due to property ordering. However, the improved code introduces a complex deepEqual method when simpler alternatives like isDeepStrictEqual from Node.js util module (already imported in the test file) could be used.
Medium
General
Fix typo and consolidate validation logic
The method name validateHahserConfig contains a typo and should be validateHasherConfig. Additionally, both methods have identical logic and could be consolidated into a single reusable method.
-private validateSignerConfig(config: SignerConfig | HasherConfig) {+private validateSignerConfig(config: SignerConfig) {+ this.validateConfig(config, 'signer');+}++private validateHasherConfig(config: HasherConfig) {+ this.validateConfig(config, 'hasher');+}++private validateConfig(config: SignerConfig | HasherConfig, type: 'signer' | 'hasher') {
if (!config.callback || typeof config.callback !== 'function') {
- throw new SDJWTVCError('signer_callback_function_is_required');+ throw new SDJWTVCError(`${type}_callback_function_is_required`);
}
if (!config.alg || typeof config.alg !== 'string') {
- throw new SDJWTVCError('signer_algorithm_is_required');+ throw new SDJWTVCError(`${type}_algorithm_is_required`);
}
}
-private validateHahserConfig(config: SignerConfig | HasherConfig) {- if (!config.callback || typeof config.callback !== 'function') {- throw new SDJWTVCError('hasher_callback_function_is_required');- }- if (!config.alg || typeof config.alg !== 'string') {- throw new SDJWTVCError('hasher_algorithm_is_required');- }-}-
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies the typo in validateHahserConfig and proposes consolidating duplicate validation logic. While the consolidation improves maintainability, the impact is moderate since the methods are private and the typo doesn't affect functionality.
There’s a typo in the method name validateHahserConfig and its invocation, causing potential runtime failures. Rename both occurrences to validateHasherConfig so the method is correctly referenced and invoked.
Why: Fixing the typo in validateHahserConfig is crucial to avoid runtime errors when initializing the issuer and ensures the hasher config validation is actually invoked.
Medium
General
Remove test-utils import
The production Holder class should not depend on test utilities. Remove the import of hasherCallbackFn from ./test-utils/helpers.js to eliminate unintended test-only dependencies.
-import { hasherCallbackFn } from './test-utils/helpers.js';+
Suggestion importance[1-10]: 5
__
Why: Dropping hasherCallbackFn from production code prevents unintended test-only dependencies, improving module isolation, though its impact is minor.
Low
Strengthen equals comparison
The equals method lacks an explicit return type and uses JSON.stringify for object comparison, which can fail on key order differences. Add a : boolean return annotation and replace the string comparison with a deep comparison using key-by-key checks.
Why: Adding a boolean return type and replacing JSON.stringify with a deep key-by-key check improves correctness, but this method is rarely critical and the change is mainly stylistic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Just an idea I had to improve how errors can be extended with codes allowing for better handling by clients.
I think it also fixes some other issues in the specs