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

Jump to bottom

[feature] #1734: Validate Name to exclude whitespaces #1743

Merged
merged 9 commits into from
Dec 22, 2021
Merged

[feature] #1734: Validate Name to exclude whitespaces #1743

merged 9 commits into from
Dec 22, 2021

Conversation

s8sato
Copy link
Contributor

@s8sato s8sato commented Dec 21, 2021 •
edited
Loading

Description of the Change

  • Prohibit a whitespace from getting into a Name of domain, account, asset, metadata key, and permission token.
  • Identify Domain by Id instead of Name

Now Id::news return Result<Self>, while previous implementations are left as Id::tests for usability

Issue

Close #1734

Benefits

Type level guarantee of no whitespaces in Name

Possible Drawbacks

There might be a better error handling for Name validation

@s8sato
Unify metadata key to Name
18c9d8e 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
@s8sato
Identify Domain by Id instead of Name
995b14f 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
@s8sato
Make Name a wrapper struct instead of a type alias of String
6bae374 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
@s8sato
Rename all Id::new()s to Id::test()s
530ee9b 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
@s8sato
Define new Id::new()s which return Result<Self>
69a1fd8 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
@s8sato
Rewrite some Id::test()s to new Id::new()s
32efe0d 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Dec 21, 2021
@codecov
Copy link

codecov bot commented Dec 21, 2021 •
edited
Loading

Codecov Report

Merging #1743 (023d738) into iroha2-dev (cb37922) will increase coverage by 0.25%.
The diff coverage is 81.20%.

Impacted file tree graph

@@              Coverage Diff               @@
##           iroha2-dev    #1743      +/-   ##
==============================================
+ Coverage       77.18%   77.43%   +0.25%     
==============================================
  Files             132      132              
  Lines           20412    20479      +67     
==============================================
+ Hits            15754    15858     +104     
+ Misses           4658     4621      -37
Impacted Files Coverage Δ
...tests/integration_tests/multiple_blocks_created.rs 0.00% <0.00%> (ø)
client/tests/integration_tests/unstable_network.rs 0.00% <0.00%> (ø)
client_cli/src/main.rs 0.32% <0.00%> (ø)
core/src/smartcontracts/isi/account.rs 38.88% <0.00%> (ø)
core/src/smartcontracts/isi/asset.rs 68.14% <0.00%> (ø)
core/src/smartcontracts/isi/world.rs 50.00% <ø> (ø)
core/test_network/tests/sumeragi_with_mock.rs 0.23% <0.00%> (ø)
data_model/src/isi.rs 77.04% <ø> (ø)
data_model/src/query.rs 27.01% <47.61%> (+0.57%) ⬆️
permissions_validators/src/lib.rs 64.35% <72.72%> (-0.34%) ⬇️
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb37922...023d738. Read the comment docs.

@s8sato
Reduce methods to Name::validate_len
4c413ab 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
@s8sato s8sato marked this pull request as ready for review December 21, 2021 15:40
appetrosyan
appetrosyan reviewed Dec 21, 2021
View reviewed changes
data_model/src/metadata.rs
@@ -245,20 +245,20 @@ mod tests {
let mut metadata = Metadata::new();
let limits = Limits::new(10, 15);
metadata
.insert_with_limits("0".to_owned(), Metadata::new().into(), limits)
.insert_with_limits(Name::test("0"), Metadata::new().into(), limits)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the function that creates a new name called test?

Copy link
Contributor Author

@s8sato s8sato Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Because it instantly constructs Name assuming the input is valid.
In contrast, Name::new gracefully constructs and returns Result<Self>

Copy link
Contributor

Choose a reason for hiding this comment

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

Unchecked?

Copy link
Contributor Author

@s8sato s8sato Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Name::test includes parsing and unwrapping so if any whitespaces it panics

Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend here:

  1. new -> parse because this is essentially wrapper for String::parse
  2. test -> new/parse_unwrap

because test is very ambiguous

Copy link
Contributor Author

@s8sato s8sato Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

new intends the default constructor.
test would be renamed to unchecked, instant, or succeed

appetrosyan
appetrosyan previously approved these changes Dec 21, 2021
View reviewed changes
mversic
mversic requested changes Dec 21, 2021
View reviewed changes
data_model/src/lib.rs Show resolved Hide resolved
data_model/src/lib.rs Outdated Show resolved Hide resolved
data_model/src/lib.rs Outdated Show resolved Hide resolved
data_model/src/lib.rs Outdated Show resolved Hide resolved
@s8sato
Fix for tests
8877c65 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
@s8sato
Apply review comments
023d738 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
@mversic mversic self-requested a review December 21, 2021 18:42
@appetrosyan appetrosyan merged commit e24a7c0 into hyperledger:iroha2-dev Dec 22, 2021
s8sato added a commit to s8sato/iroha that referenced this pull request Dec 27, 2021
@s8sato
Fix JSONs according to hyperledger#1743 Domain struct change
4799831 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
s8sato added a commit to s8sato/iroha that referenced this pull request Dec 27, 2021
@s8sato
Fix JSONs according to hyperledger#1743 Domain struct change
92b7ad3 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
s8sato added a commit to s8sato/iroha that referenced this pull request Dec 28, 2021
@s8sato
Fix JSONs according to hyperledger#1743 Domain struct change
13a50c2 
Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
This was referenced Dec 28, 2021
Merged
Closed
s8sato added a commit that referenced this pull request Dec 28, 2021
@s8sato
[fix] #1755: Minor fixes for #1743, #1725 (#1760)
3d85d89 
* Fix JSONs according to #1743 `Domain` struct change

Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>

* Fix `status` term to `telemetry` according to #1725

Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>

* Fix `status_address`

Signed-off-by: s8sato <49983831+s8sato@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@appetrosyan appetrosyan appetrosyan approved these changes

@mversic mversic mversic approved these changes

@e-ivkov e-ivkov Awaiting requested review from e-ivkov

@KalitaAlexey KalitaAlexey Awaiting requested review from KalitaAlexey

Assignees
No one assigned
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@s8sato @appetrosyan @mversic