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

chore::> Derive public key from private key using crypto package #594

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

PsychoPunkSage
Copy link
Contributor

Addresses #590 pointer 4

@PsychoPunkSage PsychoPunkSage marked this pull request as draft July 11, 2024 16:33
@PsychoPunkSage PsychoPunkSage marked this pull request as ready for review July 11, 2024 16:47
@PsychoPunkSage
Copy link
Contributor Author

@thiagodeev
were you talking about something like this??

Copy link
Collaborator

@thiagodeev thiagodeev left a comment

Choose a reason for hiding this comment

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

Yes @PsychoPunkSage! That's exactly what I had in mind.
But now, I believe we can do better. Sorry about that, but I'm going to ask you to take another approach.

account/keystore.go Outdated Show resolved Hide resolved
@PsychoPunkSage
Copy link
Contributor Author

PsychoPunkSage commented Jul 15, 2024

Hmm,
Okay... I thought about this approach when I started working on the issue.. but it was not that appealing to me to create all the fn again. Instead I was thinking to modify the existing func and check whether user is passing a pub_key or not.. if not then do the required computation...

This approach will surely make things clean but don't you think it will be redundant??
What do you think @thiagodeev ??

@thiagodeev
Copy link
Collaborator

This was also my first intention on this feature, but I realized that the MemKeyStore is not a simple privateKey pointer, is literally a "key store", that can store multiple keys and have multiple use cases. If we change it directly, users who are using this feature could be in trouble.
Being so, creating a new abstraction specific to our intention (a Signer), having a new or existing MemKeyStore linked (covering both use cases) and specific methods to it it's a valid approach IMO. What do you think?

@PsychoPunkSage
Copy link
Contributor Author

Mmmm,
Yeah I think for now we can do that, well, it can always be modified in future by any new contributor.
So, I will separate it...

@PsychoPunkSage
Copy link
Contributor Author

@thiagodeev have a look

Copy link
Collaborator

@thiagodeev thiagodeev left a comment

Choose a reason for hiding this comment

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

Great job @PsychoPunkSage!
I'll ask one general thing: please put descriptions in the code (package, functions, and methods).
Besides that, I left a few comments

account/signer.go Outdated Show resolved Hide resolved
account/signer.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@thiagodeev thiagodeev left a comment

Choose a reason for hiding this comment

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

Hey @PsychoPunkSage, left some comments.
An observation: you need to put golang doc code comments

account/signer.go Outdated Show resolved Hide resolved
account/signer.go Outdated Show resolved Hide resolved
account/signer.go Outdated Show resolved Hide resolved
account/signer.go Outdated Show resolved Hide resolved
@PsychoPunkSage
Copy link
Contributor Author

Hi @thiagodeev
Did required changes. Please have a look whenever you find time.

Copy link
Collaborator

@thiagodeev thiagodeev left a comment

Choose a reason for hiding this comment

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

Hey @PsychoPunkSage, left a few comments, otherwise looks good!

account/signer.go Outdated Show resolved Hide resolved
account/signer.go Show resolved Hide resolved
@PsychoPunkSage
Copy link
Contributor Author

@thiagodeev
Did you find time to review the PR?

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.

2 participants