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] #3962: Revoke associated tokens on entity unregistretration #4213

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

Erigara
Copy link
Contributor

@Erigara Erigara commented Jan 18, 2024

Description

Hook into visit_unregister_* was added to remove associated permission tokens.

I've decided not to add this cleanup logic for the Asset entity because asset entity could be added and remove multiple times (for example when asset value reach 0).

Also i've made decision not to use macros here as imo it wouldn't bring much value here.

Linked issue

Closes #3962

Benefits

Tokens are cleaned up.

Drawbacks

Uninteresting entities could became potentially expensive as we have to iterate through every token.

@Erigara Erigara added Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST labels Jan 18, 2024
@Erigara Erigara self-assigned this Jan 18, 2024
@mversic
Copy link
Contributor

mversic commented Jan 18, 2024

I find this implementation very brittle. If we add a new token we have to make sure that we also add the code for it's removal. I would suggest using map_token! and implement a callback on every token for every entity. Some of the callbacks won't do anything

@Erigara
Copy link
Contributor Author

Erigara commented Jan 18, 2024

I find this implementation very brittle. If we add a new token we have to make sure that we also add the code for it's removal.

I will try to figure out how to do this

@Erigara
Copy link
Contributor Author

Erigara commented Jan 18, 2024

@mversic take a look, imo this solution is less brittle

@mversic
Copy link
Contributor

mversic commented Jan 18, 2024

I've decided not to add this cleanup logic for the Asset entity because asset entity could be added and remove multiple times (for example when asset value reach 0).

I think this is fine. Register/Unregister asset operations aren't like others (personally, I would remove them, and only keep Mint/Burn). If the same asset is unregistered and then registered again it will still be the same asset. So it's not a bug to have previously assigned permission token apply.

But what I think you should do is:

  1. remove all permission tokens referring to assets when unregistering an asset definition
  2. remove all permission tokens referring to accounts and asset definitions when unregistering a domain

But as far as I can see, you have already done this

@Erigara
Copy link
Contributor Author

Erigara commented Jan 18, 2024

But as far as I can see, you have already done this

Yes, i already do this

@DCNick3 DCNick3 self-assigned this Jan 22, 2024
@mversic mversic assigned Asem-Abdelhady and unassigned DCNick3 Jan 22, 2024
@mversic mversic force-pushed the remove_token branch 2 times, most recently from fafec40 to dfcf871 Compare January 24, 2024 13:54
@Arjentix Arjentix self-assigned this Jan 24, 2024
smart_contract/executor/src/permission.rs Show resolved Hide resolved
Comment on lines +391 to +392
let bob_id: AccountId = "bob@wonderland".parse().expect("Valid");
let kingdom_id: DomainId = "kingdom".parse().expect("Valid");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we now discourage the use of expect("Valid") and prefer just unwrap()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unaware of this, where it's stated?

Comment on lines +211 to +212
for (owner_id, permission) in accounts_permission_tokens() {
if is_token_domain_associated(&permission, domain_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it can (or should in future) be implemented using query filter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Permission tokens are opaque for core, how it should extract fields?

@Erigara Erigara merged commit 8650781 into hyperledger-iroha:iroha2-dev Jan 25, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants