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

refactor: remove domain from trigger identity #4640

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

nxsaken
Copy link
Contributor

@nxsaken nxsaken commented May 23, 2024

Description

Remove DomainId from TriggerId.

Linked issue

Closes #4630.

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@github-actions github-actions bot added the api-changes Changes in the API for client libraries label May 23, 2024
@nxsaken
Copy link
Contributor Author

nxsaken commented May 23, 2024

UPD: to fix this, the default executor has to be updated (see the pre-commit hook)

I'm having a bad time with integration tests, many of which are failing with a cryptic error, even those unrelated to triggers:

domain_owner_trigger_permissions:

ERROR iroha_core::tx: Internal error occurred during transaction validation, is Runtime Executor correct?, error: "error while executing at wasm backtrace:\n    0: 0x24ccf - <unknown>!<wasm function 235>: wasm trap: wasm `unreachable` instruction executed"

grant_revoke_role_permissions:

ERROR iroha_core::tx: Internal error occurred during transaction validation, is Runtime Executor correct?, error: "error while executing at wasm backtrace:\n    0: 0x13a80 - <unknown>!<wasm function 104>\n    1: 0x562f - <unknown>!<wasm function 43>\n    2: 0x24c89 - <unknown>!<wasm function 235>: Could not decode `SmartContractQueryRequest.0`:\n\tCould not decode `QueryRequest::Query.0`:\n\t\tCould not decode `SmartContractQuery::query`:\n\t\t\tCould not decode `QueryBox::FindRolesByAccountId.0`:\n\t\t\t\tCould not decode `FindRolesByAccountId::id`:\n\t\t\t\t\tCould not decode `AccountId::signatory`:\n\t\t\t\t\t\tout of range decoding Compact<u32>\n: Could not decode `QueryRequest::Query.0`:\n\tCould not decode `SmartContractQuery::query`:\n\t\tCould not decode `QueryBox::FindRolesByAccountId.0`:\n\t\t\tCould not decode `FindRolesByAccountId::id`:\n\t\t\t\tCould not decode `AccountId::signatory`:\n\t\t\t\t\tout of range decoding Compact<u32>\n: Could not decode `SmartContractQuery::query`:\n\tCould not decode `QueryBox::FindRolesByAccountId.0`:\n\t\tCould not decode `FindRolesByAccountId::id`:\n\t\t\tCould not decode `AccountId::signatory`:\n\t\t\t\tout of range decoding Compact<u32>\n: Could not decode `QueryBox::FindRolesByAccountId.0`:\n\tCould not decode `FindRolesByAccountId::id`:\n\t\tCould not decode `AccountId::signatory`:\n\t\t\tout of range decoding Compact<u32>\n: Could not decode `FindRolesByAccountId::id`:\n\tCould not decode `AccountId::signatory`:\n\t\tout of range decoding Compact<u32>\n: Could not decode `AccountId::signatory`:\n\tout of range decoding Compact<u32>\n: out of range decoding Compact<u32>"

@nxsaken nxsaken self-assigned this May 23, 2024
@nxsaken nxsaken added the Refactor Improvement to overall code quality label May 23, 2024
@nxsaken nxsaken changed the title refactor: remove DomainId from TriggerId refactor: remove association between domains and triggers May 23, 2024
@nxsaken nxsaken marked this pull request as ready for review May 23, 2024 12:08
@Erigara Erigara self-assigned this May 23, 2024
Copy link
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

Ping for approval after executor recompile

Erigara
Erigara previously approved these changes May 23, 2024
Erigara
Erigara previously approved these changes May 23, 2024
@nxsaken nxsaken force-pushed the remove-trigger-domain_id branch 2 times, most recently from 5813e50 to 62c21ec Compare May 27, 2024 16:25
@nxsaken
Copy link
Contributor Author

nxsaken commented May 29, 2024

You're right, I forgot about the validator visitors.

@Stukalov-A-M
Copy link
Contributor

Stukalov-A-M commented May 29, 2024

Also, I don't think there is any sort of validation on whether one account can make another account the executing authority for a trigger. I see that the (registering) authority field passed to the execute method is unused. Maybe that should be a permission?

I think @Stukalov-A-M took care of this in #4616? if not let's make a separate ticket

Yea, I've made this validation. ref. client/tests/integration/triggers/by_call_trigger.rs
fn only_account_with_permission_can_register_trigger()

There are only 3 scenarios when a trigger can be registered:

  1. The transaction signer is an Action.authority
  2. The transaction signer is a domain owner for Action.authority
  3. The transaction signer has a permission to register trigger with Action.authority on behalf of Action.authority

@Stukalov-A-M
Copy link
Contributor

can we say this. There are 2 accounts, the one that registers the trigger and the one that executes(owns?) the trigger? They can but don't have to be the same.

There is a mess with authorities )))

It's possible only if account that registers trigger has an appropriate permission.

Would it make more sense to remove Action::authority and add TriggerId::account_id?

Sound good! But not here TriggerId::account_id, here Trigger::owner

@Stukalov-A-M
Copy link
Contributor

@mversic yes, the registering authority and the executing authority are not necessarily the same. I guess if we move the executing authority's AccountId inside TriggerId, it will allow registering the same trigger name under different accounts (not sure if it's an error to register a duplicate object or it just rewrites the current thing). Trigger IDs would look like my_trigger$alice@wonderland instead of just my_trigger. Do we want that?

There is no need to complicate a TriggerId imho

@Erigara
Copy link
Contributor

Erigara commented May 30, 2024

I agree that there is no need to complicate TriggerId

@nxsaken
Copy link
Contributor Author

nxsaken commented May 30, 2024

How should a trigger be unregistered when its action's authority or domain gets unregistered? What if someone unregisters an account that was the authority for a trigger, yet the one unregistering the account doesn't have the authority to unregister the trigger?

And should unregistering those orphaned triggers emit events as if Unregister<Trigger> was executed directly from Unregister<Account/Domain>?

Did some digging on how orphan accounts have been handled so far. When a domain is removed, its accounts and asset definitions are removed naturally because they are stored inside the domain, but triggers are stored separately, yet are associated with accounts.

@Mingela
Copy link
Contributor

Mingela commented May 30, 2024

How should a trigger be unregistered when its action's authority or domain gets unregistered? What if someone unregisters an account that was the authority for a trigger, yet the one unregistering the account doesn't have the authority to unregister the trigger?

I'd suggest to have a reference between an account and the triggers for which the account is the authority. Thus deleting all associated triggers on the account unregistering. Basically this way a trigger nesting would migrate from a domain to an account.

@nxsaken nxsaken changed the title refactor: remove association between domains and triggers refactor: remove domain from trigger identity May 30, 2024
@s8sato
Copy link
Contributor

s8sato commented May 30, 2024

Speaking of preventing orphaned triggers, I guess trigger will be able to be identified as an account. This can be applied to cases where the registrant is not necessarily superior to the trigger authority. It would be common for contract proposer to be subject to the contract. See #4670

EDIT: created #4672 for orphaned entities in general

@mversic
Copy link
Contributor

mversic commented Jun 6, 2024

@nxsaken can you add a mechanism to remove all triggers owned by an account on Unregister<Account>?

@nxsaken
Copy link
Contributor Author

nxsaken commented Jun 6, 2024

@mversic I think I have here, or do you mean all triggers registered by one?

mversic
mversic previously approved these changes Jun 6, 2024
@mversic mversic requested review from Erigara and dima74 June 6, 2024 08:48
@mversic
Copy link
Contributor

mversic commented Jun 6, 2024

@nxsaken rebase and ping for approval

Signed-off-by: Nurzhan Sakén <nurzhan.sakenov@gmail.com>
Signed-off-by: Nurzhan Sakén <nurzhan.sakenov@gmail.com>
Signed-off-by: Nurzhan Sakén <nurzhan.sakenov@gmail.com>
Signed-off-by: Nurzhan Sakén <nurzhan.sakenov@gmail.com>
@mversic mversic merged commit c8b8d7b into hyperledger:main Jun 6, 2024
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries Refactor Improvement to overall code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove a redundant trigger-domain_id connection
8 participants