validate provingMethodAlg

[volodymyr-basiuk]

No description provided.

[Copilot]

Pull request overview

This PR adds validation for the provingMethodAlg parameter in the DefaultZKPPacker.pack() method and updates the package version from 1.39.3 to 1.39.4.

Changes:

• Added validation checks for provingMethodAlg to ensure it's an instance of ProvingMethodAlg and has required alg and circuitId properties
• Changed circuit ID retrieval to use params.provingMethodAlg.circuitId directly instead of casting immediately
• Version bump to 1.39.4 in package.json and package-lock.json

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/iden3comm/packers/zkp-default.ts	Added validation for provingMethodAlg parameter and modified circuit ID retrieval logic
package.json	Version bump to 1.39.4
package-lock.json	Version bump to 1.39.4

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The changes removed the type assertion as CircuitId from the circuitId assignment (line 55), but the type assertion is still present when calling loadCircuitData() at line 58. This is inconsistent with the PR's stated goal of "retrieve circuit id from string."

The circuitId variable at line 55 now has type string (from provingMethodAlg.circuitId), but loadCircuitData() expects a CircuitId enum type. The type assertion at line 58 masks this type mismatch. Consider either:

1. Adding validation to ensure the circuitId string is a valid CircuitId enum value before the cast
2. Updating the method signature to accept string if that's the intended design
3. Documenting why the type assertion is safe here

[Copilot]

The validation checks are redundant since the parent class ZKPPacker.pack() already calls getProvingMethod(params.provingMethodAlg) at line 131 in zkp.ts, which will throw an error from the @iden3/js-jwz library if provingMethodAlg is invalid. Additionally, at line 132, it retrieves proving params using params.provingMethodAlg.toString(), which would fail if alg or circuitId were missing since toString() likely depends on these properties.

These validation checks add unnecessary duplication and could lead to inconsistent error messages. Consider removing them and relying on the parent class validation, or consolidating the validation logic in one place.

Suggested change

	if (!(params.provingMethodAlg instanceof ProvingMethodAlg)) {
	throw new Error('provingMethodAlg must be an instance of ProvingMethodAlg');
	}
	if (!params.provingMethodAlg.alg) {
	throw new Error('provingMethodAlg.alg is required');
	}
	if (!params.provingMethodAlg.circuitId) {
	throw new Error('provingMethodAlg.circuitId is required');
	}

[Copilot]

The new validation logic lacks test coverage. While the file is used in tests (e.g., tests/handlers/auth.test.ts:2048), there are no tests that verify:

1. The error thrown when provingMethodAlg is not an instance of ProvingMethodAlg
2. The error thrown when provingMethodAlg.alg is missing
3. The error thrown when provingMethodAlg.circuitId is missing

Consider adding unit tests to cover these validation scenarios and ensure they provide helpful error messages to users.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The validation logic checks for empty strings but doesn't handle cases where alg or circuitId might be non-string values (e.g., null, undefined, or other types). Since these properties come from the ProvingMethodAlg instance which is from an external package, consider adding type checks or using stricter validation like typeof provingMethodAlg.alg !== 'string' || provingMethodAlg.alg.trim() === '' to ensure the values are not just truthy but are actually valid strings.

Suggested change

	if (!provingMethodAlg.alg) {
	throw new Error(`provingMethodAlg.alg is required and must be a non-empty string`);
	}
	if (!provingMethodAlg.circuitId) {
	throw new Error(`provingMethodAlg.circuitId is required and must be a non-empty string`);
	if (
	typeof provingMethodAlg.alg !== 'string' ||
	provingMethodAlg.alg.trim() === ''
	) {
	throw new TypeError('provingMethodAlg.alg is required and must be a non-empty string');
	}
	if (
	typeof provingMethodAlg.circuitId !== 'string' ||
	provingMethodAlg.circuitId.trim() === ''
	) {
	throw new TypeError(
	'provingMethodAlg.circuitId is required and must be a non-empty string'
	);

[Copilot]

The new validation logic lacks test coverage. Consider adding tests that verify the validation correctly handles invalid inputs such as: 1) provingMethodAlg that is not an instance of ProvingMethodAlg, 2) provingMethodAlg with missing or empty alg property, 3) provingMethodAlg with missing or empty circuitId property. Tests should verify that the appropriate error types and messages are thrown in each case. The existing test file tests/iden3comm/zkp.test.ts would be an appropriate location for these tests.

[Copilot]

The validation is called here and then again in the parent class's pack method at line 131 of zkp.ts, resulting in redundant validation. Consider removing this call since the parent class already performs the validation, or remove the validation from the parent class if you want each subclass to control its own validation timing.

Suggested change

this.validateZKPPackerParams(params);