DelegateCall does not mutate caller's layoutful storage

[SkymanOne]

Similar background as in #1825.

This time, if the storage is layoutful (it contains at least one non-Lazy or non-Mapping filed with), the delegated callee does not mutate the caller's storage.

Detailed description

If the storage is layoutful, delegate call indeed transfers the context of the caller's storage to the callee. So all the mutation within callee's code appear to be valid. However, when the call is return to the caller, this mutations are not reflected in the caller's storage.

You can see two reproducers based on the ones from #1825

• https://github.com/paritytech/ink/blob/8e183d0c2d8c03a9fec47c1956dec052eb723e12/integration-tests/key-reproducer/lib.rs#L7
• https://github.com/paritytech/ink/blob/8e183d0c2d8c03a9fec47c1956dec052eb723e12/integration-tests/delegatecall-bug/lib.rs#L14

You can see from e2e tests that value: bool field does not get updated from delegate call. Furthermore, the Mapping field with a manually set key does not also get updated via delegate call.

The only way I got it to work is to wrap primitive types into Lazy and specify ManualKey for every single field, that way the delegate call behaves as expected.

This issue has not been raised before because the only integration test we have has only tested for reading the value via delegate call: https://github.com/paritytech/ink/blob/97f90ca6d032ecea1b0ff173ddde5ba0c18e1b30/integration-tests/lang-err-integration-tests/call-builder-delegate/lib.rs#L122

Potential Solution

With manual keys specified, the behaviour is correct. So I don't think we need to raise "code red". However, the way it works in solidity and our lack of documentation can introduce a footgun for new-coming devs.
Therefore, I propose two solutions:

• Enforce the use of manual keys when using delegate call at the type checking, and document this (fast)
• Investigate the causes of this behaviour and allow the mutation of layoutful storages via delegate_call() (slow)

[deuszx]

In your second test you're still calling flip_delegate https://github.com/paritytech/ink/blob/8e183d0c2d8c03a9fec47c1956dec052eb723e12/integration-tests/delegatecall-bug/lib.rs#L149 even though it seems you intended to call add_value_delegate.

[SkymanOne]

Yes, good spot. But it doesn't change the point of this issue.

[deuszx]

@SkymanOne , you raised the ticket as a bug but the PR addressing it only adds documentation and examples. Is it not a bug but a feature then?

[SkymanOne]

Is it not a bug but a feature then?

Yes, it has to do with the way ink! handles storage loading and flushing into and from memory.