-
Notifications
You must be signed in to change notification settings - Fork 52
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: Import some functions over from near_crypto for PublicKey #265
Conversation
} | ||
|
||
#[test] | ||
fn test_pubkey_borsh_format_change() -> anyhow::Result<()> { |
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.
Hm. Struggling a bit to understand this test.
Is there a case where this would fail that's directly impacted by the struct here? Otherwise, it looks like a data test is all we're trying to do.
So this would be equivalent, no?
let mut data = vec![KeyType::ED25519 as u8];
data.extend(bs58::decode("6E8sCci9badyRkXb3JoRpBj5p8C6Tw41ELDZoiihKEtp").into_vec()?);
let new_key = PublicKey::try_from_slice(data.as_slice())?;
assert_eq!(
new_key.try_to_vec()?,
bs58::decode("16E8sCci9badyRkXb3JoRpBj5p8C6Tw41ELDZoiihKEtp").into_vec()?
);
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.
hmm, now that you mention it, yeah it should be equivalent. I wonder why SDK did it like this in the first place
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.
oh it's because it had Vec<u8>.try_into()
but I just got rid of it in favor of borsh
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.
Ah, gotcha!
Co-authored-by: Miraculous Owonubi <omiraculous@gmail.com>
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.
Lgtm!
} | ||
|
||
#[test] | ||
fn test_pubkey_borsh_format_change() -> anyhow::Result<()> { |
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.
Ah, gotcha!
This just adds a couple convenience methods over from
near_crypto
crate. This was originally requested with justHash
impl in #263, but decided to import a couple more functions as they are convenient to have if people are managing public keys with workspaces