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

[fix] #2017: Fix role unregistration #2313

Merged

Conversation

Erigara
Copy link
Contributor

@Erigara Erigara commented Jun 3, 2022

Signed-off-by: Shanin Roman shahin1000@yandex.ru

Description of the Change

  • Add Role-related match arm in UnregisterBox execution;
  • Fix Unregister<Role>::execute by:
    • removing double deletion of role from account;
    • returning correct event after revoking role from account;
    • correctly early exiting if role doesn't exist;
  • Add Role-related queries to client;
  • Add tests:
    • to check that role removed from account after unregistering;
    • to check Role-related queries.

Issue

Resolves #2017

Benefits

  • New tests;
  • Bug fixes.

Possible Drawbacks

None.

Usage Examples or Tests

cargo test --package iroha_client --test mod -- integration::roles::unregistred_role_removed_from_account --exact --nocapture
cargo test --package iroha_client --test mod -- integration::queries::role

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
@Erigara Erigara added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Jun 3, 2022
@Erigara Erigara changed the title [test] #2017: Add integration test for unregistring role [fix] #2017: Add integration test for unregistring role Jun 3, 2022
@Erigara Erigara changed the title [fix] #2017: Add integration test for unregistring role [fix] #2017: Fix role unregistring Jun 3, 2022
@Erigara Erigara changed the title [fix] #2017: Fix role unregistring [fix] #2017: Fix role unregistration Jun 3, 2022
@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #2313 (176d4ff) into iroha2-dev (6973f71) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@              Coverage Diff               @@
##           iroha2-dev    #2313      +/-   ##
==============================================
- Coverage       65.50%   65.43%   -0.07%     
==============================================
  Files             133      133              
  Lines           24697    24720      +23     
==============================================
- Hits            16177    16176       -1     
- Misses           8520     8544      +24     
Impacted Files Coverage Δ
client/src/client.rs 44.04% <0.00%> (-0.81%) ⬇️
core/src/smartcontracts/isi/mod.rs 52.45% <0.00%> (-0.14%) ⬇️
core/src/smartcontracts/isi/world.rs 9.52% <0.00%> (+0.43%) ⬆️
data_model/src/query.rs 39.34% <0.00%> (-1.90%) ⬇️
core/src/kura.rs 92.19% <0.00%> (-0.16%) ⬇️

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 410a713...176d4ff. Read the comment docs.

client/tests/integration/roles.rs Outdated Show resolved Hide resolved
client/src/client.rs Outdated Show resolved Hide resolved
client/src/client.rs Outdated Show resolved Hide resolved
@s8sato s8sato self-assigned this Jun 3, 2022
@Erigara Erigara force-pushed the unregister_role_integaration_test branch from 445c839 to e691ef8 Compare June 3, 2022 10:16
Arjentix
Arjentix previously approved these changes Jun 3, 2022
@@ -297,6 +297,7 @@ impl<W: WorldTrait> Execute<W> for UnregisterBox {
Unregister::<Domain>::new(domain_id).execute(authority, wsv)
}
IdBox::PeerId(peer_id) => Unregister::<Peer>::new(peer_id).execute(authority, wsv),
IdBox::RoleId(role_id) => Unregister::<Role>::new(role_id).execute(authority, wsv),
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug with forgetting about adding new match arm when a new instruction was added appears too often.

@appetrosyan , @mversic , how do you think, can we add a test for it based on procedural macro for every Execute trait implementation? It's all core crate so macro expansion should work fine. At least should be only false-positive triggering, non false-negative

Copy link
Contributor

Choose a reason for hiding this comment

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

we can make a totally safe implementation (no tests needed) with a (simple?) declarative macro. But in this case the implementations of Execute for e.g. RegisterBox and it's variants need to be grouped inside the macro. I can write an example if needed, but have a look at the ffi/src/lib.rs for inspiration.

this is what I would prefer since these unboxing implementations are so similar that they cry out to be implemented in a macro. I would prefer declarative, but that is at discretion of the person implementing the macro

client/tests/integration/roles.rs Outdated Show resolved Hide resolved
client/tests/integration/queries/role.rs Show resolved Hide resolved
s8sato
s8sato previously approved these changes Jun 3, 2022
core/src/smartcontracts/isi/world.rs Show resolved Hide resolved
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
@Erigara Erigara dismissed stale reviews from s8sato and Arjentix via 176d4ff June 3, 2022 13:23
@Erigara Erigara force-pushed the unregister_role_integaration_test branch from e691ef8 to 176d4ff Compare June 3, 2022 13:23
@Erigara Erigara requested a review from Arjentix June 3, 2022 15:38
@Erigara Erigara merged commit 74fdadb into hyperledger:iroha2-dev Jun 3, 2022
@Erigara Erigara deleted the unregister_role_integaration_test branch June 3, 2022 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants