delegate_call API

[yarikbratashchuk]

Hello, ink! team. PR for delegate_call hopefully, will be merged soon, and now it's time to think about how to add this to the ink! API.

To support delegate_call we need to make changes to CallBuilder (and respective codegen part) and implement CallTypeBuilder (for building CallType structure)
I can't find an easy way to do this, but to break existing API. Here is an example how it is going to look like after the change:

 // for seal_call
 let my_return_value: i32 = build_call::<DefaultEnvironment>()
     .call_type(
         CallType::call()
             .callee(AccountId::from([0x42; 32]))
             .gas_limit(5000)
             .transferred_value(10)
     )
     .exec_input( ... )
     .returns::<ReturnType<i32>>()
     .fire()
     .unwrap();

// for seal_delegate_call
 let my_return_value: i32 = build_call::<DefaultEnvironment>()
     .call_type(
         CallType::delegate_call()
             .code_hash(CodeHash::from([0x42; 32]))
     )
     .exec_input( ... )
     .returns::<ReturnType<i32>>()
     .fire()
     .unwrap();

I am asking for your thoughts/suggestions before I make actual PR.

[yarikbratashchuk]

@athei, maybe you have any thoughts on this?

[athei]

You don't need this sub CallType builder. You can use the existing type state infrastructure to change the functions available on the main builder when the call type is changed to delegate.

[yarikbratashchuk]

@athei, so you want the CallType to hold just the type of the call, not the data related (such as gas_limit, value), right?

[athei]

Inside the CallBuilder struct this you still be the case. For the API I would keep it flat.

[yarikbratashchuk]

Ah, yeah. I got your point. Maybe that's a good idea.

[VargSupercolony]

@athei so, as the delegate call PR is merged, I think it's a good idea to get back to this issue.

If we want to keep the API flat, we'd probably have to go with creating a new builder (e.g., DelegateCallBuilder), designated to build delegate calls. This would allow for full compatibility with previous ink! versions, but it'll be a bit harder to codegen.

This has the following advantages and disadvantages:

(+) We keep the existing APIs as-is, which makes the change non-breaking
(+) We preserve the codegen for A.foo() call syntax
(-) We have to provide the get_delegate() and get_delegate_mut() functions for the contract to get the DelegateBuilder, while it makes the get() and get_mut() functions bear erroneous meaning

The other way is to do the CallType approach, where we would include the CallType param to the CallBuilder and break stuff.

(+) We allow for easier integration of further call types (if any)
(+) We do not introduce new structures, just add one param to the existing builder
(-) We break the existing codegen

IMO, breaking changes at this stage are still OK, as the versions being released are still RCs. So both approaches sound good to me, and for both, we'd have to design new codegen/utilities to turn calls to delegate_calls :) That said, I personally like the first approach more, as we don't break anything and the other CallTypes are unlikely to be added.

Code examples

If we take the first approach, our code will look like this:

/// Build simple call (non-delegated, codegen)

#[ink(message)]
pub fn foo(&mut self) -> Result<(), Error> {
  self.a.foo()
}

/// Build simple call (non-delegated, builder)

#[ink(message)]
pub fn foo(&mut self) -> Result<(), Error> {
  build_call::<DefaultEnvironment>()
    .callee(ToAccountId::to_account_id(self.a))
    .gas_limit(5000)
    .transferred_value(10)
    .exec_input(ExecutionInput::new(selector_bytes!("foo")))
    .returns::<Result<(), Error>>()
    .fire()
    .unwrap();
}

/// Build delegate call (codegen)

#[ink(message)]
pub fn foo(&mut self) -> Result<(), Error> {
  ink_env::call::delegate!(self.a.foo()) // this is a subject to change, I've no idea how to make that pretty now
}

/// Build delegate call (builder)

pub fn foo(&mut self) -> Result<(), Error> {
  build_delegate_call::<DefaultEnvironment>()
    .code_hash(self.a_code_hash)
//    .call_flags(...)
    .exec_input(ExecutionInput::new(selector_bytes!("foo")))
    .returns::<Result<(), Error>>()
    .fire()
    .unwrap();
}

