Remove `StorageContext`

[shargon]

Summary or problem description
It doesn't make sense to use the storage context class, as well as ReadOnly or Write. Do we plan to be able to forward that stack item to another contract and have it write to our storage using the context? I don't think so. In terms of security, it opens a door that I believe shouldn't exist. It also always requires obtaining storage from the current context, which is always the current contract, and it should always be ReadOnly for Find and Get, and normal for Put.

Either I'm missing the point, or I don't see the need to maintain it. Calling the context and forwarding the stack items consumes extra gas, and it opens the door to potential security issues.

Do you have any solution you want to propose?
Remove StorageContext

Where in the software does this update applies to?

• Compiler
• VM

[roman-khimov]

One feature that it currently provides is that it allows to pass RO context to other contracts which is safe and can theoretically be used for some purposes. But I've never seen this done and this certainly breaks contract isolation requiring third-party contract to know the storage scheme of other contract.

And yes, it is very dangerous because RW context can be passed around as well, in some cases unintentionally.

So I agree we can provide a safer alternative that is always bound to calling contract. I doubt we can remove StorageContext at the same time, all contracts currently depend on it. Forcing the respective syscalls to ignore StorageContext can break some contracts and doesn't help with GAS spent on context retrieval/passing around. My choice would be to add a set of new interops and deprecate (but not remove) old ones.

[shargon]

One feature that it currently provides is that it allows to pass RO context to other contracts which is safe and can theoretically be used for some purposes. But I've never seen this done and this certainly breaks contract isolation requiring third-party contract to know the storage scheme of other contract.

And yes, it is very dangerous because RW context can be passed around as well, in some cases unintentionally.

So I agree we can provide a safer alternative that is always bound to calling contract. I doubt we can remove StorageContext at the same time, all contracts currently depend on it. Forcing the respective syscalls to ignore StorageContext can break some contracts and doesn't help with GAS spent on context retrieval/passing around. My choice would be to add a set of new interops and deprecate (but not remove) old ones.

Agree, it could be easier

[shargon]

@roman-khimov we also can distinguish by num of arguments to choose the syscall

[roman-khimov]

I think we can't. The difference between native contract calls and syscalls is exactly that, syscalls have no fixed signature, just some name. It's somewhat hidden in C# codebase, but

neo/src/Neo/SmartContract/ApplicationEngine.cs

Line 268 in 9b9be47

app.OnSysCall(GetInteropDescriptor(instruction.TokenU32));

can only choose a single descriptor based on name, it knows nothing about parameters. And it's a feature in some ways, think of how

neo/src/Neo/SmartContract/ApplicationEngine.Crypto.cs

Line 65 in 9b9be47

protected internal bool CheckMultisig(byte[][] pubkeys, byte[][] signatures)

gets its parameters dynamically via

neo/src/Neo/SmartContract/ApplicationEngine.cs

Lines 588 to 594 in 9b9be47

	{
	int count = (int)item.GetInteger();
	if (count > Limits.MaxStackSize) throw new InvalidOperationException();
	av = Array.CreateInstance(descriptor.Type.GetElementType(), count);
	for (int i = 0; i < av.Length; i++)
	av.SetValue(descriptor.Converter(Pop()), i);
	}

It just picks whatever number of items from the stack it finds appropriate.