-
Notifications
You must be signed in to change notification settings - Fork 277
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
feat!: recognize and activate accounts #4668
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- unit tests for all patterns of account recognition
/// # 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This public scenario describes the main goal of this PR
Action::new( | ||
WasmSmartContract::from_compiled(wasm), | ||
Repeats::Indefinitely, | ||
ALICE_ID.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// # 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For private scenarios, the aim is no impact on membership and privacy while relaxing restrictions on the order of registering and targeting accounts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
// 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)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
data_model/src/account.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let's stay focused
core/src/smartcontracts/isi/query.rs
Outdated
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern only appears in core unit tests, so I provided a utility in its tests
module
core/src/executor.rs
Outdated
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); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be verified before calling executor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this way it seems like it's optional. Was that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you paraphrase, what seems like optional?
core/src/smartcontracts/isi/query.rs
Outdated
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
state_transaction | ||
.world | ||
.emit_events(Some(DomainEvent::Account(AccountEvent::Recognized(id)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design required emitting this data event to trigger the activator
Ok(existing_account) => { | ||
if existing_account.is_active { | ||
return Err(RepetitionError { | ||
instruction_type: InstructionType::Register, | ||
id: IdBox::AccountId(account_id), | ||
} | ||
.into()); | ||
} | ||
|
||
existing_account.activate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part is now weird because NewAccount
can carry more information (like some metadata) that you just disregard
This is not how I would implement it. I would rather just control the access through executor as follows:
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 |
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>
d72a0df
to
eb7e1e3
Compare
As I understand it, according to @mversic your suggestion, the process when an account is targeted is as follows:
This means that no intermediate state is needed for account registration. If so, let's remove Now, there are two possible approaches in terms of granularity of the account auto-registration policy:
Dealing with the concept of domains and public/private uses at the same time is new to me, and I seek further input |
wasn't that how you implemented, or was there something different?
yes
yes |
can't executor make sure account is registered before calling |
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) |
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. |
The feature might require the separate and relational entities after all.
I'll extend this idea to other ISIs and other entity maps and consider consistency. |
IMHO yes, but that should be confirmed. In private use-case it's expected users are registered before becoming entities in the network |
So the idea is to deliberately introduce an inconsistency, the absence of a primary entity in |
Closed as the issue should be addressed by a different approach such as #4668 (comment) |
BREAKING_CHANGE:
Account
containsis_active
fieldAccountEvent::Created
divides intoRecognized
andActivated
ValidationFail
containsUnrecognizedAuthority
andInactiveAuthority
variantsDescription
When an account is recognized,
When an account is activated,
Recognize and activate an account when targeted as:
Register<Account>
objectRecognize an account when targeted as:
Register<Asset>
object.accountMint<Numeric, Asset>
destination.accountSee commentTransfer<Account, DomainId, Account>
destinationTransfer<Account, AssetDefinitionId, Account>
destinationTransfer<Asset, Numeric, Account>
destinationTransfer<Asset, Metadata, Account>
destinationSetKeyValue<Account>
objectSetKeyValue<Asset>
object.accountGrant<Permission, Account>
destinationGrant<Role, Account>
destinationLet me call these creative instructions in documentation
Linked issue
Benefits
Checklist
CONTRIBUTING.md