feat!: unify executor data model #4597

0x009922 · 2024-05-16T04:19:07Z

Reopening #4596 due to some GitHub force-push branch-recreate issues

Description

This PR solves a few problems:

Allows executor define its config parameters through a single ExecutorDataModel entity.
Unifies parameters and permission tokens to ExecutorDataModelObject trait, having a definition_id and an arbitrary JSON payload (possible to support SCALE too):
- dynamic parameter value #4352
- [suggestion] Move NewParameter into Executor #3901
Parameters no longer configure Sumeragi/WASM/limits. I.e. chain wide configuration could only be set through the config file. It is temporary: this PR is a preparation for a proper implementation of chain-wide configuration: [discussion] Align configuration-related instructions with the RFC #4028
Removes extra PermissionTokenSchemaUpdated event. Since this action only happens during executor migration, which emits its own event ExecutorEvent::Upgraded, I also included the updated ExecutorDataModel in this event.

Linked issue

Closes #4352
Closes #3901

data_model/src/executor.rs

data_model/src/permission.rs

data_model/src/executor.rs

data_model/src/visit.rs

data_model/src/query/mod.rs

data_model/src/events/data/events.rs

data_model/src/permission.rs

core/src/state.rs

data_model/src/events/data/events.rs

data_model/src/executor.rs

smart_contract/executor/src/lib.rs

smart_contract/executor/src/permission.rs

data_model/src/lib.rs

client/tests/integration/asset.rs

client/tests/integration/multisignature_transaction.rs

client/tests/integration/smartcontracts/executor_with_custom_parameter/src/lib.rs

client/tests/integration/upgrade.rs

core/src/smartcontracts/wasm.rs

data_model/src/events/data/filters.rs

data_model/src/permission.rs

0x009922 · 2024-05-22T01:37:47Z

#4635 should be merged first. This PR is currently rebased on top of it.

mversic · 2024-05-22T08:40:38Z

data_model/src/lib.rs

