great work

So, I guess this PR should came after #4597 and extend ExecutorDataModel with a set of instruction types, and then to have Custom instruction somehow this way:

struct Custom {
    id: CustomInstructionId, // or just Name?
    payload: JsonString
}

I think you should make ExecutorDataModel a separate pr. And yeah, I was thinking that Custom should have an id field as you present here, but maybe there are some drawbacks?

I think you should make ExecutorDataModel a separate pr.

Created #4658

And yeah, I was thinking that Custom should have an id field as you present here, but maybe there are some drawbacks?

IMO, technically, there is no need to have an id. This would suffice:

struct ExecutorDataModel {
  schema: String,

  // -- snip --

  // type id in the schema
  custom_instruction_definition_id: Option<Name>
}

enum Instruction {
  // -- snip --
  Custom(JsonString)
}

Clients will know how to serialize Custom if they look into the data model.

IMO, technically, there is no need to have an id. This would suffice:

there is obviously no need in this PR, but I still maintain it might be useful. Maybe for migrations. Will iroha_core ever have to know that two custom instructions are of the same type, i.e. will it have to compare them by id. When Custom instruction contains only payload it's not possible to do so without invoking executor to evaluate the whole instruction. I think id brings a little more flexibility and makes it more future proof

@Erigara do you have opinion on the matter?

I think id brings a little more flexibility and makes it more future proof

I see your point, though, it sounds vague and this possible future-proof solution doesn't sound very robust to me. Due to that, I would prefer a minimal implementation now, as we are not 100% sure that having id would help in future.

I think id brings a little more flexibility and makes it more future proof

I see your point, though, it sounds vague and this possible future-proof solution doesn't sound very robust to me. Due to that, I would prefer a minimal implementation now, as we are not 100% sure that having id would help in future.

I don't have strong arguments for adding instruction id. Except that we could have nicer Display/Debug implementation, without dumping the whole payload. If we decide that instructions won't have ids, then permissions shouldn't either

I'd like some more opinions here: @Erigara @DCNick3

I see your point, though, it sounds vague and this possible future-proof solution doesn't sound very robust to me. Due to that, I would prefer a minimal implementation now, as we are not 100% sure that having id would help in future.

I didn't want to focus on future-proofing. It was more about iroha_core being able to categorize instructions. Because now there is no way for it to know whether 2 Custom instructions are of the same type nominally. Custom instructions are completely opaque

Looks cool, give ability to implement custom instructions without too much changes.

One concern i can think of is that custom instructions work more like an extension, so executor can't for example completely replace isi with own DSL for example.

Alternative design i was thinking about was to make transaction payload opaque to the core iroha (just bytes essentially).
So executor has complete freedom in how to interpret this payload.
But this would involve much more work and i'm not sure we need such extensibility in the first place.

It was more about iroha_core being able to categorize instructions.

Do we need this ability since whole custom isi execution is on the executor?
In general i don't see strong need in adding id.

moreover, remove Permission::id in a separate PR to be consistent with this one here

having ids for permissions might be still helpful because we do lookups on then which is not the case for instructions

moreover, remove Permission::id in a separate PR to be consistent with this one here

having ids for permissions might be still helpful because we do lookups on then which is not the case for instructions

but the lookups are completely unnecessary. They only happen because we keep list of permission ids in wsv. There is no need for that

moreover, remove Permission::id in a separate PR to be consistent with this one here

having ids for permissions might be still helpful because we do lookups on then which is not the case for instructions

but the lookups are completely unnecessary. They only happen because we keep list of permission ids in wsv. There is no need for that

created #4702 for further discussion, but I don't see any reason to keep Permission::id if we don't have it for instructions