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

feat: add signBuffer function #193

Open
wants to merge 3 commits into
base: @exodus/[email protected]
Choose a base branch
from

Conversation

feri42
Copy link
Contributor

@feri42 feri42 commented Dec 13, 2024

Copy the signing function from tx-signer, so that we don't need that much keychain detail knowledge in tx-signer. Looking for Conept ACK (initially).

@feri42 feri42 changed the base branch from master to @exodus/[email protected] December 13, 2024 15:31
@feri42 feri42 self-assigned this Dec 13, 2024
@feri42 feri42 requested a review from fboucquez December 13, 2024 15:32
@feri42 feri42 changed the title feat: add sign buffer function feat: add signBuffer function Dec 13, 2024
Copy link
Contributor

@kewde kewde left a comment

Choose a reason for hiding this comment

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

Concept ACK

@sparten11740
Copy link
Contributor

Concept ACK

}
}

async getPublicKey({ seedId, keyId }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have external consumers that need this or should we make it private for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would be nice to have this function so that we don't need to use exportKey in places where we know we will never need the private keys or xpubs. Making it private will serve no purpose. I can remove it if it seems overkill.

Copy link
Collaborator

@mvayngrib mvayngrib Jan 2, 2025

Choose a reason for hiding this comment

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

the current one is easy to use though:

const { publicKey } = await keychain.exportKey({ seedId, keyId })
// now we're adding this, which seems super similar
const publicKey = await keychain.getPublicKey({ seedId, keyId })

not strongly opposed to adding it though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above is exactly the same. Only getPublicKey emphasizes we area only interested in the public key. And exportKey does cause xpubs to be transported over IPC, even if they are not used afterwards. Also, it is just too easy to add exportPrivate: true to exportKey. I just think it could improve security and readability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good, let's keep it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants