[contracts] define_env! re-write as a proc macro

[agryaznov]

Resolves #11344

[athei]

I like it. Just one thing that came to mind: I proposed using Result<ReturnCode, TrapReason> but that can be confusing because we actually call .into() on it in the macro. So I think we should be using: Result<ReturnCode, impl Into<TrapReason>> so that it is clear that any type that can be converted it is okay.

[agryaznov]

but that can be confusing because we actually call .into() on it in the macro

Hm, I don't see any places where such a conversion happens. E.g. this host function definition

	#[version(1)]
	fn seal_set_storage(
		ctx: Runtime<E: Ext>,
		key_ptr: u32,
		value_ptr: u32,
		value_len: u32,
	) -> Result<u32, TrapReason> {
		ctx.set_storage(KeyType::Fix, key_ptr, value_ptr, value_len)
	}

expands the following piece of code:

            f("seal1".as_bytes(), "seal_set_storage".as_bytes(), {
                fn seal_set_storage<E: Ext>(
                    ctx: &mut crate::wasm::Runtime<E>,
                    args: &[sp_sandbox::Value],
                ) -> Result<sp_sandbox::ReturnValue, sp_sandbox::HostError>
                where
                    <E::T as frame_system::Config>::AccountId: sp_core::crypto::UncheckedFrom<<E::T as frame_system::Config>::Hash>
                        + AsRef<[u8]>,
                {
                    #[allow(unused)]
                    let mut args = args.iter();
                    let mut body = || {
                        let key_ptr : < u32 as crate :: wasm :: env_def :: ConvertibleToWasm > :: NativeType = args . next () . and_then (| v | < u32 as crate :: wasm :: env_def :: ConvertibleToWasm > :: from_typed_value (v . clone ())) . expect ("precondition: all imports should be checked against the signatures of corresponding
                               functions defined by `#[define_env]` proc macro by the user of the macro;
                               thus this can never be `None`;
                               qed;") ;
                        let value_ptr : < u32 as crate :: wasm :: env_def :: ConvertibleToWasm > :: NativeType = args . next () . and_then (| v | < u32 as crate :: wasm :: env_def :: ConvertibleToWasm > :: from_typed_value (v . clone ())) . expect ("precondition: all imports should be checked against the signatures of corresponding
                               functions defined by `#[define_env]` proc macro by the user of the macro;
                               thus this can never be `None`;
                               qed;") ;
                        let value_len : < u32 as crate :: wasm :: env_def :: ConvertibleToWasm > :: NativeType = args . next () . and_then (| v | < u32 as crate :: wasm :: env_def :: ConvertibleToWasm > :: from_typed_value (v . clone ())) . expect ("precondition: all imports should be checked against the signatures of corresponding
                               functions defined by `#[define_env]` proc macro by the user of the macro;
                               thus this can never be `None`;
                               qed;") ;
                        {
                            ctx.set_storage(KeyType::Fix, key_ptr, value_ptr, value_len)
                        }
                    };
                    let r = body().map_err(|reason| {
                        ctx.set_trap_reason(reason);
                        sp_sandbox::HostError
                    })?;
                    return Ok(sp_sandbox::ReturnValue::Value({ r.to_typed_value() }));
                }
                seal_set_storage::<E>
            });

it just stores the TrapReason to the runtime as it is, and returns the sp_sandbox::HostError

[athei]

You are right. The conversion happens explicitly. Mostly through ?.

[ascjones]

The functional style would be let host_funcs = items.iter().map(|i| HostFn::try_from(i.clone())).collect::<Result<Vec<_>, _>()?. To be fair a lot of people do not like this (e.g. Sergei) 🙈

[ascjones]

Should be last() in case of fully qualified type e.g. ::core::result::Result

[ascjones]

Suggested change

	.first()
	.last()

[ascjones]

Could possibly simply the args extraction with slice pattern matching e,g, if let [arg1, arg2] = &group.args[..] { }

[agryaznov]

this would add up one more indent lvl 🙈

[ascjones]

You could do something like:

let (arg1, arg2) = if let [arg1, arg2] = &group.args[..]  {
  Ok((arg1, arrg2))
} else {
  Err("Expected exactly 2 args")
}?;

[agryaznov]

would be great, but &group.args is Punctuated and even .iter().collect() won't work on it (the trait FromIterator<GenericArgument> is not implemented for GenericArgument)

[ascjones]

LGTM

[athei]

Once you satisfied the CI (clippy) we can merge.

[athei]

bot merge