refactor(types): change account id to newtype and deprecate ValidAccountId #448

austinabell · 2021-06-24T15:38:06Z

Definitely more involved than I would have liked and it's unclear what is the best way to initialize an AccountId but here is what changes:

Changes AccountId from a String alias to a newtype which performs the validation, JSON/borsh serialization, and other necessary interactions
Removes ValidAccountId
- I was planning on aliasing AccountId for this, but it caused a lot of confusing errors linking to it instead of AccountId, and should probably be removed sooner than later

This is quite a large breaking change, because using String is quite nested within everything, and interactions from ValidAccountId specifically will cause breakages with existing contracts.

closes #446

…untId

near-sdk/src/types/account_id.rs

matklad · 2021-06-24T15:57:49Z

examples/status-message/src/lib.rs

-            env::is_valid_account_id(account_id.as_bytes()),
-            "Given account ID is invalid"
-        );
+    pub fn get_status(&self, account_id: AccountId) -> Option::<String> {


Suggested change

pub fn get_status(&self, account_id: AccountId) -> Option::<String> {

pub fn get_status(&self, account_id: AccountId) -> Option<String> {

Or is the turbofish actually required by the macro?

It is required, I haven't dug into why specifically it's needed yet

it's needed because of the metadata macro generation, because it calls Option::<String>::schema_container()

near-sdk-sim/src/user.rs

near-sdk/src/types/account_id.rs

mikedotexe

Looks quite thorough. And I believe you'd addressed what Kladov has brought up. My only silly suggestion is to have the one error begin with Invalid instead of In but 🤷🏼

frol · 2021-06-25T13:00:45Z

Hey-hey! Node Interfaces team is working on a similar thing in nearcore (@miraclx has it in a branch state for now: near/nearcore@master...miraclx:strict-account-id-validity) and even decided to create a separate crate out of it (near-account-id), so we can merge our efforts here.

austinabell · 2021-06-25T13:24:14Z

Hey-hey! Node Interfaces team is working on a similar thing in nearcore (@miraclx has it in a branch state for now: near/nearcore@master...miraclx:strict-account-id-validity) and even decided to create a separate crate out of it (near-account-id), so we can merge our efforts here.

We can't use that implementation in its current state because it pulls in a few dependencies and uses regex for validation which is too bulky for the SDK (especially since an OKR for the last and upcoming quarter is minimizing contract size)

austinabell · 2021-06-25T15:19:49Z

Because this is such a breaking change, it'd be great to get input from others.

Also, the primary remaining design decision is if we change the underlying type to Box<[u8]> which on one hand could allow for smaller code sizes but also does not hold up the compiler guarantee that it will always be utf8 bytes. It would also require calling from_utf8 on the bytes for every reference, and I'm not sure how that gets optimized.

frol · 2021-06-25T17:39:41Z

We can definitely onboard this implementation as I also don't feel very good about pulling the whole regex machinery to validate the account ID format.

Have you measured the difference of Box<[u8]> vs Box<str>?

austinabell · 2021-06-25T18:10:56Z

We can definitely onboard this implementation as I also don't feel very good about pulling the whole regex machinery to validate the account ID format.

Have you measured the difference of Box<[u8]> vs Box<str>?

The size of memory is the same, not sure about the compiled code (as it depends a lot on the use case). I am also noticing when switching to the Boxed versions that BorshSchema is broken for them. I can look into fixing this if it's possible.

austinabell · 2021-06-25T18:48:49Z

@frol wrt above near/borsh-rs#36 which is just a benefit to borsh, but would allow these alternatives to be used here (I checked)

matklad

Looks good to me, left a whole bunch of nits, but only three serious comments:

deriving BorshDeserialize for AccountId is wrong
still not sure that extra conversions are worth it, but don't see anything particularly problematic there
i don't undrestand the reason for AsRef-ing promise API

HELP.md

matklad · 2021-07-05T12:53:48Z

examples/cross-contract-high-level/src/lib.rs

@@ -7,6 +7,7 @@ use near_sdk::{
    //    callback_vec,


Why do we need cross_contract_high_level.wasm and cross_contract_low_level.wasm blobs here, and why they grew by several kbs?

matklad · 2021-07-05T12:55:42Z

examples/cross-contract-low-level/tests/general.rs

@@ -39,7 +39,7 @@ fn check_promise() {
    let (master_account, contract, _alice) = init(to_yocto("10000"));
    let res = view!(contract.promise_checked());
    assert_eq!(res.unwrap_json::<bool>(), false);
-    let status_id = "status".to_string();
+    let status_id: near_sdk::AccountId = "status".parse().unwrap();


I wonder if these unwraps are to blame for additional bloat?

I'll check around the code size in a bit. Committing the wasm files was a mistake and how I built them also causes a difference on master. Definitely want to look into making it part of the CI in some way to check differences

Agreed, and as we discussed in this morning's meeting, this is capture in #465

near-sdk/src/types/account_id.rs

near-sdk/src/types/mod.rs

near-sdk/src/types/account_id.rs

austinabell · 2021-07-05T22:11:51Z

deriving BorshDeserialize for AccountId is wrong

Think I've addressed everything else, but curious why you think this is wrong? Any contract using the old AccountId will have the ID borsh serialized as a string, and it would be a state-breaking change when they update the SDK. What would you suggest is a better format? I don't see how string would be a bad borsh format either, or am I missing something about borsh storing more metadata for strings over byte slices?

matklad · 2021-07-06T11:10:58Z

why you think this is wrong?

It allows creating AccoundId which is not valid

    #[test]
    fn deser_checks_validity() {
        let r1 = "💩".parse::<AccountId>();
        assert!(r1.is_err()); // correct, 💩 is not a valid account name

        let bytes = "💩".try_to_vec().unwrap();
        let r2 = AccountId::try_from_slice(&bytes);
        let acc = r2.unwrap();
        assert_eq!(acc.as_str(), "💩") // BUG, 💩 is treated as a valid account
    }

As a rule of thumb, type's invariants should be checked in every constructor, and deserialization provides an extra constructor.

We shouldn't change serialization format, but we should add an extra validation to deserialization.

austinabell · 2021-07-06T14:33:43Z

As a rule of thumb, type's invariants should be checked in every constructor, and deserialization provides an extra constructor.

We shouldn't change serialization format, but we should add an extra validation to deserialization.

Ah, of course, oops

…unt_id_newtype

near-sdk/src/types/account_id.rs

near-sdk/src/utils/mod.rs

refactor(types): change account id to newtype and deprecate ValidAcco…

3ad2d55

…untId

matklad reviewed Jun 24, 2021

View reviewed changes

austinabell added 7 commits June 24, 2021 13:12

update contract standards

07c1fec

update sim and tests

26a9f8d

fmt

51c2782

fix examples and refactor sim usage

05e57ed

remove ValidAccountId

17475b0

fix examples

3e858be

delete commented out mod

2c4e449

austinabell marked this pull request as ready for review June 24, 2021 20:25

austinabell requested review from evgenykuzyakov and mikedotexe as code owners June 24, 2021 20:25

austinabell requested a review from ChaoticTempest June 24, 2021 20:25

Merge branch 'master' into austin/account_id_newtype

7cae165

mikedotexe reviewed Jun 24, 2021

View reviewed changes

near-sdk-sim/src/user.rs Show resolved Hide resolved

mikedotexe reviewed Jun 25, 2021

View reviewed changes

near-sdk/src/types/account_id.rs Outdated Show resolved Hide resolved

mikedotexe reviewed Jun 25, 2021

View reviewed changes

near-sdk/src/types/account_id.rs Outdated Show resolved Hide resolved

mikedotexe approved these changes Jun 25, 2021

View reviewed changes

austinabell added 2 commits June 24, 2021 20:16

fix typo

71fa951

update docs

d36276c

austinabell force-pushed the austin/account_id_newtype branch from 4a0dd74 to d36276c Compare June 25, 2021 01:59

changelog

082c5d5

austinabell mentioned this pull request Jun 25, 2021

List of odd APIs #387

Open

10 tasks

austinabell requested a review from matklad June 25, 2021 15:15

Merge branch 'master' into austin/account_id_newtype

f007efd

matklad reviewed Jul 5, 2021

View reviewed changes

austinabell added 6 commits July 5, 2021 17:21

update account id initializations

a409a25

remove generic use of account id from promise calls

b5388f6

cleanup account id code

3bf1cae

auxiliary tweaks and remove into_string method

a0f4c25

other relevant cleanup

8e704b9

add test for borsh serialize

e7112fc

austinabell added 9 commits July 6, 2021 11:06

remove TryFrom<&str> and add borsh deserialize validation

1376e92

fix breakages

e8e06dd

avoid re-allocation by not parsing from str

36e6708

remove conversion from bytes to account id

28f1d39

updated wasm blobs

d38e251

remove validations to keep consistent with previous version

a13c8f2

remove other unnecessary conversions

11c42fb

Merge branch 'master' of github.com:near/near-sdk-rs into austin/acco…

afbc330

…unt_id_newtype

update mission control example

f22bdb5

austinabell requested a review from matklad July 8, 2021 18:44

matklad approved these changes Jul 9, 2021

View reviewed changes

near-sdk/src/types/account_id.rs Outdated Show resolved Hide resolved

near-sdk/src/utils/mod.rs Outdated Show resolved Hide resolved

austinabell added 2 commits July 9, 2021 10:34

remove account id error kind and nit

3d050be

prohibit error creation manually

3e24240

austinabell force-pushed the austin/account_id_newtype branch from 33e7684 to 3e24240 Compare July 9, 2021 14:42

mikedotexe merged commit e4abb73 into master Jul 12, 2021

mikedotexe deleted the austin/account_id_newtype branch July 12, 2021 23:20

austinabell mentioned this pull request Sep 20, 2021

Unsafe AccountId to ValidAccountId conversion #312

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(types): change account id to newtype and deprecate ValidAccountId #448

refactor(types): change account id to newtype and deprecate ValidAccountId #448

austinabell commented Jun 24, 2021 •

edited

Loading

matklad Jun 24, 2021

austinabell Jun 24, 2021

austinabell Jun 24, 2021

mikedotexe left a comment

frol commented Jun 25, 2021

austinabell commented Jun 25, 2021

austinabell commented Jun 25, 2021

frol commented Jun 25, 2021

austinabell commented Jun 25, 2021 •

edited

Loading

austinabell commented Jun 25, 2021

matklad left a comment

matklad Jul 5, 2021

matklad Jul 5, 2021

austinabell Jul 5, 2021

mikedotexe Jul 12, 2021

austinabell commented Jul 5, 2021

matklad commented Jul 6, 2021 •

edited

Loading

austinabell commented Jul 6, 2021

	pub fn get_status(&self, account_id: AccountId) -> Option::<String> {
	pub fn get_status(&self, account_id: AccountId) -> Option<String> {

refactor(types): change account id to newtype and deprecate ValidAccountId #448

refactor(types): change account id to newtype and deprecate ValidAccountId #448

Conversation

austinabell commented Jun 24, 2021 • edited Loading

matklad Jun 24, 2021

Choose a reason for hiding this comment

austinabell Jun 24, 2021

Choose a reason for hiding this comment

austinabell Jun 24, 2021

Choose a reason for hiding this comment

mikedotexe left a comment

Choose a reason for hiding this comment

frol commented Jun 25, 2021

austinabell commented Jun 25, 2021

austinabell commented Jun 25, 2021

frol commented Jun 25, 2021

austinabell commented Jun 25, 2021 • edited Loading

austinabell commented Jun 25, 2021

matklad left a comment

Choose a reason for hiding this comment

matklad Jul 5, 2021

Choose a reason for hiding this comment

matklad Jul 5, 2021

Choose a reason for hiding this comment

austinabell Jul 5, 2021

Choose a reason for hiding this comment

mikedotexe Jul 12, 2021

Choose a reason for hiding this comment

austinabell commented Jul 5, 2021

matklad commented Jul 6, 2021 • edited Loading

austinabell commented Jul 6, 2021

austinabell commented Jun 24, 2021 •

edited

Loading

austinabell commented Jun 25, 2021 •

edited

Loading

matklad commented Jul 6, 2021 •

edited

Loading