Come up with an idea for generic mutable references about ink HOT 6 CLOSED

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.

from ink.

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.

from ink.

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.

from ink.

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.

from ink.

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.

from ink.

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.

from ink.