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.

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

Define AccountId to hold a contract of being valid #2074

Closed
frol opened this issue Feb 3, 2020 · 1 comment · Fixed by #4440 or #4621
Closed

Define AccountId to hold a contract of being valid #2074

frol opened this issue Feb 3, 2020 · 1 comment · Fixed by #4440 or #4621
Assignees
Labels
A-RPC Area: rpc A-security Area: Security issues A-testing Area: Unit testing / integration testing C-enhancement Category: An issue proposing an enhancement or a PR with one. C-housekeeping Category: Refactoring, cleanups, code quality P-low Priority: low

Comments

@frol
Copy link
Collaborator

frol commented Feb 3, 2020

We should leverage strong typing and safety contracts of Rust, and make AccountId type that enforces its validity. This way, we improve node security, especially, around RPC boundary.

@frol frol added C-enhancement Category: An issue proposing an enhancement or a PR with one. C-housekeeping Category: Refactoring, cleanups, code quality A-testing Area: Unit testing / integration testing A-security Area: Security issues A-RPC Area: rpc labels Feb 3, 2020
@frol frol self-assigned this Feb 3, 2020
@frol frol added the P-low Priority: low label Mar 14, 2020
@ilblackdragon ilblackdragon added this to the Post MainNet milestone Mar 18, 2020
@frol frol mentioned this issue Apr 11, 2020
3 tasks
@frol frol assigned miraclx and unassigned frol Jun 3, 2021
near-bulldozer bot pushed a commit that referenced this issue Jul 29, 2021
…4440)

This PR introduces a strictly typed structure - `AccountId` that enforces account ID validation.
This way, we can ensure validity and thereby fail fast before an invalid account name is used.

This also, optionally, ensures validation on {,de}serialization so, yeah..

The structure is defined within its own self-contained crate `near-account-id` to allow for easy publishing.
The extra dependencies - `borsh`, `serde` are made optional to allow the crate to be lightweight enough to be depended upon.

Within `nearcore`, the structure is reexported via `near_primitives::AccountId` and the crate via `near_primitives{,_core}::account::id` with default features (`borsh`, `serde`) but you can opt-out of those by depending on it directly.

Resolves #2074, supersedes #2831.

# Testing Plan

* [x] Existing unit tests pass with minor updates
* [x] Nayduck tests pass without modifications
* [x] Manually ensured that the protocol is not changed
miraclx added a commit to miraclx/nearcore that referenced this issue Aug 2, 2021
…ear#4440)

This PR introduces a strictly typed structure - `AccountId` that enforces account ID validation.
This way, we can ensure validity and thereby fail fast before an invalid account name is used.

This also, optionally, ensures validation on {,de}serialization so, yeah..

The structure is defined within its own self-contained crate `near-account-id` to allow for easy publishing.
The extra dependencies - `borsh`, `serde` are made optional to allow the crate to be lightweight enough to be depended upon.

Within `nearcore`, the structure is reexported via `near_primitives::AccountId` and the crate via `near_primitives{,_core}::account::id` with default features (`borsh`, `serde`) but you can opt-out of those by depending on it directly.

Resolves near#2074, supersedes near#2831.

# Testing Plan

* [x] Existing unit tests pass with minor updates
* [x] Nayduck tests pass without modifications
* [x] Manually ensured that the protocol is not changed
miraclx added a commit that referenced this issue Aug 2, 2021
…4440)

This PR introduces a strictly typed structure - `AccountId` that enforces account ID validation.
This way, we can ensure validity and thereby fail fast before an invalid account name is used.

This also, optionally, ensures validation on {,de}serialization so, yeah..

The structure is defined within its own self-contained crate `near-account-id` to allow for easy publishing.
The extra dependencies - `borsh`, `serde` are made optional to allow the crate to be lightweight enough to be depended upon.

Within `nearcore`, the structure is reexported via `near_primitives::AccountId` and the crate via `near_primitives{,_core}::account::id` with default features (`borsh`, `serde`) but you can opt-out of those by depending on it directly.

Resolves #2074, supersedes #2831.

* [x] Existing unit tests pass with minor updates
* [x] Nayduck tests pass without modifications
* [x] Manually ensured that the protocol is not changed
@frol
Copy link
Collaborator Author

frol commented Aug 4, 2021

Just for the future reference, we had to revert the PR due to #4600, and the fix will land in #4621.

@frol frol reopened this Aug 4, 2021
@frol frol closed this as completed in #4621 Aug 5, 2021
frol pushed a commit that referenced this issue Aug 5, 2021
# refactor: Introduce strictly typed and assuredly validated AccountId (#4440)

This PR introduces a strictly typed structure - `AccountId` that enforces account ID validation.
This way, we can ensure validity and thereby fail fast before an invalid account name is used.

This also, optionally, ensures validation on {,de}serialization so, yeah..

The structure is defined within its own self-contained crate `near-account-id` to allow for easy publishing.
The extra dependencies - `borsh`, `serde` are made optional to allow the crate to be lightweight enough to be depended upon.

Within `nearcore`, the structure is reexported via `near_primitives::AccountId` and the crate via `near_primitives{,_core}::account::id` with default features (`borsh`, `serde`) but you can opt-out of those by depending on it directly.

Resolves #2074, supersedes #2831.

* [x] Existing unit tests pass with minor updates
* [x] Nayduck tests pass without modifications
* [x] Manually ensured that the protocol is not changed


# impl workaround for invalid AccessKey-s in genesis records (resolves #4600)

* [x] Manually booted a testnet node to ensure genesis loads properly and that no further AccountId-related issues occur.
* [x] Ensured unit tests pass with no further updates
* [x] Nayduck tests pass without modifications
nikurt pushed a commit that referenced this issue Aug 9, 2021
This PR introduces a strictly typed structure - `AccountId` that enforces account ID validation.
This way, we can ensure validity and thereby fail fast before an invalid account name is used.

This also, optionally, ensures validation on {,de}serialization so, yeah..

The structure is defined within its own self-contained crate `near-account-id` to allow for easy publishing.
The extra dependencies - `borsh`, `serde` are made optional to allow the crate to be lightweight enough to be depended upon.

Within `nearcore`, the structure is reexported via `near_primitives::AccountId` and the crate via `near_primitives{,_core}::account::id` with default features (`borsh`, `serde`) but you can opt-out of those by depending on it directly.

Resolves #2074, supersedes #2831.

* [x] Existing unit tests pass with minor updates
* [x] Nayduck tests pass without modifications
* [x] Manually ensured that the protocol is not changed

* [x] Manually booted a testnet node to ensure genesis loads properly and that no further AccountId-related issues occur.
* [x] Ensured unit tests pass with no further updates
* [x] Nayduck tests pass without modifications
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-RPC Area: rpc A-security Area: Security issues A-testing Area: Unit testing / integration testing C-enhancement Category: An issue proposing an enhancement or a PR with one. C-housekeeping Category: Refactoring, cleanups, code quality P-low Priority: low
Projects
None yet
4 participants