For the second approach, builder examples are provided in the original issue. Codegen would look the same, I think.

I'd love to hear your thoughts on both of the approaches :)

[xgreenx]

I want to highlight another important point:

With the CallType approach we easy can convert between common and delegated calls. The code generation for CallBuilder still can generate the implementation for cross-contract calls(common call like before). But if the user wants to do a delegated call instead, he can easily change the type of the call.

ContractRef{ account_id: some_account }.call().foo(). // <- Currently it returns `CallBuilder` with selector for method `foo` and configured arguments for cross-contract call
     .call_type(
         CallType::delegate_call()
             .code_hash(CodeHash::from([0x42; 32]))
     ) // <- Here we changed the type to delegated call, but all previous configuration related to input and selector are correct
     .fire();

[athei]

What is wrong about my suggestion? Keeping a single CallBuilder but switch the available functions after calling .delegate_call() on it using type state. That would be the natural extension to the existing API.

This would also allow conversion between the two modes.

[xgreenx]

What is wrong about my suggestion? Keeping a single CallBuilder but switch the available functions after calling .delegate_call() on it using type state. That would be the natural extension to the existing API.

It means that delegate_call should implicitly unset other variables inside(create another CallBuilder with unseted types).
I'm okay with it but it can be not clear for the user.

This would also allow conversion between the two modes.

It is clear that delegate_call converts the call from common call to delegated. How we should name the function with reverse conversion? call() or common_call? If in the future we add one more call type, that will require us to add a function for conversion between each call type:

common_call -> delegated_call
common_call -> new_call
new_call -> common_call
new_call -> delegated_call
delegated_call -> common_call
delegated_call -> new_call

[athei]

It means that delegate_call should implicitly unset other variables inside(create another CallBuilder with unseted types).

This is fair. It might be confusing.

If in the future we add one more call type, that will require us to add a function for conversion between each call type:

The source type would be irrelevant. So it would be one function with the name of the mode you are switching to.

Your example would look like this:

ContractRef{ account_id: some_account }.call().foo(). // <- Currently it returns `CallBuilder` with selector for method `foo` and configured arguments for cross-contract call
     .delegate_call()
     .code_hash(CodeHash::from([0x42; 32]))
     .fire();

Please note that in both cases you will delete the supplied some_account which is not used at all.

I am not really sure which is the best way to do this but unflatten this seems harder to read.

[VargSupercolony]

Right now for delegate_call we only need the CodeHash generic param, but it means we should create new fire() and params() implementations of the CallBuilder, as well as add the CodeHash param to all the existing implementations. On the other hand, the CallType approach abstracts away the data, related purely to some CallTypes, and we:

1. Lessen the number of generic params in the CallBuilder to <E, CallType, Args, RetType>
2. Do not do param unsets/do not implicitly provide only one place to call .delegate_call() (if everything is Unset)
3. Incapsulating the data related to specific CallType. Simple call does not need to know about the CodeHash generic param (or potentially others), as well as delegate_call doesn't know about the callee, transferred value, and gas limit.

CallType allows for code that is more concise and more expressive :) And if anything goes wrong, it definitely makes it easier to read the sources and deal with any problem.

[athei]

Okay I get that we don't want to litter the CallBuilder with more type parameters than necessary. But the CallType has this generic parameter and needs to be stored within the CallBuilder. So we still need to add that parameter to the CallType?

[VargSupercolony]

I'm working on a PoC right now to show you, but the basic idea is following:

pub struct Call<Callee, GasLimit, TransferredValue> {
    callee: Callee,
    gas_limit: GasLimit,
    transferred_value: TransferredValue,
}

pub struct DelegateCall<E: Environment> {
    code_hash: E::Hash,
}

/// Builds up a cross contract call.
pub struct CallBuilder<E, CallType, Args, RetType>
where
    E: Environment,
    CallType: seal::CallTypeSealed,
{
    env: PhantomData<fn() -> E>,
    /// The current parameters that have been built up so far.
    call_type: CallType,
    call_flags: CallFlags,
    exec_input: Args,
    return_type: RetType,
}

