-
Notifications
You must be signed in to change notification settings - Fork 252
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: Make CurveType accessible within Base58PublicKey #453
Conversation
I think also we should decide if the Edit: this would also be awesome to have @matklad chime in on if this was the direction he was thinking when he added this point. The APIs don't seem super clean but it's hard to point out the best long-term plan for this type. |
Yea let's do that in a separate issue/PR. Don't wanna snowball this one |
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.
Seems like there's a small amount of conversation to wrap up and bump from comments, but looking good to me
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.
Yeah, I think @austinabell is correct in that we don't want to have to different PublicKey
types -- there should be only one key we use throughout.
So, something like
#[derive(Debug, Clone, Copy PartialEq)]
pub enum CurveType { ED25519, SECP256K1 }
pub struct PublicKey { data: Vec<u8> }
impl PublicKey {
pub fn curve_type(&self) -> CurveType {
match self.data[0] {
0 => CurveType::ED25519,
1 => CurveType::SECP256K1,
_ => unreachable!(),
}
}
pub fn key_bytes(&self) -> &[u8] { &self.data[1..] }
/// Don't necessary want to expose the fact that at the ABI level we prepend
/// curve type to the key bytes. Might consider making this public later.
pub(crate) fn as_bytes(&self) -> &[u8] { self.data.as_slice() }
}
impl FromStr for PublicKey { ... }
impl BorshSerialize for PublicKey { ... }
impl BorshDeserialize for PublicKey { ... }
impl serde::Serialize for PublicKey { ... }
impl serde::Deserialize for PublicKey { ... }
Note that we probably do want to use a single Vec
as an internal repr, as that's what required by promise_batch_action_add_key_with_full_access
and friends. We just don't want to expose this fact for the user.
This PR moves in the generally right direction, but don't know what would be faster -- merge this and then send a follow up, or to make this existing thing mode ambitious.
Found while reviewing: #458 |
Can you add a changelog entry in this PR or #464 for these changes? Are you planning on merging that into here or separately? |
…/near/near-sdk-rs into feature/public-key-as-base58-key
@austinabell I added the changelog in #464. I think it be best to merge it into here first, then merge this completely |
Use Base58PublicKey for PublicKey instead of Vec<u8>
Addresses #447. This makes it possible to access the curve type without directly accessing the
Vec<u8>
itself