+}
+
+/// A helper trait for polymorphism, implemented for various types
+pub trait IntoJsonString {


why is this trait necessary?

It is not necessary.

It allows all of that:

let id = "SomeId".parse().unwrap(); Permission::new(id, JsonString::from(&json!(null))); Permission::new(id, &json!(null)); Permission::new(id, json!(null));

I find iroha_data_model to lack polymorphism in constructors a lot, actually.

data_model/src/lib.rs

client/tests/integration/extra_functional/unregister_peer.rs

client/tests/integration/permissions.rs

mversic · 2024-05-22T09:01:50Z

client/tests/integration/parameters.rs

+
+    assert_eq!(
+        client
+            .request(client::parameter::all())?


this query should return core parameters as well

This PR removes "core parameters" entirely from Iroha, temporarily. Parameter is now related only to executor-defined ones.

After merging of this PR, I will implement core parameters properly, according to #4028 (comment).

For now chain-wide parameters are only configured via config file.

client/tests/integration/smartcontracts/executor_with_custom_parameter/src/lib.rs

client/tests/integration/smartcontracts/executor_remove_permission/src/lib.rs

client/tests/integration/smartcontracts/executor_with_custom_permission/src/lib.rs

client_cli/src/main.rs

mversic · 2024-05-22T09:22:16Z

core/src/smartcontracts/isi/world.rs

+        state_transaction
+            .world
+            .parameters
+            .get_mut()
+            .retain(|parameter| {
+                state_transaction
+                    .world
+                    .executor_data_model
+                    .parameters
+                    .contains(&parameter.id)
+            });


this looks as if it also removes core parameters?

Replied in other thread (in sum: there are no core parameters, yet)

mversic · 2024-05-22T09:25:15Z

core/src/smartcontracts/isi/world.rs

+            if !world.executor_data_model.parameters.contains(&parameter.id) {
+                return Err(FindError::Parameter(parameter.id).into());


what if the parameter being set is a core parameter?

Replied in other thread.

default_executor/src/lib.rs

smart_contract/executor/derive/src/permission.rs

smart_contract/executor/src/default/permissions.rs

smart_contract/src/lib.rs

smart_contract/executor/src/parameter.rs

smart_contract/executor/src/permission.rs

smart_contract/executor/src/lib.rs

dima74 · 2024-05-22T11:03:04Z

tools/kagami/Cargo.toml

@@ -13,7 +13,7 @@ license.workspace = true
 workspace = true

 [dependencies]
-iroha_crypto = { workspace = true }
+iroha_crypto = { workspace = true, features = ["rand"] }


rand is in default-features of iroha-crypto, looks like no need to add it here

iroha_crypto is used in workspace without default-features (because it also gets used in wasm crates)

I think we have this setup because there were previously some bugs around this in cargo: rust-lang/cargo#11329

Oh, it's actually not a bug, default-features = true in workspace crate makes the default features always enabled. The fix to that issue was to add a warning when you try to disable default features on a workspace dependency..

Bottom line: either the rand feature or default features as a whole must be enabled explicitly when using workspace dependencies like this (at least the ones which have default features disabled in the workspace). I think it's fine to have only rand enabled, though putting default-features = true here would also work

rand is in default-features of iroha-crypto, looks like no need to add it here

Doesn't compile with cargo check -p kagami

I think it's fine to have only rand enabled, though putting default-features = true here would also work

Not sure what is better, leaving as-is.

smart_contract/executor/src/default/permissions.rs

mversic · 2024-05-24T05:59:20Z

smart_contract/executor/src/permission.rs

+    /// Try to convert from [`PermissionObject`]
+    /// # Errors
+    /// See [`TryFromDataModelObjectError`]
+    fn try_from_object(object: &PermissionObject) -> Result<Self, TryFromDataModelObjectError> {
+        if *object.id() != <Self as Permission>::id() {
+            return Err(TryFromDataModelObjectError::Id(object.id().name().clone()));
+        }
+        object
+            .payload()
+            .deserialize()
+            .map_err(TryFromDataModelObjectError::Deserialize)
+    }
+
+    /// Convert into [`PermissionObject`]
+    fn to_object(&self) -> PermissionObject {
+        PermissionObject::new(
+            <Self as Permission>::id(),
+            JsonString::serialize(&self)
+                .expect("failed to serialize concrete data model entity; this is a bug"),
+        )
+    }


what was wrong with having implemented TryFrom and From?

🤔 good question

Ah, yes. Because:

impl<T: Parameter> TryFrom<&ParameterObject> for T { // ^^^^^^^^^^^^^^^^^^^^^^^^^ // Only traits defined in the current crate can be implemented for arbitrary types type Error = TryFromDataModelObjectError; fn try_from(value: &ParameterObject) -> core::result::Result<Self, Self::Error> { todo!() } }

you can't make a blanket implementation, but you can derive for each type on #[derive(PermissionTrait)]

Then we also need to introduce a derive macro for ParameterTrait, and I wouldn't want it just for the sake of this conversion.

smart_contract/executor/src/lib.rs

smart_contract/executor/derive/src/default.rs

smart_contract/executor/derive/src/entrypoint.rs

smart_contract/executor/src/lib.rs

Co-authored-by: Marin Veršić <marin.versic101@gmail.com> Co-authored-by: Nikita Strygin <dcnick3@users.noreply.github.com> Co-authored-by: Shanin Roman <shanin1000@yandex.ru> Co-authored-by: Dmitry Murzin <diralik@yandex.ru> Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>

Signed-off-by: 0x009922 <43530070+0x009922@users.noreply.github.com>

0x009922 · 2024-05-27T09:47:23Z

Drafted due to some questions related to chain-wide config design. More on that commented here: #4028 (comment)

0x009922 · 2024-05-30T01:39:57Z

Close in favor of #4658

0x009922 self-assigned this May 16, 2024

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

0x009922 mentioned this pull request May 16, 2024

feat!: unify executor data model #4596

Closed

0x009922 force-pushed the unified-executor-data-model branch 2 times, most recently from 1520c94 to ab863c0 Compare May 17, 2024 00:08

mversic requested changes May 17, 2024

View reviewed changes

0x009922 mentioned this pull request May 20, 2024

[discussion] Split setting of permissions and parameters from within Executor #4618

Open

0x009922 force-pushed the unified-executor-data-model branch 2 times, most recently from 67af3ba to 0979da0 Compare May 20, 2024 07:22

0x009922 requested a review from mversic May 20, 2024 07:23

0x009922 assigned mversic May 20, 2024

DCNick3 self-assigned this May 20, 2024

mversic mentioned this pull request May 20, 2024

[refactor] #3901: Add method to define parameter in executor migrate entrypoint #4513

Closed

5 tasks

DCNick3 reviewed May 20, 2024

View reviewed changes

0x009922 force-pushed the unified-executor-data-model branch from 0979da0 to 67b7e71 Compare May 20, 2024 21:36

0x009922 marked this pull request as ready for review May 20, 2024 21:37

0x009922 requested review from s8sato, SamHSmith, outoftardis, Erigara, VAmuzing, Asem-Abdelhady and dima74 as code owners May 20, 2024 21:37

0x009922 requested a review from DCNick3 May 21, 2024 03:53

mversic reviewed May 21, 2024

View reviewed changes

client/tests/integration/asset.rs Outdated Show resolved Hide resolved

Erigara reviewed May 21, 2024

View reviewed changes

0x009922 mentioned this pull request May 21, 2024

refactor: rename PermissionToken to Permission #4635

Merged

0x009922 force-pushed the unified-executor-data-model branch from 248973e to 07e10db Compare May 22, 2024 01:35

0x009922 force-pushed the unified-executor-data-model branch from 07e10db to 3a09ab6 Compare May 22, 2024 06:52

mversic requested changes May 22, 2024

View reviewed changes

mversic assigned dima74 May 22, 2024

mversic reviewed May 22, 2024

View reviewed changes

client/tests/integration/smartcontracts/executor_with_custom_permission/src/lib.rs Outdated Show resolved Hide resolved

mversic reviewed May 22, 2024

View reviewed changes

client_cli/src/main.rs Outdated Show resolved Hide resolved

mversic requested changes May 22, 2024

View reviewed changes

dima74 reviewed May 22, 2024

View reviewed changes

0x009922 force-pushed the unified-executor-data-model branch 2 times, most recently from 76f18a9 to 09995af Compare May 23, 2024 07:00

0x009922 requested review from mversic and dima74 May 23, 2024 10:09