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.

Sign up for GitHub

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

AccountType, add EthImplicitAccount #14

Merged
merged 3 commits into from
Nov 3, 2023
Merged

AccountType, add EthImplicitAccount #14

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08da7d8
Add AccountType, ETH-implicit accounts
staffik Nov 2, 2023
d3b440e
Move is_near_implicit and is_eth_implicit to validation.rs
staffik Nov 3, 2023
07bf40f
Treat EthImplicit as implicit
staffik Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 116 additions & 11 deletions src/account_id_ref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,28 @@ use crate::{AccountId, ParseAccountError};
#[cfg_attr(feature = "abi", derive(schemars::JsonSchema, BorshSchema))]
pub struct AccountIdRef(pub(crate) str);

/// Enum representing possible types of accounts.
frol marked this conversation as resolved.
Show resolved Hide resolved
#[derive(PartialEq)]
pub enum AccountType {
/// Any valid account, that is neither NEAR-implicit nor ETH-implicit.
NamedAccount,
/// An account with 64 characters long hexadecimal address.
NearImplicitAccount,
/// An account which address starts with '0x', followed by 40 hex characters.
EthImplicitAccount,
}

impl AccountType {
pub fn is_implicit(&self) -> bool {
match &self {
Self::NearImplicitAccount => true,
// TODO(eth-implicit) change to true later, see https://github.com/near/nearcore/issues/10018
Self::EthImplicitAccount => false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would look like a breaking change for near-account-id crate, so I would just return true here from the get-go. Any strong objections? It would be a separate challenge for the future if NEAR Protocol changes account-id rules drastically.

Copy link
Contributor Author

@staffik staffik Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I can get around that within the nearcore repo, to keep this repo stable. @wacban wdyt? There are few places in the nearcore repo where we call is_implicit, so I can replace them with get_account_type() == NearImplicit and mark as TODO to change it back in the next PR.

Copy link

Choose a reason for hiding this comment

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

SGTM

Copy link

Choose a reason for hiding this comment

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

We still need to work out what to do about the existing named accounts that have the eth account format. I would still go ahead with this PR as agreed because it's blocking everything elsewhere.

Self::NamedAccount => false,
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the motivation for this extra enum? I feel that is_eth_implicit and is_near_implicit + is_implicit would be enough.

Copy link

Choose a reason for hiding this comment

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

@frol please see the links in the pr description for more details about this project

The reason for using an enum here is good software development practice. We want to be explicit about the different types of accounts and force ourselves and future developers to be aware and correctly handle the different types of account. Additionally this is more future proof if we ever need to add more account types. This was also discussed on the original PR in nearcore.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We want to be explicit about the different types of accounts and force ourselves and future developers to be aware and correctly handle the different types of account.

If that is the intent, we should remove the is_near_implicit and is_eth_implicit methods from the AccountIdRef type as having two different ways to do the same thing is not helping here and does not really force users to handle all the cases.

Just for the future reference, I will link the prior discussion: near/nearcore#9969 (comment)

Copy link
Collaborator

@frol frol Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

P.S. Given that "renaming" is_implicit to is_near_implicit is already a breaking change, removing all those is*implicit methods might be just fine.

Just to model the usage here:

BEFORE

fn send_near(receiver_id: AccountIdRef) {
    if !receiver_id.is_implicit() {
        assert!(is_account_recorded_on_chain(receiver_id), "you cannot send tokens to non-existing non-implicit accounts");
    }
    ...
}

AFTER renaming to is_near_implicit and naive (most probably invalid, right?) changes:

fn send_near(receiver_id: AccountIdRef) {
    if !receiver_id.is_near_implicit() {
        assert!(is_account_recorded_on_chain(receiver_id), "you cannot send tokens to non-existing non-implicit accounts");
    }
    ...
}

AFTER if no is_*implicit methods:

fn send_near(receiver_id: AccountIdRef) {
    if let AccountType::NamedAccount = receiver_id.get_account_type() {
        assert!(is_account_recorded_on_chain(receiver_id), "you cannot send tokens to non-existing non-implicit accounts");
    }
    ...
}

OR

fn send_near(receiver_id: AccountIdRef) {
    match receiver_id.get_account_type() {
        AccountType::NamedAccount => {
            assert!(is_account_recorded_on_chain(receiver_id), "you cannot send tokens to non-existing non-implicit accounts");
        }
        AccountType::NearImplicitAccount => {},
        AccountType::EthImplicitAccount => {},
    }
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frol Would it be ok if I just rename pub fn is_eth_implicit to fn is_eth_implicit and pub fn is_near_implicit to fn is_near_implicit? I would remove docstrings with tests from is_eth_implicit and is_near_implicit, and add corresponding docstring with tests for get_account_type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@staffik Do you mean removing pub from is_eth_implicit and is_near_implicit? I would consider moving them to validation.rs file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frol Yes. Sure, I will try to move them to validation.rs file.


impl AccountIdRef {
/// Shortest valid length for a NEAR Account ID.
pub const MIN_LEN: usize = crate::validation::MIN_LEN;
Expand Down Expand Up @@ -140,6 +162,29 @@ impl AccountIdRef {
.map_or(false, |s| !s.contains('.'))
}

/// Returns `true` if the `AccountId` is a 40 characters long hexadecimal prefixed with '0x'.
///
/// See [Implicit-Accounts](https://docs.near.org/docs/concepts/account#implicit-accounts).
///
/// ## Examples
///
/// ```
/// use near_account_id::AccountId;
///
/// let alice: AccountId = "alice.near".parse().unwrap();
/// assert!(!alice.is_eth_implicit());
///
/// let rando = "0xb794f5ea0ba39494ce839613fffba74279579268"
/// .parse::<AccountId>()
/// .unwrap();
/// assert!(rando.is_eth_implicit());
/// ```
pub fn is_eth_implicit(&self) -> bool {
self.len() == 42
&& self.0.starts_with("0x")
&& self.0[2..].as_bytes().iter().all(|b| matches!(b, b'a'..=b'f' | b'0'..=b'9'))
}

/// Returns `true` if the `AccountId` is a 64 characters long hexadecimal.
///
/// See [Implicit-Accounts](https://docs.near.org/docs/concepts/account#implicit-accounts).
Expand All @@ -150,21 +195,31 @@ impl AccountIdRef {
/// use near_account_id::AccountId;
///
/// let alice: AccountId = "alice.near".parse().unwrap();
/// assert!(!alice.is_implicit());
/// assert!(!alice.is_near_implicit());
///
/// let rando = "98793cd91a3f870fb126f66285808c7e094afcfc4eda8a970f6648cdf0dbd6de"
/// .parse::<AccountId>()
/// .unwrap();
/// assert!(rando.is_implicit());
/// assert!(rando.is_near_implicit());
/// ```
pub fn is_implicit(&self) -> bool {
pub fn is_near_implicit(&self) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would consider keeping is_implicit (= is_near_implicit || is_eth_implicit) to denote that account does not need to be created explicitly with CreateAccount action. What do you think?

Copy link

Choose a reason for hiding this comment

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

Please see the original pr for more details.

TLDR - we want to be explicit about type different types of implicit account. In order to not impede developer productivity we are adding the is_implict method to the account type enum so it is still reasonably convenient to use while making developers aware of the different account types.

self.0.len() == 64
&& self
.as_bytes()
.iter()
.all(|b| matches!(b, b'a'..=b'f' | b'0'..=b'9'))
}

pub fn get_account_type(&self) -> AccountType {
if self.is_eth_implicit() {
return AccountType::EthImplicitAccount;
}
if self.is_near_implicit() {
return AccountType::NearImplicitAccount;
}
AccountType::NamedAccount
}

/// Returns `true` if this `AccountId` is the system account.
///
/// See [System account](https://nomicon.io/DataStructures/Account.html?highlight=system#system-account).
Expand Down Expand Up @@ -457,6 +512,9 @@ mod tests {
"alex-skidanov",
"b-o_w_e-n",
"no_lols",
// ETH-implicit account
"0xb794f5ea0ba39494ce839613fffba74279579268",
// NEAR-implicit account
"0123456789012345678901234567890123456789012345678901234567890123",
];
for account_id in ok_top_level_account_ids {
Expand Down Expand Up @@ -581,6 +639,11 @@ mod tests {
"123456789012345678901234567890123456789012345678901234567890",
"1234567890.123456789012345678901234567890123456789012345678901234567890",
),
(
"b794f5ea0ba39494ce839613fffba74279579268",
// ETH-implicit account
"0xb794f5ea0ba39494ce839613fffba74279579268",
),
("aa", "ъ@aa"),
("aa", "ъ.aa"),
];
Expand All @@ -598,40 +661,82 @@ mod tests {
}

#[test]
fn test_is_account_id_64_len_hex() {
let valid_64_len_hex_account_ids = &[
fn test_is_account_id_near_implicit() {
let valid_near_implicit_account_ids = &[
"0000000000000000000000000000000000000000000000000000000000000000",
"6174617461746174617461746174617461746174617461746174617461746174",
"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
"20782e20662e64666420482123494b6b6c677573646b6c66676a646b6c736667",
];
for valid_account_id in valid_64_len_hex_account_ids {
for valid_account_id in valid_near_implicit_account_ids {
assert!(
matches!(
AccountIdRef::new(valid_account_id),
Ok(account_id) if account_id.is_implicit()
Ok(account_id) if account_id.get_account_type() == AccountType::NearImplicitAccount
),
"Account ID {} should be valid 64-len hex",
valid_account_id
);
}

let invalid_64_len_hex_account_ids = &[
let invalid_near_implicit_account_ids = &[
"000000000000000000000000000000000000000000000000000000000000000",
"6.74617461746174617461746174617461746174617461746174617461746174",
"012-456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
"fffff_ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
"oooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo",
"00000000000000000000000000000000000000000000000000000000000000",
];
for invalid_account_id in invalid_64_len_hex_account_ids {
for invalid_account_id in invalid_near_implicit_account_ids {
assert!(
!matches!(
AccountIdRef::new(invalid_account_id),
Ok(account_id) if account_id.is_implicit()
Ok(account_id) if account_id.get_account_type() == AccountType::NearImplicitAccount
),
"Account ID {} is not a NEAR-implicit account",
invalid_account_id
);
}
}

#[test]
fn test_is_account_id_eth_implicit() {
let valid_eth_implicit_account_ids = &[
"0x0000000000000000000000000000000000000000",
"0x6174617461746174617461746174617461746174",
"0x0123456789abcdef0123456789abcdef01234567",
"0xffffffffffffffffffffffffffffffffffffffff",
"0x20782e20662e64666420482123494b6b6c677573",
];
for valid_account_id in valid_eth_implicit_account_ids {
assert!(
matches!(
valid_account_id.parse::<AccountId>(),
Ok(account_id) if account_id.get_account_type() == AccountType::EthImplicitAccount
),
"Account ID {} should be valid 42-len hex, starting with 0x",
valid_account_id
);
}

let invalid_eth_implicit_account_ids = &[
"04b794f5ea0ba39494ce839613fffba74279579268",
"0x000000000000000000000000000000000000000",
"0x6.74617461746174617461746174617461746174",
"0x012-456789abcdef0123456789abcdef01234567",
"0xfffff_ffffffffffffffffffffffffffffffffff",
"0xoooooooooooooooooooooooooooooooooooooooo",
"0x00000000000000000000000000000000000000000",
"0000000000000000000000000000000000000000000000000000000000000000",
];
for invalid_account_id in invalid_eth_implicit_account_ids {
assert!(
!matches!(
invalid_account_id.parse::<AccountId>(),
Ok(account_id) if account_id.get_account_type() == AccountType::EthImplicitAccount
),
"Account ID {} is not an implicit account",
"Account ID {} is not an ETH-implicit account",
invalid_account_id
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,5 @@ mod test_data;
mod validation;

pub use account_id::AccountId;
pub use account_id_ref::AccountIdRef;
pub use account_id_ref::{AccountIdRef, AccountType};
pub use errors::{ParseAccountError, ParseErrorKind};
Loading