Skip to content

validate provingMethodAlg #389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
volodymyr-basiuk wants to merge 4 commits into
base: main
Choose a base branch
from
Open

validate provingMethodAlg #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
53a3cbd
retrive circuit id from string in default zkp packer
volodymyr-basiuk Jan 23, 2026
22a2b45
validate provingMethodAlg
volodymyr-basiuk Jan 23, 2026
3fbdc01
validateZKPPackerParams
volodymyr-basiuk Jan 23, 2026
681f588
fix verification
volodymyr-basiuk Jan 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@0xpolygonid/js-sdk",
"version": "1.39.3",
"version": "1.39.4",
"description": "SDK to work with Polygon ID",
"source": "./src/index.ts",
"exports": {
Expand Down
7 changes: 3 additions & 4 deletions src/iden3comm/packers/zkp-default.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,9 @@ export class DefaultZKPPacker extends ZKPPacker {
* @throws If the circuit data (proving key or wasm) is not found for the given circuit ID.
*/
async pack(payload: Uint8Array, params: ZKPPackerParams): Promise<Uint8Array> {
const provingMethodAlg = params.provingMethodAlg;
const circuitId = provingMethodAlg.circuitId as CircuitId;
const provingParamsKey = provingMethodAlg.toString();

this.validateZKPPackerParams(params);

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

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

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);

Copilot uses AI. Check for mistakes.
const provingParamsKey = params.provingMethodAlg.toString();
const circuitId = params.provingMethodAlg.circuitId;
if (!this.provingParamsMap.has(provingParamsKey)) {
const { provingKey, wasm } = await this.circuitStorage.loadCircuitData(
circuitId as CircuitId,
Comment on lines +47 to 50

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Expand Down
14 changes: 14 additions & 0 deletions src/iden3comm/packers/zkp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ export class ZKPPacker implements IPacker {
* @returns `Promise<Uint8Array>`
*/
async pack(payload: Uint8Array, params: ZKPPackerParams): Promise<Uint8Array> {
this.validateZKPPackerParams(params);
const provingMethod = await getProvingMethod(params.provingMethodAlg);
const provingParams = this.provingParamsMap.get(params.provingMethodAlg.toString());

Expand Down Expand Up @@ -229,6 +230,19 @@ export class ZKPPacker implements IPacker {
!alg?.length || alg.some((a) => supportedAlgArr.includes(a as AcceptJwzAlgorithms));
return algSupported && circuitIdSupported;
}

protected validateZKPPackerParams(value: ZKPPackerParams) {
const { provingMethodAlg } = value;
if (!(provingMethodAlg instanceof ProvingMethodAlg)) {
throw new TypeError('provingMethodAlg must be an instance of ProvingMethodAlg');
}
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`);
Comment on lines +239 to +243

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
}
Comment on lines +234 to +245

Copilot AI Jan 23, 2026

Copy link

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}

const verifySender = async (token: Token, msg: BasicMessage): Promise<void> => {
Expand Down
Loading