Add validator pem and extend validator signer #657

danielailie · 2025-09-15T12:23:51Z

No description provided.

…r-signer

popenta

looks good, but add unit tests

Copilot

Pull Request Overview

This PR adds a new ValidatorPEM class for handling validator keys in PEM format and extends the ValidatorSigner class with additional functionality including constructor initialization and PEM file loading capabilities.

Introduces ValidatorPEM class for parsing and managing validator keys from PEM files
Extends ValidatorSigner with constructor, sign methods, and static factory method for PEM loading
Adds comprehensive test coverage for ValidatorPEM functionality

Reviewed Changes

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

Show a summary per file

File	Description
src/wallet/validatorSigner.ts	Enhanced with constructor, sign methods, and PEM file loading support
src/wallet/validatorPem.ts	New class for handling validator PEM file operations
src/wallet/validatorPem.spec.ts	Test suite for ValidatorPEM functionality
src/wallet/index.ts	Export added for new ValidatorPEM class
src/testdata/testwallets/validators.pem	Test data file with multiple validator keys

Comments suppressed due to low confidence (1)

src/wallet/validatorSigner.ts:1

There are now two different sign methods and two different trySign methods with different signatures. The original trySign method (lines 17-23) takes a secretKey parameter, while the new trySign method (lines 41-42) uses the instance's secretKey. Consider renaming one of these methods to avoid confusion and maintain consistency.

import { ErrSignerCannotSign } from "../core/errors";

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-16T09:39:07Z

src/wallet/validatorSigner.ts

+    sign(data: Uint8Array): Uint8Array {
+        try {
+            return this.trySign(data);
+        } catch (err) {
+            throw new ErrSignerCannotSign(err as Error);
+        }
+    }


There are now two different sign methods and two different trySign methods with different signatures. The original trySign method (lines 17-23) takes a secretKey parameter, while the new trySign method (lines 41-42) uses the instance's secretKey. Consider renaming one of these methods to avoid confusion and maintain consistency.

Comment is somehow wrong.

But maybe async signUsingPem should be static?

Copilot · 2025-09-16T09:39:07Z

src/wallet/validatorSigner.ts

+            return this.trySign(data);
+        } catch (err) {
+            throw new ErrSignerCannotSign(err as Error);
+        }
+    }
+
+    private trySign(data: Uint8Array): Uint8Array {


There are now two different sign methods and two different trySign methods with different signatures. The original trySign method (lines 17-23) takes a secretKey parameter, while the new trySign method (lines 41-42) uses the instance's secretKey. Consider renaming one of these methods to avoid confusion and maintain consistency.

Suggested change

return this.trySign(data);

} catch (err) {

throw new ErrSignerCannotSign(err as Error);

}

}

private trySign(data: Uint8Array): Uint8Array {

return this.signWithSecretKey(data);

} catch (err) {

throw new ErrSignerCannotSign(err as Error);

}

}

private signWithSecretKey(data: Uint8Array): Uint8Array {

Copilot · 2025-09-16T09:39:08Z

src/wallet/validatorPem.ts

+        for (const entry of entries) {
+            await BLS.initIfNecessary();
+            const secretKey = new ValidatorSecretKey(entry.message);
+            const item = new ValidatorPEM(entry.label, secretKey);
+            resultItems.push(item);
+        }


The BLS.initIfNecessary() call is inside the loop, which means it will be called for each entry. Since this is likely an expensive initialization operation, it should be moved outside the loop to be called only once before processing all entries.

andreibancioiu · 2025-09-16T09:39:44Z

src/wallet/validatorSigner.ts

 * Validator signer (BLS signer)
 */
 export class ValidatorSigner {
+    private secretKey: ValidatorSecretKey;


Can be private readonly.

andreibancioiu · 2025-09-16T09:40:45Z

src/wallet/validatorPem.ts

+    label: string;
+    secretKey: ValidatorSecretKey;


Maybe can be readonly. Should they be private, as well?

There are places where I use the fields if I'll make them private I'll have to add get Methods

andreibancioiu · 2025-09-16T09:41:31Z

src/wallet/validatorPem.ts

+        const resultItems: ValidatorPEM[] = [];
+
+        for (const entry of entries) {
+            await BLS.initIfNecessary();


This can be moved above the loop (opinion).

andreibancioiu · 2025-09-16T09:43:31Z

src/wallet/validatorPem.spec.ts

+
+        // fromFile is async → await
+        const pem = await ValidatorPEM.fromFile(pemPath);
+        pem.save(savedPath);


I think there's no test for toText(), but maybe this one for save() is sufficient?

I added tests for toText, save is already tested via the other tests

andreibancioiu · 2025-09-16T09:47:12Z

src/wallet/validatorSigner.ts

+    sign(data: Uint8Array): Uint8Array {
+        try {
+            return this.trySign(data);
+        } catch (err) {
+            throw new ErrSignerCannotSign(err as Error);
+        }
+    }


Comment is somehow wrong.

But maybe async signUsingPem should be static?

andreibancioiu · 2025-09-16T10:57:33Z

src/wallet/validatorSigner.ts

     * Signs a message.
     */
-    async signUsingPem(pemText: string, pemIndex: number = 0, signable: Buffer | Uint8Array): Promise<Uint8Array> {
+    static async signUsingPem(


Tiny breaking change in theory, but can be in minor version - since, actually, it's a "design correction change" (it's a fix).

as discussed with @andreibancioiu, I think we can mark this as deprecated since we already have the sign method and this does not seem to bring significant benefits.

Add validatorPem and extend validatorSigner

f48c1a5

danielailie self-assigned this Sep 15, 2025

Merge branch 'feat/next' into TOOL-677-add-validator-pem-and-validato…

f69ca88

…r-signer

popenta reviewed Sep 15, 2025

View reviewed changes

Add validator pem tests

5b44df2

andreibancioiu requested a review from Copilot September 16, 2025 09:37

Copilot AI reviewed Sep 16, 2025

View reviewed changes

andreibancioiu reviewed Sep 16, 2025

View reviewed changes

add toText tests and update private fields

3a6839b

andreibancioiu approved these changes Sep 16, 2025

View reviewed changes

andreibancioiu previously approved these changes Sep 16, 2025

View reviewed changes

Mark method with deprecated

d80d3c9

danielailie dismissed andreibancioiu’s stale review via d80d3c9 September 16, 2025 13:07

popenta approved these changes Sep 16, 2025

View reviewed changes

andreibancioiu approved these changes Sep 16, 2025

View reviewed changes

danielailie merged commit cd9757d into feat/next Sep 16, 2025
5 checks passed

danielailie deleted the TOOL-677-add-validator-pem-and-validator-signer branch September 16, 2025 13:35

Add validator pem and extend validator signer #657

Add validator pem and extend validator signer #657

Uh oh!

Conversation

danielailie commented Sep 15, 2025

Uh oh!

popenta left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants