[feature] #55: Add unregister account and domain #60
Conversation
Signed-off-by: Vladimir Pesterev <pesterev@pm.me>
There was a problem hiding this comment.
Overall this is a fine first approximation. However, I would approach this from a different angle.
Registering and immediately unregistering objects isn't very realistic. Much more realistic would be a way to unregister existing domains by querying the blockchain and unregistering something which already existed in more than this block.
| Operation::UnregisterDomain => { | ||
| // Register a domain | ||
| let new_domain_name = | ||
| Name::from_str(format!("wonderland_unregister_{}", index).as_str()) | ||
| .expect("Failed to create a new domain name"); | ||
| let new_domain_id = DomainId::new(new_domain_name); | ||
| let register_domain_box = RegisterBox::new(Domain::new(new_domain_id.clone())); | ||
|
|
||
| // Unregister a domain | ||
| let unregister_domain_box = UnregisterBox::new(IdBox::DomainId(new_domain_id)); | ||
|
|
||
| vec![vec![ | ||
| register_domain_box.into(), | ||
| unregister_domain_box.into(), | ||
| ]] | ||
| } |
There was a problem hiding this comment.
From my perspective, it would be better to query for an existing domain and unregister it. Current implementation is not "real-life-like".
It seems that this change requires big refactoring, bacause currently each operation is mapped into a set of instructions to perform later, which... is not actually what operation is, as I proposed at #30. Operation might have its own complex logic with multiple steps consisted of queries, transactions and even events.
There was a problem hiding this comment.
I tried to generalize all operations and got this inconvenient design: Operation -> Instructions iterator where on every iteration the tool submits a set of instructions. That's why the Instructionss iterator is Vec<Vec<Operation>>.
I understood your proposal. According to it, we have to design every Operation separately with its own queries and instructions, as I correctly understand.
My suggestion is to create modules following this structure:
- src/
- ...
- operation/
- mod.rs
- register_account.rs
- register_domain.rs
- ...
- transfer_asset.rs
Every file will contain Operation that implements the following trait:
trait Operation {
fn execute(&mut self, status: Arc<Status>, client: Client, index: usize, test_env: TestEnv);
}And also it seems we have to correctly update Status. For that, we need to enhance the structure of Status:
struct Status {
count_by_operation: Arc<Mutex<HashSet<Operation, usize>>>,
total_count: usize,
// ...
}@0x009922 to be honest, I don't know what format of data is needed as the status info.
There was a problem hiding this comment.
@pesterev, what kind of data do you need to know about each operation? How many transactions were committed/rejected?
About Operation trait - why do we need to pass status, index and test_env into an operation in order to execute it? I could miss something, but can't we just execute it with &Client?
About index - am I right that it is used only to avoid overlaps with already created entities? If I am, then... it might be difficult to re-run the utility in order to produce more entities. Imagine the case: some developer wants to fill Iroha with 5 random accounts. They run the tool in "One shot" mode with RegisterAccount operation, let's say, in a single tool run, and it produces alice1, alice2... accounts. Then the developer wants to produce 5 more accounts. How could he avoid overlaps?
In my mind, Operation trait looks somehow like this:
trait Operation {
fn execute(self, client: &Client) -> OperationResult;
}
struct OperationResult {
txs_committed: usize,
// other data common for any operation
}OperationResult might contain any data needed, for example, to update some global Status. In this approach Operation no longer depends on actual Status struct to be passed in as a mutable reference (dependency inversion in action).
Task
Closes #55
Changes
This PR adds new operations related to unregistering accounts and domains.
Author
Signed-off-by: Vladimir Pesterev pesterev@soramitsu.co.jp