Substrate: srml-contracts: ext_terminate

[pepyakin]

Due to upcoming changes with the FRAME balances pallet we won't be able to rely on the callback called upon decreasing a contract's balance below existential deposit / minimum balance.

Instead of that, we should not allow for a contract's balance to go below the minimum balance. That would prevent the contract to destroy itself, so to give a substitute we introduce a dedicated function tentatively called ext_terminate with the following signature:

fn ext_terminate(beneficiary: AccountId) → !;

This function would take an ID of an account that would receive all the remaining funds possessed by the terminated contract. Because self-termination and sending all funds implies removing the associated account, this function doesn't return control and finishes the contract execution. The function traps with reversal (i.e. not performing the termination) if the contract is already present on the execution stack.

Open questions:

• [ ] It is a bit weird that this function doesn't allow to return a value to the caller since it is similar to ext_return.
• [ ] In case that a contract is already present on execution stack should we return a status code? This will give an ability to the caller contract to handle this condition. However, I don't see a way to handle this meaningfully other than trap.

[Robbepop]

In case that a contract is already present on execution stack should we return a status code? This will give an ability to the caller contract to handle this condition. However, I don't see a way to handle this meaningfully other than trap.

I like to go with an API that allows for handling error cases because you never know if someone actually has a proper use case for it. OTOH, how breaking would it be to introduce such functionality at a later point?

According to the ! return type is it correct that ext_terminate terminates the contract immediately? If not, we might want to instead return nothing and allow for doing trivial tasks (non contract mutating operations) afterwards, e.g. set a return value. For ink! having a ! return value is a bit harder to manage gracefully with a decent API.

[pepyakin]

I like to go with an API that allows for handling error cases because you never know if someone actually has a proper use case for it. OTOH, how breaking would it be to introduce such functionality at a later point?

I think it should be straightforward to extend this later to return a error code instead of trapping, but not the opposite.

According to the ! return type is it correct that ext_terminate terminates the contract immediately?

Yeah, in my description I meant that the termination happens immediately and that's why it doesn't return control. This is, IMO, the simplest implementation from the PoV of the contracts runtime.

I think it should be possible to implement it so that it returns the control back to the contract, but I'd like to not go this way. The reason for that is that it introduces a new modality into the contract runtime where a contract is only allowed to do a limited set of actions.

We can try to think about deferring the termination to the end of contract invocation therefore allowing some actions, but I feel that this would add more complexity while not bringing any benefits. E.g. it doesn't make a lot of sense to alter the storage, calling other contracts is possible, but seems cheesy, calling the runtime either synchronously or asynchronously seems dangerous. Returning a value seems OK, but I think introducing a modality for that would be an overkill - a parameter should do. But this should be also possible to extend in future.

There is another approach, that is aligned with our direction to leverage status codes. However, now, we are coming to a realization that it won't work good for languages like Solidity so probably we will be moving away from it.

[athei]

I think it should be straightforward to extend this later to return a error code instead of trapping, but not the opposite.

You cannot go from diverging to converging without breaking the callers control flow. Example:

if fatal_condition {
    ext_terminate();
} else {
    println!("Everything is A OK");
}
// I am confident that ext_terminate is diverging
do_stuff_that_is_unsound_on_fatal_condition();

That said, I am also in favor of trapping here. It just smells like exploit allowing a contact to execute after termination.

[pepyakin]

@athei I meant that we could introduce a separate function that would return an error code instead of trapping. But now thinking about this more, I think it would be fine either way.

[Robbepop]

I'd love to propose a reason to beneficiary that is simply data that could be a reason to terminate a contract, e.g. changing the signature to:

fn ext_terminate(beneficiary_ptr: u32, beneficiary_len: u32, reason_ptr: u32, reason_len: u32) -> !;

This would also properly trigger an event to catch by 3rd party tools to see when a contract has terminated itself with the associated reason. Contract metadata could then provide the type or encoding/decoding of the reason data.

[pepyakin]

As per offline discussion, I don't think that there is a good reason for the reason buffer: what would the caller do with that anyway? At the same time it would introduce new failure modes to the ext_call and ext_instantite.
Luckily, that is not really necessary because this can be addressed at the userland level: if a contract had a reason (if there are multiple) that it wanted to communicate to 3rd party tools then it could deposit an event.

[athei]

So should ext_terminate even deposit an termination event on its own?

[Robbepop]

So should ext_terminate even deposit an termination event on its own?

I still think it would have been useful to do so, however, probably without reason. The reason data is superflous if it only exists for the event. My original thinking was that it could theoretically also be used as "indicator" for another contract A calling B in the case where B then calls ext_terminate to singal A. But on the other hand having the reason is mixing semantics into one API.

[pepyakin]

Reopening the issue due to the fact that this piece of code hasn't been removed:

https://github.com/paritytech/substrate/blob/0108665c3353610d112926e2c2b32e3277629f36/frame/contracts/src/lib.rs#L944-L948