-
Notifications
You must be signed in to change notification settings - Fork 51
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: Expose account keys #144
Conversation
/// Get the keys of this account. The public key can be retrieved from the secret key. | ||
pub fn keys(&self) -> SecretKey { | ||
SecretKey(self.signer.0.secret_key.clone()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to only return SecretKey
, but I would argue that keys
is not a good name in this case because of plurality. Maybe just key
or secret_key
while leaving the clarification that public key can be retrieved from the secret key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, noting it's probably best if we keep this representation internally, and then this method can return &SecretKey
. Cloning implicitly feels weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I'll re-work how InMemorySigner
is represented internally but shouldn't be too big of a deal to expose it as a &SecretKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright should be good. Ready to be re-reviewed whenever you guys get the chance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not completely sure how this Signer
type. I suppose it has to do with creating TLAs? Maybe you can clarify how you plan to fit this in cleanly?
Reading in between the lines, but I'm guessing you meant to say how does the |
Addresses #142
Exposes the account keys. Might be a little weird that it's exposed just as a
SecretKey
type instead of something like aKeyPair
type. This is due toSecretKey
being aKeyPair
itself containing thePublicKey
. WDYT? Is the return type good enough just asSecretKey
or should we do something like(PublicKey, SecretKey)
?