[PROPOSAL] Support for scoped storage variables

Namespaces are a very powerful (although undocumented) way to write Cairo modules. It allows scoping function definitions under an identifier, helping prevent collisions when importing functions from multiple modules.

Nevertheless, the current implementation does not allow to scope storage variables (or functions representing them) within namespaces, opening the door for storage collisions in the case of two modules defining the same storage variable, like this:

# module_a.cairo

@storage_var
func counter() -> (foo: felt):
end
# module_b.cairo

@storage_var
func counter() -> (foo: felt):
end

You can try this working example by @koloz based on @andrew’s previous research. Note that because of how storage variables work in StarkNet, these two modules will be writing and reading from the same storage slot potentially resulting in bugs or losses.

This is the case even if the user of the modules doesn’t manually import any of these storage variables, since any @external or @storage_var directive gets executed and effectively “imported” when importing any identifier from a module.

Proposal

  1. Support @storage_var definitions within the scope of a namespace

    • To make it easy for devs and tooling to reason about storage location, it can be internally computed as:
      var_name = f'{namespace}.{variable_name}'
      starknet_keccak(var_name.encode("ascii"))
      
  2. Stop “auto-importing” storage variables and external functions that are not explicitly imported. It is a bug. Smart contract development is a critical task and it’s already hard enough, let’s minimize surprises, let’s aim for a predictable execution and programming environment.

56 Likes

I had the opportunity to use namespaces in the StarkNet hackathon and I agree it improves the experience of writing composable functions.

I also support nesting storage variables inside namespaces to avoid name collisions.

20 Likes

I have run into some issues when using bound usage patterns and support both parts of the proposal.

The issue I ran into involved wanting to define a contract with “hook” functions that should be overridden by derivative contracts.

I had a contract A.cairo which defined some “hook” function extensible(). In this contract I had a function user() that calls extensible(). Afterwards, I created a contract B.cairo that imported the function user() but wrote a different implementation of function extensible(). Unfortunately, contrary to how it would work in a contract inheritance pattern, this led to the old version of extensible() being used in user() since usage relationships are bound on a function level.

In general, I believe in incremental proposals that move towards more familiar inheritance/multiple inheritance patterns and this is a great example.

23 Likes

I 100% agree with the proposal. It removes two major footguns from the language that unless removed are bound to result in vulnerabilities and hacks.

Though, if this proposal is not implemented then it’s very feasible for a static analysis tool to discover and expose potential issues created by storage variable name collisions and accidental imports of external calls. (especially namespace collisions could be a static check in the compiler)

On not auto-importing, does it make sense to spin off a separate discussion for this? There is a danger of go down a rabbithole about how to approach composition, and how to do it well :sweat_smile: .

Final note: with plain not-auto importing external calls there is a danger that you use (“extend”) an external library and forget to import all the external calls.

21 Likes

Doesn’t this couple the storage var address to where its code location (where it’s defined in the code)?

It’s not necessarily a bad thing, but I can imagine a case where some contract treats the storage var of another contract as something it might want to rely on (essentially making the storage var an API of the defining contract).

Another way to overcome name collision would be to include the name qualification explicitly in the storage var name.
i.e. a storage var s1 in namespace n1 could be called n1_s1, so another variable named s1 in a separate namespace won’t collide with it.
This isn’t perfect. It’s arguably less elegant (it’s like always referring to the storage var by its fully qualified name), and it relies on convention.
But it solves the name collision, and allows use case where the you’d want the storage var address to not rely on its location in the code.

16 Likes

I think that relying on autoimporting is not an acceptable extensibility pattern.

This is what we currently do but it’s one more thing to be mindful about, another point of failure. There’s too many security considerations and we need to minimize the mental burden for developers to focus on business logic.

Sure but that’s just another convention ¯\(ツ)/¯ . External parties can rely on ERC20.decimals instead of ERC20_decimals.

15 Likes

Oh, for sure. Just thinking about what could go wrong if you turn off auto importing straight away without a nice extensibility pattern to put in place.

I feel like implicit storage aliasing will be a really difficult thing to do right even if you do use it intentionally.

12 Likes

i can vouch for this. it’s really helpful to have namespaces for functions, and even better if we have one for storage variables

11 Likes

i’m in favor. the less we can rely on people to have a standard the less bugs that will potentially pop up overall and people won’t have to think about it when writing contracts (especially new devs who may not know about how variables are stored)

11 Likes

+1

thoughts on adding support for namespaced events as well?

11 Likes

Totally agree with this proposal!

12 Likes

If this idea is not adopted, a static analysis tool might easily find and reveal potential problems brought on by storage variable name conflicts and unintentional imports of external functions. Namespace clashes in particular might be subject to a static compiler check.

7 Likes