Come up with an idea for generic mutable references

[Robbepop]

Generic Mutable References

Problem

Currently wrappers around objects stored on the contract storage do not provide a way to receive mutable references (&mut T) to those objects.

Rational

The reason for this is that users could do arbitrary changes to the objects without having an automated way to synchronize the storage.

Example

Storage wrappers like storage::Vec and storage::HashMap provide APIs like mutate_with with which it is possible for users to do an efficient in-place mutation of an object owned by the collection.

Goal

This is the tracking issue to come up with a user friendly, efficient and possibly general way to allow for mutable references that keep their referenced objects in sync between memory and storage.

Solutions

Solution 1: Do not provide mutable reference wrappers

This is the naive approach that we currently use.
However, it has the downside that it simply doesn't support what some users would like to have.

Solution 2: ?

[Robbepop]

Today I tried out a solution that makes use of two new traits

/// Types that are mutable by some closure.
pub trait Mutate<T> {
	fn mutate<F>(&mut self, f: F)
	where
		F: FnOnce(&mut T);
}

/// Types that are assignable to some new value.
pub trait Assign<T> {
	fn assign(&mut self, val: T);
}

With these we could define different ref-mut types for all the different data structures, such as storage::{Vec, HashMap, Stash, Value, ..} and wrap them with another unifying and general storage::RefMut<T> that implements many core traits depending on whether T implements them so we could theoretically write something like this:

let mut v = my_storage_vec(); // storage::Vec<i32>
v.get_mut(5).unwrap() += 42;

The downside to this is that due to the nature of core::ops::IndexMut we cannot write something like this:

let mut v = my_storage_vec(); // storage::Vec<i32>
v[5] += 42;

Note that this implementation of a mutable reference to storage items works without having a proper flushing system.
With a proper flushing system that flushes all synchronized state to the contract storage as soon as there is a context switch (i.e. due to calling a remote contract) we could simply use &mut T as our mutable references. Wrapping of them and forwarding state synchronization would no longer be necessary since we would flush their state to contract storage anyway as soon as needed.

Although the approach implemented today is already pretty nice in certain ways I think we really want a proper flushing system for the best usability we could achieve.

Flushing System

A mandatory problem with flushing systems that allow for raw mutable references is that we somehow need to restrict context switches to be only possible if there are no more mutable captures of storage variables. This can be done by coupling the context switch with the flushing system and also by coupling the contract state itself with the flushing system.

An example could help here: If we have the following contract state:

struct MyContractState {
    vec: storage::Vec<i32>, // some complex data in the storage
    map: storage::HashMap<u64, bool>, // some more complex data in the storage
    val: storage::Value<u8>, // some not so complex data in the storage
}

We would also need a mutable method for the context switch and flushing system, such as

impl MyContractState {
    fn flush_everything(&mut self) {
         self.vec.flush();
         self.map.flush();
         self.val.flush();
    }
}

However, this method should not be directly called by users since it would just destroy performance if used incorrectly. Instead it should be coupled with switching contexts, i.e. through calling a remote contract or calling an associated runtime module's functions.

impl MyContractState {
    fn call_remote(&mut self, remote_call_data: RemoteCallData) {
        self.flush_everything();
        ContractEnv::call_remotely(remote_call_data)
    }
}

To encapsulate all this logic we could define some new traits and types such as:

/// Types that can flush their synchronized state back into the contract.
trait Flush {
    fn flush();
}

/// A builder pattern using type to construct binary data for calling a remote contract or
/// runtime module function encoded in pDSL abi.
struct RemoteCallData { .. }

Also we would need to enhance pdsl_core::env::Env trait with a remote call trait method.
Note that the contract's MyContractState::call_remote would need to be automatically generated by something like a derive macro in order to remove this burden from the user.

Flush State

The problem with the above design is that nothing prevents users from using ContractEnv::call_remotely without flushing the state beforehand. So we should at least define ContractEnv::call_remotely as being unsafe to tell users not to use it directly, otherwise bad things may happen.

[eira-fransham]

Why not just store the state inside env and make it so that call_remote is a method on env requiring &mut? That way you'd get a notification at compile-time that you have overlapping mutable references.

[eira-fransham]

To come back to this, the real root problem is that the ownership graph is all over the place. call_remote (essentially) needs mutable access to the state in the general case, if it was inside the same process this would be cut and dry - you either pass in a mutable reference to call_remote or you make call_remote an &mut self method on a struct that also owns the state. The only reason we're considering other options is because we're treating this like we're making an HTTP request, which is not how this works conceptually (HTTP requests are asynchronous and so don't have this issue, you can lock the state without causing a deadlock as long as unlocking the state doesn't wait on the request returning). Since call_remote is synchronous we have to treat it as if it were an in-process function call, from an API design perspective.

[Robbepop]

This is very valuable feedback!
Having compile time checks would be greatly appreciated we should really try to target having them.
For this to work we need to change how Env in pdsl interoperates with storage allocation.
For example, at the moment Key is seen as raw pointer having no lifetime that could be bound to a certain environment which was a sane default since environments so far were (stateless) wrappers around the SRML provided interfaces. However, if we want to make this change (and I really want to experiment with this) we would need to change that.

[Robbepop]

Btw.: The flushing system has already been implemented and with the flushing system our mutable reference wrappers went extinct. I am currently trying to resolve some issues around storage allocation and initialization and will then continue with this.

[Robbepop]

Note that since the flushing system has been successfully implemented and with commit c3a942b even storage::Value now has a DerefMut implementation which allows for very abstract mutable accesses just like in normal Rust code.

So far it seems to work very well.

tldr; I think we can close this issue.