StorageVec

[SwayStar123]

Making a vector for use in storage, with many utility functions

[SwayStar123]

closes #2035

[mohammadfawaz]

Nice work! 🚀

Even though I like seeing all the utility functions you have here, I recommend starting with only the core ones:

• pub fn push(mut self, value: T)
• pub fn get(self, index: u64) -> Option<T>
• pub fn clear(mut self)
• pub fn is_empty(self) -> bool
• pub fn len(self) -> u64

It would be weird to have all the extra utility functions for StorageVec before having them for Vec. Also, writing a test for all these utilities at once for all possible data types is going to take forever. The simple list above will be easier to test, review, and document well. There are some non-obvious things going (like why is len stored in __get_storage_key(), etc.).

If you want, keep this PR open with only the core functions, and create a separate PR based on this one that adds the other stuff. We will prioritize reviewing and getting this one in first with its tests, and then focus on the other stuff in the other PR.

[SwayStar123]

Nice work! 🚀

Even though I like seeing all the utility functions you have here, I recommend starting with only the core ones:

• pub fn push(mut self, value: T)
• pub fn get(self, index: u64) -> Option<T>
• pub fn clear(mut self)
• pub fn is_empty(self) -> bool
• pub fn len(self) -> u64

It would be weird to have all the extra utility functions for StorageVec before having them for Vec. Also, writing a test for all these utilities at once for all possible data types is going to take forever. The simple list above will be easier to test, review, and document well. There are some non-obvious things going (like why is len stored in __get_storage_key(), etc.).

If you want, keep this PR open with only the core functions, and create a separate PR based on this one that adds the other stuff. We will prioritize reviewing and getting this one in first with its tests, and then focus on the other stuff in the other PR.

Added clear(), len(), is_empty()

My reasoning for adding those specific functions was that those functions are whats actually blocking my app dev, which is why i started working on a storagevec. These two (remove, insert) functions seem pretty basic and very useful so im thinking of keeping them and potentially even adding swap_remove for gas efficiency. After that ill work on testing

[adlerjohn]

and potentially even adding swap_remove for gas efficiency

To echo @mohammadfawaz's suggestion: smaller atomic PRs are easier to review and faster to merge. It's an art to hold yourself back and break down your work into pieces, but learning to do this well will pay dividends.

[Braqzen]

• can_insert() - Given that the entire vector is being changed which assertion do you think is needed before & after the insertion?
  • The same applies to can_remove()
• can_remove() tests the remove() method which uses a loop. The current test checks a single value unlike insert() which also has a loop but its test checks the other values. What do you think can go wrong in a method that uses a loop to shift its values but only a single value is checked?
• There are missing tests for every other built-in type

[Braqzen]

• can_remove(), can_swap_remove() & can_insert() have len checks at the beginning of the test. Those should be removed because there is a len check just before each assert_eq!() at the start. We do not need to check each and every operation from the other implemented methods. We only care about the snapshot of the state just before we perform our test. It does not matter how we got there, all that matters is that we know what the state is so when we perform a call to the method we are testing we know that is the only thing that could be changing the state. Could you remove the len check just before the pushing to the vec occurs?
• Tests look reasonable aside from the duplication. This could probably be transformed to be a single contract that uses generics and likewise the Rust side too. This would mean that each test may have slightly different constants etc. defined somewhere but it would significantly cut down on the code. Is there something blocking a generic implementation?

Don't see an immediate reason to continue to block when code duplication is the only nuisance

[mohammadfawaz]

This is really impressive work. Well done!

[mohammadfawaz]

What happens if I call insert() on an empty vector (i.e. len = 0) and set index = 0? Wouldn't len - 1 cause a problem?

[Braqzen]

Good catch

[SwayStar123]

then it would underflow and panic right?

[mohammadfawaz]

But this should work, no? Shouldn't Inserting at index len (even if len is zero) exhibit the same behavior as push?

[SwayStar123]

Nope, even rust panics here

[mohammadfawaz]

Hmm this is not panicking for me: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c05a852a5b60fb1f13fa37bfd783b1f3

Even then, I think relying on an underflow to get a panic is an antipattern. The better practice is to do all your checking in separate code for safety and readability even if that means larger bytecode. The optimizer can then do its magic!

[mohammadfawaz]

As this PR is basically ready aside from this, I won't block the merge but I suggest you create "P: critical" issue tracking the above and hopefully we'll get to it asap.

[SwayStar123]

Ah sorry my bad I looked at the wrong thing, yeah i guess this works in rust.

After sway starts allowing methods to call other methods ill just add a if len == 0 before the while loop there to early return with a push

[SwayStar123]

• can_remove(), can_swap_remove() & can_insert() have len checks at the beginning of the test. Those should be removed because there is a len check just before each assert_eq!() at the start. We do not need to check each and every operation from the other implemented methods. We only care about the snapshot of the state just before we perform our test. It does not matter how we got there, all that matters is that we know what the state is so when we perform a call to the method we are testing we know that is the only thing that could be changing the state. Could you remove the len check just before the pushing to the vec occurs?
• Tests look reasonable aside from the duplication. This could probably be transformed to be a single contract that uses generics and likewise the Rust side too. This would mean that each test may have slightly different constants etc. defined somewhere but it would significantly cut down on the code. Is there something blocking a generic implementation?

ABIs will never support generics, so this will never be possible

[adlerjohn]

Due to the general lacking of being able to call functions properly when they're in the wrong order, this is fine for now. Can you file an issue to abstract away key calculation into a function?