contracts: Refactor trait Ext::*_storage_transparent functions

[pgherveou]

fix #13594

[pgherveou]

/ptal @athei

[pgherveou]

The obvious solution is to refactor the tests to no longer use Ext as an object. Maybe there are other solutions for removing the code duplication.

Went for something a bit easier, to keep the trait object safe.
I moved both the static and variable size key into a new combined enum type.

Generics only fails because the test is using Ext as an object as you mentioned, I can look into doing what you suggested as well, if you think this is a better solution

[athei]

I agree. Refactoring the tests is probably not worth it.

[pgherveou]

@athei replied to your comments, and applied some of the suggestions.

[pgherveou]

pushed another commit to make these methods take the key by value, since it looks like they are consumed by these methods and don't need to be passed around to other calls.
We can change these signatures and use impl Into<Key<Self::T>> and get rid of all the into(), later on if we refactor the test to not be objectsafe

substrate/frame/contracts/src/exec.rs

Lines 186 to 192 in a449aa6

	fn get_storage(&mut self, key: Key<Self::T>) -> Option<Vec<u8>>;
	/// Returns `Some(len)` (in bytes) if a storage item exists at `key`.
	///
	/// Returns `None` if the `key` wasn't previously set by `set_storage` or
	/// was deleted.
	fn get_storage_size(&mut self, key: Key<Self::T>) -> Option<u32>;

[athei]

I pushed another commit to make these methods take the key by value, since it looks like they are consumed by these methods and don't need to be passed around to other calls.

Is this enough reason to make it pass by value? Usually, when both is viable I go for reference: Passing by value makes the compiler copy the value. I know that logically it is moved but the optimizer is not smart enough to demote this to a pass by reference. Since we are passing an array the whole thing will be copied. For VarSizedKey it is less severe since it is a Vec. But since Key is an enum you even copy the array in this case.

I am just saying: It is not really consumed. It just happens to be not being used after being passed into those functions. I think if you can pass something by reference without having to call clone somewhere you should go for this.

impl Into<Key<Self::T>> would be impl AsRef<Key<Self::T>> in this case.

[pgherveou]

I pushed another commit to make these methods take the key by value, since it looks like they are consumed by these methods and don't need to be passed around to other calls.

Is this enough reason to make it pass by value? Usually, when both is viable I go for reference: Passing by value makes the compiler copy the value. I know that logically it is moved but the optimizer is not smart enough to demote this to a pass by reference. Since we are passing an array the whole thing will be copied. For VarSizedKey it is less severe since it is a Vec. But since Key is an enum you even copy the array in this case.

I am just saying: It is not really consumed. It just happens to be not being used after being passed into those functions. I think if you can pass something by reference without having to call clone somewhere you should go for this.

impl Into<Key<Self::T>> would be impl AsRef<Key<Self::T>> in this case.

I pushed another commit to make these methods take the key by value, since it looks like they are consumed by these methods and don't need to be passed around to other calls.

Is this enough reason to make it pass by value? Usually, when both is viable I go for reference: Passing by value makes the compiler copy the value. I know that logically it is moved but the optimizer is not smart enough to demote this to a pass by reference. Since we are passing an array the whole thing will be copied. For VarSizedKey it is less severe since it is a Vec. But since Key is an enum you even copy the array in this case.

I am just saying: It is not really consumed. It just happens to be not being used after being passed into those functions. I think if you can pass something by reference without having to call clone somewhere you should go for this.

impl Into<Key<Self::T>> would be impl AsRef<Key<Self::T>> in this case.

Did not know the perf implication, will try to remember that.
So you only use value when you want to design an API that change ownership, else you default to reference if you have the choice

[athei]

So you only use value when you want to design an API that change ownership, else you default to reference if you have the choice

Exactly

[pgherveou]

@athei should be good to go, rolled back the commit that used value instead of references and remove the type aliases(kept VarSizedKey but made it private)

[athei]

Looking good. Just some nits.

[agryaznov]

Nice refactoring! LGTM

[pgherveou]

mmm
need to look into this failing job

[2023-03-17 12:49:02]     Checking cumulus-client-consensus-aura v0.1.0 (/builds/parity/mirrors/substrate/cumulus/client/consensus/aura)
[2023-03-17 12:49:02] error[E0432]: unresolved import `sp_keystore::SyncCryptoStorePtr`
[2023-03-17 12:49:02]   --> client/consensus/aura/src/lib.rs:43:5
[2023-03-17 12:49:02]    |
[2023-03-17 12:49:02] 43 | use sp_keystore::SyncCryptoStorePtr;
[2023-03-17 12:49:02]    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `SyncCryptoStorePtr` in the root
[2023-03-17 12:49:02] 
[2023-03-17 12:49:02] For more information about this error, try `rustc --explain E0432`.
[2023-03-17 12:49:02] error: could not compile `cumulus-client-consensus-aura` due to previous error
[2023-03-17 12:49:02] warning: build failed, waiting for other jobs to finish...
[2023-03-17 12:49:02] error: could not compile `cumulus-client-consensus-aura` due to previous error
[2023-03-17 12:51:19] 
Running after_script
00:02
Running after script...
$ env RUSTY_CACHIER_SUPRESS_OUTPUT=true rusty-cachier snapshot destroy
Cleaning up project directory and file based variables
00:01
ERROR: Job failed: exit code 1

looks unrelated to this PR to me 🤔
rebasing...

[athei]

Nice