Skip to content
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.

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

Improved key generation with factory pattern #120

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Improved key generation with factory pattern #120

merged 1 commit into from
Feb 23, 2024

Conversation

DanielBoye
Copy link
Contributor

Fixes a TODO in the code:

// TODO: This can be improved further with a factory pattern

Motivation

Updating to a factory pattern is smart to decouple object creation from its usage in our key generation logic. This change addresses the challenge of supporting multiple key types (ECDSA and BLS) in a scalable way.

Solution

The first I did was to include a KeyGenerator interface and two concrete implementations for ECDSA and BLS key types. The factory function, NewKeyGenerator, dynamically selects the appropriate generator based on input parameters, simplifying the generate function.

Open questions

Would someone mind if we wrote unit tests for the newly implemented factory pattern in our key generation system, particularly focusing on the dynamic selection logic of the NewKeyGenerator function and ensuring each key generator (ECDSA and BLS) behaves as expected under various input conditions? Since as for now the egnkey does not have any test cases.

@DanielBoye DanielBoye changed the title improved with factory pattern Improved key generation with factory pattern Feb 19, 2024
@samlaf samlaf requested a review from shrimalmadhur February 23, 2024 12:42
@samlaf
Copy link
Collaborator

samlaf commented Feb 23, 2024

@shrimalmadhur can you take a look at this? I believe you wrote the egnkey util?

@shrimalmadhur shrimalmadhur merged commit d145eb2 into Layr-Labs:master Feb 23, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants