feat!: recognize and activate accounts #4668

s8sato · 2024-05-30T03:01:16Z

BREAKING_CHANGE:

Account contains is_active field
AccountEvent::Created divides into Recognized and Activated
ValidationFail contains UnrecognizedAuthority and InactiveAuthority variants

Description

When an account is recognized,

it becomes able to hold assets, permissions, roles, and metadata
it cannot yet execute any queries or transactions

When an account is activated,

it becomes able to execute queries and transactions

Recognize and activate an account when targeted as:

Register<Account> object

Recognize an account when targeted as:

Register<Asset> object.account
Mint<Numeric, Asset> destination.account
~~Transfer<Account, DomainId, Account> destination~~ See comment
Transfer<Account, AssetDefinitionId, Account> destination
Transfer<Asset, Numeric, Account> destination
Transfer<Asset, Metadata, Account> destination
SetKeyValue<Account> object
SetKeyValue<Asset> object.account
Grant<Permission, Account> destination
Grant<Role, Account> destination

Let me call these creative instructions in documentation

Linked issue

closes Target accounts before registration #4426

Benefits

who targets an account does not need to care if it has been registered or not
depending on use case, accounts can be registered implicitly/automatically or explicitly/manually

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

s8sato

unit tests for all patterns of account recognition

s8sato · 2024-05-30T03:03:40Z

client/tests/integration/account_registration/public.rs

+/// # Scenario
+///
+/// 0. ... administrative trigger registered
+/// 0. new carol targeted ... carol recognized
+/// 0. clock forward by one block ... carol activated
+/// 0. carol tries query ... ok
+/// 0. carol tries transaction ... ok


This public scenario describes the main goal of this PR

s8sato · 2024-05-30T03:04:08Z

client/tests/integration/account_registration/public.rs

