-
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(json_types): implement FromStr for ValidAccountId #391
Conversation
#[derive( | ||
Debug, Clone, PartialEq, PartialOrd, Ord, Eq, BorshDeserialize, BorshSerialize, Serialize, | ||
)] | ||
pub struct ValidAccountId(AccountId); | ||
|
||
impl ValidAccountId { | ||
fn is_valid(&self) -> bool { | ||
is_valid_account_id(&self.0.as_bytes()) | ||
} | ||
pub fn to_string(&self) -> String { |
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, this function does nothing, since to_string()
comes with ToString
which is auto impl from fmt::Display
. I kept it in though because technically it's a breaking change although I don't know if you can actually trigger it because ValidAccountId::to_string(&id)
still works.
Edit: or is this to avoid some unnecessary logic around fmt::Display
?
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, we should remove this once we are ready to to a semver-breaking change.
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.
This makes total sense to me 👍
#[derive( | ||
Debug, Clone, PartialEq, PartialOrd, Ord, Eq, BorshDeserialize, BorshSerialize, Serialize, | ||
)] | ||
pub struct ValidAccountId(AccountId); | ||
|
||
impl ValidAccountId { | ||
fn is_valid(&self) -> bool { | ||
is_valid_account_id(&self.0.as_bytes()) | ||
} | ||
pub fn to_string(&self) -> String { |
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, we should remove this once we are ready to to a semver-breaking change.
/// # Example | ||
/// ``` | ||
/// use near_sdk::AccountId; | ||
/// use near_sdk::json_types::ValidAccountId; | ||
/// | ||
/// let id: AccountId = "bob.near".to_string(); | ||
/// let validated: ValidAccountId = id.parse().unwrap(); | ||
/// ``` |
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.
❤️ , examples are great, we need more of those!
@@ -87,7 +121,7 @@ mod tests { | |||
|
|||
#[test] | |||
fn test_ser() { | |||
let key: ValidAccountId = "alice.near".try_into().unwrap(); | |||
let key: ValidAccountId = "alice.near".parse().unwrap(); |
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.
Cool, this is new to me. 👍🏼
Just saw https://stackoverflow.com/questions/67332443/how-to-convert-accountid-into-validaccountid-in-near-protocol-contracts
and this makes it easier to convert as it doesn't require importing the
TryFrom
orTryInto
traitAlso changes the logic a bit to avoid allocating a
String
from&str
before it's checked as valid.