[feature] #2073: Prefer ConstString over String

[ales-tsurko]

Description of the Change

Replaces String with ConstString for types stored in the blockchain. It also encapsulates iroha_data_primitives into a shared crate.

Issue

Closes #2073

Benefits

Possible Drawbacks

Usage Examples or Tests [optional]

Alternate Designs [optional]

[codecov]

Codecov Report

Merging #2505 (892eb81) into iroha2-dev (4f81009) will decrease coverage by 0.32%.
The diff coverage is 54.95%.

@@              Coverage Diff               @@
##           iroha2-dev    #2505      +/-   ##
==============================================
- Coverage       67.61%   67.28%   -0.33%     
==============================================
  Files             140      140              
  Lines           26173    26428     +255     
==============================================
+ Hits            17696    17783      +87     
- Misses           8477     8645     +168     

Impacted Files	Coverage Δ
actor/src/broker.rs	88.05% <ø> (ø)
cli/src/main.rs	1.35% <0.00%> (ø)
cli/src/torii/mod.rs	28.88% <ø> (ø)
client_cli/src/main.rs	0.26% <0.00%> (ø)
core/src/kura.rs	90.02% <ø> (ø)
core/src/lib.rs	100.00% <ø> (ø)
core/src/smartcontracts/isi/asset.rs	54.82% <ø> (ø)
core/src/smartcontracts/isi/mod.rs	56.77% <ø> (+3.90%)	⬆️
core/src/smartcontracts/isi/triggers.rs	0.00% <0.00%> (ø)
data_model/src/events/execute_trigger.rs	7.40% <ø> (ø)
... and 93 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

[mversic]

we don't need ConstString in tests. We also don't need it when returning errors. It mostly makes sense to use it for things that are stored in memory and never modified. An example of this is PublicKey::digest in iroha_crypto or IpfsPath in data_model

[ales-tsurko]

we don't need ConstString in tests. We also don't need it when returning errors. It mostly makes sense to use it for things that are stored in memory and never modified. An example of this is PublicKey::digest in iroha_crypto or IpfsPath in data_model

OK, I'll remove it in tests and errors where possible, but somewhere it's updated because of dependencies.

It mostly makes sense to use it for things that are stored in memory and never modified.

This part of the task is not very clear to me. For example, we initialize an error with string, this string is stored in memory and it's never modified afterwards. Actually, there's a very rare case when we need to modify String, so these requirements can be applied to a lot of types, including errors and those from the tests.

[mversic]

This part of the task is not very clear to me. For example, we initialize an error with string, this string is stored in memory and it's never modified afterwards. Actually, there's a very rare case when we need to modify String, so these requirements can be applied to a lot of types, including errors and those from the tests.

what you say is true but the benefit is marginal. We need ConstString only for types that are stored on the blockchain

[ales-tsurko]

We need ConstString only for types that are stored on the block

I got it. Thanks!

[mversic]

Looks good
I see you dropped implementation on Peer::address. I support this revert.

[mversic]

I'd consider naming it primitives. Let's see what the other reviewer would have to say

[ales-tsurko]

I'd rename it into primitives as well. Just kept existing name. Should I rename it?

[ales-tsurko]

Will revert PeerId::address, ok

[mversic]

Will revert PeerId::address, ok

I don't think there is anything to change now. All is good

[ales-tsurko]

What about renaming data_primitives into primitives?

[mversic]

What about renaming data_primitives into primitives?

Now I see data_model/primitives was already a separate crate. I think you could have referenced it directly from iroha_crypto without moving it outside of data_model?

[ales-tsurko]

Well, anyway it had a strange design. It was re-exported from data_model, but still used somewhere directly. I don't recall why I didn't (or couldn't) use it directly, but then I already knew that it's a separate crate. Anyway, it's better to make it shared only or private only to omit confusions and inconsistency.

UPD

I've just checked it on iroha2-dev branch and actually yes, I could use it directly. Well, maybe because it was a deep night or something alike, I don't know how I skipped it 😅

[mversic]

then just rename to primitives and all is good

[Erigara]

also don't mind renaming it to just primitives

[Erigara]

also don't mind renaming it to just primitives