+        Action::new(
+            WasmSmartContract::from_compiled(wasm),
+            Repeats::Indefinitely,
+            ALICE_ID.clone(),


The trigger authority is tentatively set to alice, but once the account ID is expanded to off-curve multihash, this should be a system (non-personal) authority

Is there an issue for it?

I don't think it has been broken down into issues yet.
If we want to support use cases where a trigger needs different permission than the registrant account (e.g. swap between two accounts, #1212), the trigger should reference its own account (or should be identified as an account).
The ideas of system level triggers and technical accounts were suggested in as early as Triggers ADR.
Similar concepts include Contract Address and Program Derived Addresses

I found my first comment off topic. In this use case the trigger manages domain membership, so alice (domain owner) would be an appropriate authority

created identify trigger as an account? #4670

s8sato · 2024-05-30T03:04:34Z

client/tests/integration/account_registration/private.rs

+/// # Scenario
+///
+/// 0. new carol targeted ... carol recognized
+/// 0. carol tries query ... err
+/// 0. carol tries transaction ... err
+/// 0. register carol ... carol activated
+/// 0. carol tries query ... ok
+/// 0. carol tries transaction ... ok


For private scenarios, the aim is no impact on membership and privacy while relaxing restrictions on the order of registering and targeting accounts

s8sato · 2024-05-30T03:05:01Z

client/tests/integration/account_registration/mod.rs

The idea is to realize public/private characteristics not by different executors, but by different account activators (trigger/administrator). In this way, each domain can have different activation policies under the common executor

s8sato · 2024-05-30T03:05:39Z

core/src/smartcontracts/isi/domain.rs

+            // Exceptionally, the destination account should not be recognized here.
+            // Otherwise, the risk is that the previous owner can no longer activate the current owner who cannot yet take any action by oneself.
+            // Thus, the destination account should be explicitly registered before this transfer
            let _ = state_transaction.world.account(&destination_id)?;


It may be dangerous to recognize an account when targeted as Transfer<Account, DomainId, Account> destination. The risk is that the previous owner can no longer activate the current owner who cannot yet take any action by oneself

s8sato · 2024-05-30T03:06:04Z

data_model/src/account.rs

This PR keeps the nested structure of IDs as it is focused on feat. My another commit eliminated dependency between entity IDs and the event hierarchy, so hopefully entities can be placed in storages directly under the world. However, I'd leave it to another perf. PR with proper benchmarking and profiling

I agree, let's stay focused

core/src/executor.rs

nxsaken · 2024-05-30T07:05:54Z

core/src/smartcontracts/isi/query.rs

@@ -316,7 +316,8 @@ mod tests {
    fn world_with_test_domains() -> World {
        let domain_id = DomainId::from_str("wonderland").expect("Valid");
        let mut domain = Domain::new(domain_id).build(&ALICE_ID);
-        let account = Account::new(ALICE_ID.clone()).build(&ALICE_ID);
+        let mut account = Account::new(ALICE_ID.clone()).build(&ALICE_ID);


This frequent pattern of creating a new account and immediately activating it is asking for an Account::new_active / Account::activated constructor (marked with #[cfg(test)] probably).

yes, this is not something that client should be able to call. I think having a separate activate method is perfectly fine, but you should mark it with transparent_api

This pattern only appears in core unit tests, so I provided a utility in its tests module

client/tests/integration/account_registration/private.rs

client/tests/integration/account_registration/public.rs

mversic · 2024-06-06T09:29:12Z

core/src/executor.rs

+        if 0 != state_transaction.height() {
+            let Ok(authority_account) = state_transaction.world.account(authority) else {
+                return Err(ValidationFail::UnrecognizedAuthority);
+            };
+            if !authority_account.is_active {
+                return Err(ValidationFail::InactiveAuthority);
+            }
+        }


this should be verified before calling executor

this way it seems like it's optional. Was that intended?

Could you paraphrase, what seems like optional?

mversic · 2024-06-06T09:39:57Z

core/src/smartcontracts/isi/query.rs

@@ -316,7 +316,8 @@ mod tests {
    fn world_with_test_domains() -> World {
        let domain_id = DomainId::from_str("wonderland").expect("Valid");
        let mut domain = Domain::new(domain_id).build(&ALICE_ID);
-        let account = Account::new(ALICE_ID.clone()).build(&ALICE_ID);
+        let mut account = Account::new(ALICE_ID.clone()).build(&ALICE_ID);


yes, this is not something that client should be able to call. I think having a separate activate method is perfectly fine, but you should mark it with transparent_api

mversic · 2024-06-06T09:51:35Z

core/src/smartcontracts/isi/mod.rs

+    state_transaction
+        .world
+        .emit_events(Some(DomainEvent::Account(AccountEvent::Recognized(id))));


I don't think this event useful for the end user? seems more like an internal thing - iroha has started tracking this account in memory

The design required emitting this data event to trigger the activator

core/src/smartcontracts/isi/domain.rs

core/src/smartcontracts/isi/account.rs

mversic · 2024-06-06T10:04:03Z

core/src/smartcontracts/isi/domain.rs

+                Ok(existing_account) => {
+                    if existing_account.is_active {
+                        return Err(RepetitionError {
+                            instruction_type: InstructionType::Register,
+                            id: IdBox::AccountId(account_id),
+                        }
+                        .into());
+                    }
+
+                    existing_account.activate();


this part is now weird because NewAccount can carry more information (like some metadata) that you just disregard

mversic · 2024-06-06T10:22:32Z

This is not how I would implement it. I would rather just control the access through executor as follows:

By default all instructions that target accounts are allowed:

this covers public use
requires no executor (public case shouldn't require executor)

Custom executor rejects instructions that target accounts that have not yet been registered:

this covers private use
should be part of our default executor

I also believe this approach reduces the implementation complexity.

I don't think there is any need to support inactive accounts in private blockchains. In private blockchains accounts first have to get registered then they can be used. The is_active limbo doesn't bring anything for private case

BREAKING_CHANGE: - `Account` contains `is_active` field - `AccountEvent::Created` divides into `Recognized` and `Activated` - `ValidationFail` contains `UnrecognizedAuthority` and `InactiveAuthority` variants Signed-off-by: Shunkichi Sato <49983831+s8sato@users.noreply.github.com>

s8sato · 2024-06-07T07:40:22Z

As I understand it, according to @mversic your suggestion, the process when an account is targeted is as follows:

public use
- the target is immediately registered no matter what domain the target belongs to
  - may be reasonable, since the domain boundaries may not make sense in public chains
private use
- as before, only registered accounts can be valid targets

This means that no intermediate state is needed for account registration. If so, let's remove is_active flag and forget about recognize/activate terms.

Now, there are two possible approaches in terms of granularity of the account auto-registration policy:

core processes back to executor for auto-registering the target
- executor is responsible for registering targeted accounts
  - returns uniform results regardless of authority of the instruction
- chain level public/private characterization
- needs a flag to indicate if Register<Account> is called back from core for auto-registration
each domain has a flag, and core determines whether to auto-register the target from its domain, without processing back to executor
- core is responsible for registering targeted accounts
- domain level public/private characterization
- how should the flag be modified, if it should be?

Dealing with the concept of domains and public/private uses at the same time is new to me, and I seek further input

mversic · 2024-06-07T07:48:36Z

the target is immediately registered no matter what domain the target belongs to

wasn't that how you implemented, or was there something different?

as before, only registered accounts can be valid targets

yes

This means that no intermediate state is needed for account registration. If so, let's remove is_active flag and forget about recognize/activate terms.

yes

mversic · 2024-06-07T07:50:05Z

core processes back to executor for auto-registering the target

can't executor make sure account is registered before calling isi.execute(), i.e. handing over execution to the core

Erigara · 2024-06-11T06:37:12Z

I don't think there is any need to support inactive accounts in private blockchains.

What about offline transactions? do you think they are required only in public case?

Is full registration even possible automatically?

I mean that executor/trigger might have not enough data to perform registration (i.e. metadata)

Erigara · 2024-06-11T06:47:12Z

Requirement for auto registration emerge from our implementation details (account structs holds assets).

Shouldn't we reconsider this?

In this case we would allow isi where target account is not registered yet and if required we can prohibit this isi by checking if account is registered in the executor.

s8sato · 2024-06-13T09:08:51Z

The feature might require the separate and relational entities after all.
As I understand it, an example where a new account "carol" comes to exercise its authority in public use is as follows:

the world has separate assets and accounts maps

- world:
  - numeric_assets:
    - rose:
      - alice: 13
  - accounts:
    - alice:
      - metadata:

(public use) alice transfers 3 roses to carol, who is not registered yet

- world:
  - numeric_assets:
    - rose:
      - alice: 10
      - carol:  3 # appeared
  - accounts: # not checked when carol is targeted
    - alice:
      - metadata:

(public use) carol registers herself

- world:
  - numeric_assets:
    - rose:
      - alice: 10
      - carol:  3
  - accounts:
    - alice:
      - metadata:
    - carol: # registered
      - metadata:

carol successfully transfers 2 roses to alice

- world:
  - numeric_assets:
    - rose:
      - alice: 12
      - carol:  1
  - accounts: # checked when carol is authority
    - alice:
      - metadata:
    - carol:
      - metadata:

I'll extend this idea to other ISIs and other entity maps and consider consistency.
For example, I can immediately see that SetKeyValue<Account> would be invalid before account registration

mversic · 2024-06-14T09:43:48Z

What about offline transactions? do you think they are required only in public case?

IMHO yes, but that should be confirmed. In private use-case it's expected users are registered before becoming entities in the network

s8sato · 2024-07-02T05:58:40Z

So the idea is to deliberately introduce an inconsistency, the absence of a primary entity in ACCOUNTS in this diagram while other storages referencing it, and thereby represent a pre-registration state of the account. I'll convert this PR to draft (and maybe close it) and resume after #4792

s8sato · 2024-08-05T08:44:25Z

Closed as the issue should be addressed by a different approach such as #4668 (comment)

github-actions bot added the api-changes Changes in the API for client libraries label May 30, 2024

s8sato commented May 30, 2024

View reviewed changes

s8sato added the Enhancement New feature or request label May 30, 2024

s8sato force-pushed the feat/4426 branch from 19873bd to a8813f3 Compare May 30, 2024 03:22

s8sato marked this pull request as ready for review May 30, 2024 03:38

s8sato requested review from mversic, SamHSmith, outoftardis, Erigara, 0x009922, DCNick3, VAmuzing and dima74 as code owners May 30, 2024 03:38

nxsaken reviewed May 30, 2024

View reviewed changes

Erigara assigned s8sato May 30, 2024

Erigara reviewed May 30, 2024

View reviewed changes

client/tests/integration/account_registration/private.rs Outdated Show resolved Hide resolved

client/tests/integration/account_registration/public.rs Show resolved Hide resolved

s8sato mentioned this pull request May 30, 2024

identify trigger as an account? #4670

Open

mversic reviewed Jun 6, 2024

View reviewed changes

mversic requested changes Jun 6, 2024

View reviewed changes

s8sato force-pushed the feat/4426 branch from a8813f3 to dc1f946 Compare June 7, 2024 07:28

s8sato force-pushed the feat/4426 branch 2 times, most recently from d72a0df to eb7e1e3 Compare June 7, 2024 07:32

mversic assigned 0x009922 and mversic Jun 10, 2024

s8sato marked this pull request as draft July 2, 2024 05:59

s8sato mentioned this pull request Jul 2, 2024

Target accounts before registration #4426

Open

0x009922 removed their assignment Jul 21, 2024

s8sato closed this Aug 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: recognize and activate accounts #4668

feat!: recognize and activate accounts #4668

s8sato commented May 30, 2024 •

edited

Loading

s8sato left a comment

s8sato May 30, 2024

s8sato May 30, 2024

nxsaken May 30, 2024

s8sato May 30, 2024 •

edited

Loading

s8sato May 30, 2024

s8sato May 30, 2024

s8sato May 30, 2024

s8sato May 30, 2024

s8sato May 30, 2024

s8sato May 30, 2024

Erigara May 30, 2024

nxsaken May 30, 2024 •

edited

Loading

mversic Jun 6, 2024

s8sato Jun 7, 2024

mversic Jun 6, 2024

mversic Jun 6, 2024

s8sato Jun 7, 2024

mversic Jun 6, 2024

mversic Jun 6, 2024

s8sato Jun 7, 2024

mversic Jun 6, 2024

mversic commented Jun 6, 2024 •

edited

Loading

s8sato commented Jun 7, 2024

mversic commented Jun 7, 2024

mversic commented Jun 7, 2024 •

edited

Loading

Erigara commented Jun 11, 2024

Erigara commented Jun 11, 2024

s8sato commented Jun 13, 2024

mversic commented Jun 14, 2024

s8sato commented Jul 2, 2024 •

edited

Loading

s8sato commented Aug 5, 2024

feat!: recognize and activate accounts #4668

feat!: recognize and activate accounts #4668

Conversation

s8sato commented May 30, 2024 • edited Loading

Description

Linked issue

Benefits

Checklist

s8sato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s8sato May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nxsaken May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mversic commented Jun 6, 2024 • edited Loading

s8sato commented Jun 7, 2024

mversic commented Jun 7, 2024

mversic commented Jun 7, 2024 • edited Loading

Erigara commented Jun 11, 2024

Erigara commented Jun 11, 2024

s8sato commented Jun 13, 2024

mversic commented Jun 14, 2024

s8sato commented Jul 2, 2024 • edited Loading

s8sato commented Aug 5, 2024

s8sato commented May 30, 2024 •

edited

Loading

s8sato May 30, 2024 •

edited

Loading

nxsaken May 30, 2024 •

edited

Loading

mversic commented Jun 6, 2024 •

edited

Loading

mversic commented Jun 7, 2024 •

edited

Loading

s8sato commented Jul 2, 2024 •

edited

Loading