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

[feature] #2257: Revoke<Role> emits RoleRevoked event #2264

Merged

Conversation

omkar-mohanty
Copy link
Contributor

Description of the Change

This PR makes Revoke role emit RoleRevoked instead of Permission event.

Issue

Revoke role emits Permission events which is a misleading since roles are a collection of permission events.

Benefits

More accurate information about the user.

Possible Drawbacks

Not sure about drawbacks because I am new to the codebase.

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label May 25, 2022
@appetrosyan appetrosyan changed the title [enhancement] #2257 : Revoke should emit RoleRevoked [feature] #2257: Revoke<Role> emits RoleRevoked event May 25, 2022
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #2264 (309b1e9) into iroha2-dev (2ba9c8f) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@              Coverage Diff               @@
##           iroha2-dev    #2264      +/-   ##
==============================================
- Coverage       64.67%   64.66%   -0.02%     
==============================================
  Files             131      131              
  Lines           24664    24666       +2     
==============================================
- Hits            15952    15950       -2     
- Misses           8712     8716       +4     
Impacted Files Coverage Δ
core/src/smartcontracts/isi/account.rs 12.30% <0.00%> (ø)
data_model/src/events/data/events.rs 39.58% <0.00%> (-0.85%) ⬇️
data_model/src/events/data/filters.rs 24.23% <ø> (ø)
tools/kura_inspector/src/print.rs 95.38% <0.00%> (-0.77%) ⬇️
core/src/kura.rs 92.04% <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 2ba9c8f...309b1e9. Read the comment docs.

Signed-off-by: omkar-mohanty <franzohouser@gmail.com>
@appetrosyan
Copy link
Contributor

OK. At this point the implementation of a new event seems to be complete. @omkar-mohanty

The remaining question is that of semantics. If we merge as is, an account can lose a permission that's included in a role and event listeners that look for the permission being removed will not be notified. To me this seems like it would complicate things for trigger designers.

What I want, is for event listeners that look for PermissionAdded/Removed to also be notified if a role that contains this permission is being revoked.

This would also require a test, but I think we can handle that.

@omkar-mohanty
Copy link
Contributor Author

I stumbled upon impl<W: WorldTrait> Execute<W> for Unregister<Role> which emits a PermissionRemoved event should I change that to RoleRevoked event?

@omkar-mohanty omkar-mohanty marked this pull request as ready for review May 27, 2022 15:51
@omkar-mohanty
Copy link
Contributor Author

omkar-mohanty commented May 27, 2022

I have decided that implementing changes to the event listeners is beyond the scope of my work. As such this PR is complete.

appetrosyan
appetrosyan previously approved these changes May 27, 2022
@omkar-mohanty
Copy link
Contributor Author

Hello maintainers I have addressed the issue in #2257 which was originally from #2007 by adding RoleRevoked event implementation. This PR still does not implement proper event processing for RoleRevoked as noted by @appetrosyan but I would like to have this PR merged as it is. Thanks for reviewing my code.

@omkar-mohanty
Copy link
Contributor Author

For the event processing issue I have created #2280

Copy link
Contributor

@s8sato s8sato left a comment

Choose a reason for hiding this comment

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

For symmetry we also need something like RoleGranted for PermissionAdded

Signed-off-by: omkar-mohanty <franzohouser@gmail.com>
@appetrosyan appetrosyan merged commit 5ddb17c into hyperledger-iroha:iroha2-dev May 31, 2022
BAStos525 pushed a commit to BAStos525/soramitsu-iroha that referenced this pull request Jul 8, 2022
hyperledger-iroha#2264)

Signed-off-by: omkar-mohanty <franzohouser@gmail.com>
Signed-off-by: BAStos525 <jungle.vas@yandex.ru>
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.

4 participants