`AccountType`, add `EthImplicitAccount` #14

staffik · 2023-11-02T09:27:20Z

This PR introduces some changes from near/nearcore#9969 laying groundwork for real protocol changes to be done in a separate PR.

Summary:

Add AccountType enum: NamedAccount, NearImplicitAccount, or EthImplicitAccount.
Parse 40 characters long hexadecimal addresses prefixed with '0x' as EthImplicitAccount.
For now, AccountType::is_implicit() returns false for EthImplicitAccount.

wacban · 2023-11-02T10:01:42Z

I didn't even know about this repo. Where is it used? Do we need to keep it synchronized with nearcore?

wacban

LGTM

birchmd

I also do not know what this repo is used for, but it seems good to keep it up to date with nearcore.

staffik · 2023-11-02T15:22:18Z

@frol Can you look at this PR? How to make a release so that it is accessible from nearcore?

frol · 2023-11-03T09:01:46Z

I didn't even know about this repo. Where is it used? Do we need to keep it synchronized with nearcore?

It has to be used everywhere in Rust code in NEAR ecosystem:

refactor: Integrate extracted and published near-account-id nearcore#10019
refactor: Replace internal account id type with the near-account-id crate near-sdk-rs#1105

near-account-id has been part of nearcore, but since we wanted to stabilize it and have 1.0 release, we extracted it into a standalone repo.

It is all part of a major stabilization effort:

borsh-rs 1.0 is released
near-token-rs is nice and small, and will probably be promoted to 1.0 soon
near-gas-rs is nice and small, and will probably be promoted to 1.0 soon
near-sdk-rs 5.0 release is coming
near-workspaces-rs also targeting 1.0

In order to release 1.0, we need to ensure that the underlying dependencies are stable, so near-account-id, was extracted, and near-primitives-core might also be extracted at a certain point (though, I won't even try to convince nearcore to use the primitives, so probably that will be a standalone effort)

wacban · 2023-11-03T09:19:23Z

@frol Thanks for the explanation. As nearcore developers we need a way to iterate quickly on the repo. I don't know about 1.0 but need to get that PR in, released and synced to nearcore quickly. Can you confirm that we are free to work on this repo as we would on nearcore (e.i. the same review process), please? Can you share instructions on how to release and sync to nearcore please? Ideally in form of some documentation but a comment here would be a good start too.

frol · 2023-11-03T11:18:44Z

src/account_id_ref.rs

+/// Enum representing possible types of accounts.
+#[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,
+            Self::NamedAccount => false,
+        }
+    }
+}


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

@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.

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)

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 => {}, } ... }

@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.

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

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

frol · 2023-11-03T11:20:49Z

src/account_id_ref.rs

    /// ```