#[allow(clippy::type_complexity)]
pub fn build_call<E, CallType>() -> CallBuilder<
    E,
    CallType,
    Unset<ExecutionInput<EmptyArgumentList>>,
    Unset<ReturnType<()>>,
>
where
    E: Environment,
    CallType: CallTypeSealed,
{
    CallBuilder {
        env: Default::default(),
        call_type: Default::default(),
        call_flags: Default::default(),
        exec_input: Default::default(),
        return_type: Default::default(),
    }
}

We expose the CallType to the user, so we'll call build_call like build_call::<DefaultEnvironment, DelegateCall<DefaultEnvironment>>() or build_call::<DefaultEnvironment, Call<AccountId, u64, Balance>>()ю

At the same time, CallType is sealed, so user will not create their own STATICCALLs and stuff :)

[athei]

How will that work with codegen? I think all your examples started from the wrong premise that it is created from a ContractRef. What you want is to be able to have delegate_call() on a CodeHash and not on a ContractRef.

[VargSupercolony]

Ahhhh ok I see what you mean. Then it's simple too: we will provide build_call for ContractRefs and build_delegate_call for CodeHash :) In the first case, it gets parametrized by Call, in the second - DelegateCall.

[xgreenx]

How will that work with codegen? I think all your examples started from the wrong premise that it is created from a ContractRef. What you want is to be able to have delegate_call() on a CodeHash and not on a ContractRef.

By default, the codegen will generate the implementation for ContractRef like before - it is a common cross-contract call. But if the user wants, he can easily change the type of the CallType like here.

Also in the future, we also can generate ContractDelegatedRef that contains CodeHash inside.

[athei]

How will that work with codegen? I think all your examples started from the wrong premise that it is created from a ContractRef. What you want is to be able to have delegate_call() on a CodeHash and not on a ContractRef.

By default, the codegen will generate the implementation for ContractRef like before - it is a common cross-contract call. But if the user wants, he can easily change the type of the CallType like here.

Also in the future, we also can generate ContractDelegatedRef that contains CodeHash inside.

Imho we should do what @VargSupercolony says. Creating a delegate call from a ContractRef makes no sense. You should create it from a CodeHash.

[xgreenx]

Imho we should do what @VargSupercolony says. Creating a delegate call from a ContractRef makes no sense. You should create it from a CodeHash.

I meant that the approach with CallType allows easy convert one call into another. I agree with "Creating a delegate call from a ContractRef makes no sense"=)

ContractRef is auto-generated and it contains AccountId inside. I meant that we also can generate ContractDelegatedRef that contains CodeHash inside. ContractDelegatedRef will contain auto-generated code that will do delegated calls.

I meant the same idea as Varg=)

You should create it from a CodeHash.

It is ContractDelegatedRef.

let erc20: Erc20DelegatedRef = CodeHash::from_hash(some_hash);
erc20.transfer(to, amount); // <- It is delegated call instead of cross contract call here

[athei]

I meant that the approach with CallType allows easy convert one call into another.

We don't want to convert. Those are different things.

[xgreenx]

We don't want to convert. Those are different things.

Maybe we don't want to convert, but we can provide a flexible API. If the developer is simpler to convert instead of using ContractRef and ContractDelegatedRef, he can easily do that=)

So, what do you think about the idea of the usage of one generic type CallType(described here) is better than extending the number of generics in CallBuilder? If you agree, we don't need to discuss conversion because it will be available by default.

[athei]

So, what do you think about the idea of the usage of one generic type CallType(described #1109 (comment)) is better than extending the number of generics in CallBuilder?

I think this approach is fine.

conversion because it will be available by default.

I think we should allow a call to call_type exactly once for a builder. Once you set it you can't switch to another call_type.

[VargSupercolony]

@athei I will make a PR really soon; what should I check before submitting the PR? Does the check-workspace.sh script have to pass, or just tests?

[athei]

I have never called this script. CI doesn't do either as far as I can tell. So I would say "NO".

[VargSupercolony]

#1133