Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a StorageVec datastructure #1682

Open
xermicus opened this issue Feb 24, 2023 · 4 comments
Open

Provide a StorageVec datastructure #1682

xermicus opened this issue Feb 24, 2023 · 4 comments
Assignees
Labels
A-ink_storage [ink_storage] Work Item OpenZeppelin

Comments

@xermicus
Copy link
Contributor

Currently, we only provide a Mapping. However, storing things in a Vector (Array) on contract storage is also as thing our users need. Using the rust Vec from the prelude has a fundamental issue: It exhibits packed layout. This makes it a footgun when used on contract storage, easily leading to various sorts of DoS vulnerabilities.

There once was a dedicated storage vec data structure. This data structure would still use classical data structures from the prelude, but used its own StorageEntry struct to wrap it's data in and lazily read or write to/from storage. The approach would be to re-work it to function with the new storage API. On a high level, this should work. It will generate some amount of work implement though (I think it is more involved than just copying over the old code and make some minor changes).

Another thing is, for example, iterating over a Vec with 1000 elements on it, will still cause 1000 storage reads to the contracts pallet, and this will be costly. So I thought about whether we should try to be smart and do some further caching by designing a Lazy data structure that can read and write data in chunks. But it implies some drawbacks as well, e.g. additional complexity and it is not clear to me how we should determine an "optimal" chunk size.

@xermicus xermicus self-assigned this Feb 24, 2023
@cmichi cmichi added OpenZeppelin A-ink_storage [ink_storage] Work Item labels Feb 24, 2023
@xermicus xermicus moved this from Todo to In Progress in OpenZeppelin ↔ ink! Feb 25, 2023
@xermicus
Copy link
Contributor Author

xermicus commented Mar 22, 2023

From another discussion; it could also be useful to provide some kind of "reference" data structure (akin to Box<>), that just holds a reference (the storage key) to a Vec<> or any other data packed data structure on storage. This would specifically allow to store any packed data in a mapping, without risking to blow up the contract because a mapping value got too large to decode.

So, the approach here would be:

  • Implement the reference type
  • Implement the storage vector type describe above

@xgreenx WDYT?

@xgreenx
Copy link
Collaborator

xgreenx commented Mar 22, 2023

I didn't get how the "reference" should work. For me, it is similar to the Lazy type that we have=) Could you elaborate more, please?

This would specifically allow to store any packed data in a mapping, without risking to blow up the contract because a mapping value got too large to decode.

Hmm, but Mapping already pulls only the data from one storage cell for packed types. It seems we move the problem from Mapping to the "reference"

@xermicus
Copy link
Contributor Author

You are right, it's very similar. I think what we had in mind was to give the contract authors some way of trying to encode (or decode) the type, so that the error can be handled in case the Packed value gets too large. Do you think this can be built into Lazy?

@xgreenx
Copy link
Collaborator

xgreenx commented Jul 30, 2023

You are right, it's very similar. I think what we had in mind was to give the contract authors some way of trying to encode (or decode) the type, so that the error can be handled in case the Packed value gets too large. Do you think this can be built into Lazy?

We shouldn't allow storing values bigger than the buffer to decode them=)

I still didn't get how you want to use Lazy to address the decoding size problem. It will fail if the Lazy value exceeds a buffer. It will fail if the number of Lazy in the Vec<Lazy<SomeType>> is vast enough.

I think it is better to create a new type - StorageVec that uses Mapping inside to manage values and the len field. The type may work similarly to Solidity's storage array. But in our case, we can provide native iterator support because of transparent hashing. Plus some additional new features.

In this case, we have only one problem: the single elements may exceed the buffer, but it is unlikely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ink_storage [ink_storage] Work Item OpenZeppelin
Projects
None yet
Development

No branches or pull requests

3 participants