-    pub fn is_implicit(&self) -> bool {
+    pub fn is_near_implicit(&self) -> bool {


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?

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.

frol · 2023-11-03T11:27:18Z

As nearcore developers we need a way to iterate quickly on the repo. I don't know about 1.0 but need to get that PR in, released and synced to nearcore quickly.

Well, if you break the ecosystem fast it does not really help, so I encourage nearcore team to take it into consideration as well. near-account-id is the primitive that needs to be the same across the board.

Can you confirm that we are free to work on this repo as we would on nearcore (e.i. the same review process), please?

It is slightly different here due to the focus on delivering a reusable crate (well, it should have been treated this way in nearcore, but it was not). The difference is that we need to make a conscious decision about breaking changes (in nearcore only protocol changes were considered breaking).

Can you share instructions on how to release and sync to nearcore please?

It is easy - after this PR is merged to main branch, automation will kick in, and create a release PR, once that PR is reviewed and merged in main branch, automation will take care of publishing the release.

wacban · 2023-11-03T11:43:55Z

@frol Thanks!

Well, if you break the ecosystem fast it does not really help, so I encourage nearcore team to take it into consideration as well.

I don't think breaking the ecosystem at any speed is helpful but I would argue that breaking it fast is better than breaking is slow. Then at least we can recover equally fast.

near-account-id is the primitive that needs to be the same across the board.

That makes sense but it's still not clear to me what is the process for making changes to this new repo. In particular this new feature will be released with a protocol version upgrade while this repo doesn't seem to be aware of it.

JFYI this PR should be mostly a no-op, preparatory work. In a latter PR @staffik will do the work described in TODOs (make is_implicit return true for the ethereum account types).

The difference is that we need to make a conscious decision about breaking changes (in nearcore only protocol changes were considered breaking).

Can you describe this process please? This project will be a breaking change so we need to know how to handle it.

automation will take care of publishing the release.

Brilliant, thanks!

src/account_id_ref.rs

frol

Overall, looks good to me. Just one discussion point regarding is_implicit, see below.

frol · 2023-11-03T17:12:12Z

src/account_id_ref.rs

+            // TODO(eth-implicit) change to true later, see https://github.com/near/nearcore/issues/10018
+            Self::EthImplicitAccount => false,


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.

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.

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.

staffik · 2023-11-03T20:20:56Z

@frol or @wacban, I need approve again because I pushed a commit now.

## 🤖 New release * `near-account-id`: 1.0.0-alpha.1 -> 1.0.0-alpha.2 (⚠️ API breaking changes) ### ⚠️ `near-account-id` breaking changes ``` --- failure inherent_method_missing: pub method removed or renamed --- Description: A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.24.2/src/lints/inherent_method_missing.ron Failed in: AccountIdRef::is_implicit, previously in file /tmp/.tmpEzh7Ck/near-account-id/src/account_id_ref.rs:160 ``` <details><summary>Changelog</summary> <blockquote> ## [1.0.0-alpha.2](v1.0.0-alpha.1...v1.0.0-alpha.2) - 2023-11-03 ### Other - `AccountType`, add `EthImplicitAccount` ([#14](#14)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). Signed-off-by: Vlad Frolov <304265+frol@users.noreply.github.com> Co-authored-by: Vlad Frolov <304265+frol@users.noreply.github.com>

frol · 2023-11-03T22:00:22Z

@staffik @wacban 1.0.0-alpha.2 is published!

Part of #10018. This PR introduces some changes from #9969 laying groundwork for real protocol changes to be done in a separate PR. Some changes might look redundant (especially `match receiver_id.get_account_type() { ...`) but they will make it easier to read changes done in the next PR. Changes done in [near-account-id 1.0.0-alpha.2](near/near-account-id-rs#14): - Add `AccountType` enum: `NamedAccount`, `NearImplicitAccount`, or `EthImplicitAccount`. - Parse 40 characters long hexadecimal addresses prefixed with `'0x'` as `EthImplicitAccount`. - `AccountType::is_implicit()` returns true for both `NearImplicitAccount` and `EthImplicitAccount`. Summary of this PR: - Bump version of `near-account-id` to `1.0.0-alpha.2`. - Do not use `is_implicit()` for now, as we do not want to treat `EthImplicitAccount` as implicit as for now. - Add `derive_account_id_from_public_key` function that currently only supports `ED25519` key type and returns hex-encoded copy of the key. - Refactor/rename tests a bit.

staffik force-pushed the eth-implicit-account branch from 4237c40 to 73c41e2 Compare November 2, 2023 09:38

staffik changed the title ~~Setup for ETH-implicit accounts~~ AccountType, add EthImplicitAccount Nov 2, 2023

staffik changed the title ~~AccountType, add EthImplicitAccount~~ AccountType, add EthImplicitAccount Nov 2, 2023

This was referenced Nov 2, 2023

[ProjectTracking]: Ethereum address implicit accounts near/nearcore#10018

Closed

Setup for ETH-implicit accounts near/nearcore#10020

Merged

Add AccountType, ETH-implicit accounts

08da7d8

staffik force-pushed the eth-implicit-account branch from 73c41e2 to 08da7d8 Compare November 2, 2023 09:44

staffik requested review from frol and wacban November 2, 2023 09:47

wacban requested a review from birchmd November 2, 2023 09:56

wacban previously approved these changes Nov 2, 2023

View reviewed changes

birchmd previously approved these changes Nov 2, 2023

View reviewed changes

frol requested changes Nov 3, 2023

View reviewed changes

wacban requested a review from frol November 3, 2023 11:34

frol reviewed Nov 3, 2023

View reviewed changes

src/account_id_ref.rs Show resolved Hide resolved

frol mentioned this pull request Nov 3, 2023

Prepare for stability (1.0 release) #5

Open

7 tasks

Move is_near_implicit and is_eth_implicit to validation.rs

d3b440e

staffik dismissed stale reviews from birchmd and wacban via d3b440e November 3, 2023 13:45

staffik requested a review from frol November 3, 2023 13:46

frol previously approved these changes Nov 3, 2023

View reviewed changes

Treat EthImplicit as implicit

07bf40f

staffik dismissed frol’s stale review via 07bf40f November 3, 2023 20:19

wacban approved these changes Nov 3, 2023

View reviewed changes

staffik merged commit 2efa2d9 into main Nov 3, 2023
5 checks passed

staffik deleted the eth-implicit-account branch November 3, 2023 20:27

frol mentioned this pull request Nov 3, 2023

chore: release v1.0.0-alpha.2 #16

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`AccountType`, add `EthImplicitAccount` #14

`AccountType`, add `EthImplicitAccount` #14

staffik commented Nov 2, 2023

wacban commented Nov 2, 2023

wacban left a comment

birchmd left a comment

staffik commented Nov 2, 2023

frol commented Nov 3, 2023

wacban commented Nov 3, 2023

frol Nov 3, 2023

wacban Nov 3, 2023

frol Nov 3, 2023

frol Nov 3, 2023 •

edited

Loading

staffik Nov 3, 2023

frol Nov 3, 2023

staffik Nov 3, 2023

frol Nov 3, 2023

wacban Nov 3, 2023

frol commented Nov 3, 2023

wacban commented Nov 3, 2023

frol left a comment

frol Nov 3, 2023

staffik Nov 3, 2023 •

edited

Loading

wacban Nov 3, 2023

wacban Nov 3, 2023

staffik commented Nov 3, 2023

frol commented Nov 3, 2023

		// TODO(eth-implicit) change to true later, see https://github.com/near/nearcore/issues/10018
		Self::EthImplicitAccount => false,

AccountType, add EthImplicitAccount #14

AccountType, add EthImplicitAccount #14

Conversation

staffik commented Nov 2, 2023

wacban commented Nov 2, 2023

wacban left a comment

Choose a reason for hiding this comment

birchmd left a comment

Choose a reason for hiding this comment

staffik commented Nov 2, 2023

frol commented Nov 3, 2023

wacban commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frol Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frol commented Nov 3, 2023

wacban commented Nov 3, 2023

frol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staffik Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staffik commented Nov 3, 2023

frol commented Nov 3, 2023

`AccountType`, add `EthImplicitAccount` #14

`AccountType`, add `EthImplicitAccount` #14

frol Nov 3, 2023 •

edited

Loading

staffik Nov 3, 2023 •

edited

Loading