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

[refactor]: encapsulate access to data model structures #1984

Merged
merged 7 commits into from
Apr 1, 2022
Merged

[refactor]: encapsulate access to data model structures #1984

merged 7 commits into from
Apr 1, 2022

Conversation

mversic
Copy link
Contributor

@mversic mversic commented Mar 20, 2022 •
edited
Loading

Signed-off-by: Marin Veršić marin.versic101@gmail.com

Description of the Change

This PR encapsulates access to the data model structures and separates internal structure from the API presented. This will bring about separation of concerns and enable us to have the same API for client applications and wasm access through FFI. Namely, this PR encapsulates FFI heavy structures, it doesn't encapsulate structure such as Identifiable::Id because they should be easily serialized across FFI and it's unlikely their internal representation will change

Includes:

  • field accessors - custom and derived with getset
  • builder pattern for structures implementing Identifiable
  • name deserialization fix - we should be more mindful of all the paths for constructing structures maintaining invariants

I invite you to think whether this modification of the API is a step in a right direction. Additional simplifications should be left for a separate PR

Issue

Closes #1982

Benefits

Possible Drawbacks

Usage Examples or Tests [optional]

Alternate Designs [optional]

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Mar 20, 2022
@codecov
Copy link

codecov bot commented Mar 20, 2022 •
edited
Loading

Codecov Report

Merging #1984 (3aa4666) into iroha2-dev (8d83a3e) will decrease coverage by 0.16%.
The diff coverage is 76.46%.

@@              Coverage Diff               @@
##           iroha2-dev    #1984      +/-   ##
==============================================
- Coverage       77.94%   77.78%   -0.17%     
==============================================
  Files             176      176              
  Lines           24039    24216     +177     
==============================================
+ Hits            18738    18837      +99     
- Misses           5301     5379      +78
Impacted Files Coverage Δ
client/examples/million_accounts_genesis.rs 0.00% <0.00%> (ø)
client/examples/million_accounts_tx.rs 0.00% <0.00%> (ø)
client/examples/ten_million_accounts.rs 0.00% <0.00%> (ø)
...lient/tests/integration/multiple_blocks_created.rs 0.00% <0.00%> (ø)
client/tests/integration/unstable_network.rs 0.00% <0.00%> (ø)
client_cli/src/main.rs 0.32% <0.00%> (+<0.01%) ⬆️
core/test_network/tests/sumeragi_with_mock.rs 0.28% <0.00%> (+<0.01%) ⬆️
data_model/src/events/data/events.rs 30.30% <ø> (ø)
data_model/src/permissions.rs 100.00% <ø> (ø)
data_model/src/query.rs 33.00% <ø> (ø)
... and 86 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 8d83a3e...3aa4666. Read the comment docs.

@appetrosyan appetrosyan changed the title encapsulate access to data model structures [refactor]: encapsulate access to data model structures Mar 21, 2022
@appetrosyan appetrosyan added the api-changes Changes in the API for client libraries label Mar 21, 2022
@mversic mversic force-pushed the data_model_encapsulation branch 2 times, most recently from 37509b3 to 66ddf1d Compare March 21, 2022 07:59
@appetrosyan
Copy link
Contributor

Also, maybe Name::parse makes a bit more sense than from_str.

@mversic mversic force-pushed the data_model_encapsulation branch 4 times, most recently from 7bc71e0 to 67d2fba Compare March 22, 2022 10:53
@mversic
Copy link
Contributor Author

mversic commented Mar 22, 2022

Also, maybe Name::parse makes a bit more sense than from_str.

it can be either String::parse or Name::from_str

@appetrosyan appetrosyan self-requested a review March 22, 2022 12:04
mversic
mversic commented Mar 22, 2022
View reviewed changes
data_model/src/domain.rs Outdated Show resolved Hide resolved
@mversic mversic requested review from Arjentix and s8sato March 27, 2022 15:46
s8sato
s8sato previously approved these changes Mar 28, 2022
View reviewed changes
@Arjentix Arjentix mentioned this pull request Mar 28, 2022
Closed
Arjentix
Arjentix reviewed Mar 28, 2022
View reviewed changes
Copy link
Contributor

@Arjentix Arjentix left a comment

Choose a reason for hiding this comment

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

I'll approve after rebasing

@appetrosyan appetrosyan mentioned this pull request Mar 30, 2022
Closed
@mversic
encapsulate access to data model structures
857ca99 
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic
add data_model mutable_api feature flag
d48325b 
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic
add constructor type to Identifiable
2974fb7 
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic
fix review comments, allow for empty identifiers
14823b0 
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic
remove serde and parity_scale_codec bloat from SmallVec
a6925c8 
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic
fix register isi execution, check permissions before executing isi, r…
b0e22b4 
…eturn error instead of assert panics

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic mversic mentioned this pull request Mar 31, 2022
Closed
@mversic
add test to ensure permisions are checked before execution
3aa4666 
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
@mversic mversic requested a review from Arjentix April 1, 2022 10:07
appetrosyan
appetrosyan reviewed Apr 1, 2022
View reviewed changes
client/tests/integration/add_domain.rs
let create_domain = RegisterBox::new(IdentifiableBox::from(Domain::new(
normal_domain_name.clone(),
)));
let normal_domain_id: DomainId = "sora".parse()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

While I think this type can be inferred, having more explicit type annotations doesn't hurt.

@mversic mversic merged commit 2a94dba into hyperledger:iroha2-dev Apr 1, 2022
@mversic mversic deleted the data_model_encapsulation branch April 1, 2022 13:31
mversic added a commit to mversic/iroha that referenced this pull request May 2, 2022
@mversic
[refactor]: encapsulate access to data model structures (hyperledger#…
be5d5f4 
…1984)

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022
@mversic @appetrosyan
[refactor]: encapsulate access to data model structures (hyperledger#…
66d8908 
…1984)
appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022
@mversic @appetrosyan
[refactor]: encapsulate access to data model structures (hyperledger#…
451f828 
…1984)

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
mversic added a commit to mversic/iroha that referenced this pull request May 13, 2022
@mversic
[refactor]: encapsulate access to data model structures (hyperledger#…
9d13097 
…1984)

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
mversic added a commit to mversic/iroha that referenced this pull request May 13, 2022
@mversic
[refactor]: encapsulate access to data model structures (hyperledger#…
284c1fe 
…1984)
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

@s8sato s8sato s8sato approved these changes

@Arjentix Arjentix Awaiting requested review from Arjentix

Assignees
No one assigned
Labels
api-changes Changes in the API for client libraries 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.
4 participants
@mversic @appetrosyan @Arjentix @s8sato