Add Deque #807

chipshort · 2022-09-26T12:48:54Z

closes #776

Implements a deque, similar to the std lib VecDeque.
This allows having both queue and stack semantics with a single data structure.
I used wrapping math, such that, in case the indexes ever reach u32::MAX or you always push to the front, it will still work (as long as it's not filled completely).
No random access at the moment, but can add that, if desired.

The implementation is inspired by #786

uint

In general, I really appreciate this PR. It's thorough, some important edge cases are covered, convention is followed, there's plenty of documentation, tests, and there's some clever stuff in there. Nice.

packages/storage-plus/README.md

uint · 2022-09-26T16:09:55Z

packages/storage-plus/README.md

+        age: 123,
+    };
+
+    // use it like a queue by pushing and popping at opposite ends


Nice examples!

packages/storage-plus/src/deque.rs

uint · 2022-09-26T16:28:22Z

packages/storage-plus/src/deque.rs

+
+    /// Helper method for `tail` and `head` methods to handle reading the value from storage
+    fn read_meta_key(&self, storage: &dyn Storage, key: &[u8]) -> StdResult<u32> {
+        let full_key = namespaces_with_key(&[self.namespace], key);


Using an existing helper that handles length encoding 👍

packages/storage-plus/src/deque.rs

uint · 2022-09-26T16:48:05Z

packages/storage-plus/src/deque.rs

+
+    /// Tries to get the value at the given position
+    /// Used internally
+    fn get_at_unchecked(&self, storage: &dyn Storage, pos: u32) -> StdResult<Option<T>> {


Should we just name it get_unchecked? It feels like array accessors in std are usually named get, not get_at.

Since it costs us nothing, would you want to provide a public and checked get variant of this, and then call the type VecDeque, like you suggested before? I think it makes a lot of sense.

uint · 2022-09-26T16:55:32Z

packages/storage-plus/src/deque.rs

+    /// Adds the given value to the front of the deque
+    pub fn push_front(&self, storage: &mut dyn Storage, value: &T) -> StdResult<()> {
+        // need to subtract first, because head potentially points to existing element
+        let pos = self.head(storage)?.wrapping_sub(1);


Clever. But I guess this effectively means there's a length limit to these deques, and once someone goes beyond that limit, there's undefined behavior. I feel this should be documented and/or we should guard the user from that. What do you think?

Yes, the limit is u32::MAX. We could guard against this when pushing by checking whether head and tail are equal and throwing an error. We will have to do an additional read from storage (of head or tail) for that though.
Which one do you prefer? Documenting or checking?

If you store the head and tail indexes under one storage key, this would become just one read.

Either way works for me. I guess if we document it and someone requests guards or bigger arrays, we can always add that later.

It looks like std::collections::VecDeque will probably panic when grown beyond usize. I think we're fine just documenting.

packages/storage-plus/src/deque.rs

uint

Nice! Feel free to merge once it's up to date with main and tests pass.

webmaster128 · 2022-10-03T05:30:40Z

Very nice. Looks like a JavaScript Array now.

Can you explain the reasoning for renaming Deque to VecDeque? It looks like a deque but it is not based in a vector (in contrast to std::collections::VecDeque).

uint · 2022-10-03T06:56:25Z

Can you explain the reasoning for renaming Deque to VecDeque? It looks like a deque but it is not based in a vector (in contrast to std::collections::VecDeque).

I suggested it. Fair point. I guess I got swayed by the fact it implements indices and random access. It isn't resizable though. Would you prefer something like ArrDeque, or just Deque?

webmaster128 · 2022-10-03T14:49:37Z

Would you prefer something like ArrDeque, or just Deque?

I vote for just Deque – nice and simple.

I think here is no need to add implementation details in the name itself. Rust tends to do that because it matters at the system language level and it allows them to communicate their version is not a linked list, which has certain overhead.

webmaster128 · 2022-10-03T14:56:51Z

packages/storage-plus/src/deque.rs

+#[cfg(feature = "iterator")]
+impl<'a, T> Iterator for VecDequeIter<'a, T>


The range/iterator implementation is genius.

Do I understand correctly that this even works without the native iterator implementation? I think you can remove #[cfg(feature = "iterator")] since you don't rely on storage ranges.

Yes, it only needs storage.get. Actually, I was wondering about that iterator feature. Are there CosmWasm chains that don't support storage.range?

IIRC yes, there are chains that don't support it due to how their storage works. I'm sure Simon or Ethan could elaborate.

There are underlying storage engines that don't have iterators. The default storage has it but e.g. Secret Network has an encrypted KV storage which you cannot iterate. Also Patricia Merkle Tries (Ethereum) do not support iterating.

chipshort · 2022-10-04T07:45:54Z

So, should I make a follow-up PR renaming to Deque and with the iterator feature checks removed?

uint · 2022-10-04T08:06:18Z

So, should I make a follow-up PR renaming to Deque and with the iterator feature checks removed?

That would be great!

chipshort requested review from uint and webmaster128 September 26, 2022 12:48

uint reviewed Sep 26, 2022

View reviewed changes

uint mentioned this pull request Sep 26, 2022

Added Stack storage. #786

Closed

uint self-requested a review September 27, 2022 10:50

uint approved these changes Sep 27, 2022

View reviewed changes

chipshort added 17 commits September 27, 2022 12:53

Implement basic queue

fafa185

Add double ended iterator for queue

b434138

Add more queue tests

8bc0e95

Add deque functionality to queue

f339278

Add missing feature checks

4c325b5

Add front and back methods to deque

9982a46

Add deque to readme

c2062cb

Fix tests

f36e920

Fix lints

420cce8

Throw error instead of silently ending iterator

19bb5cd

Rename methods

7b8342d

Fix docs

82e843b

Add test for iterator error

5592368

Add documentation about max deque size

4616d33

Add random access to deque

b4f3fd3

Rename Deque to VecDeque

48561bf

Update docs

db6cab2

chipshort force-pushed the 776-queue branch from f83f2b8 to db6cab2 Compare September 27, 2022 10:53

chipshort merged commit 4da8c32 into main Sep 27, 2022

chipshort deleted the 776-queue branch September 27, 2022 11:01

webmaster128 reviewed Oct 3, 2022

View reviewed changes

chipshort mentioned this pull request Oct 4, 2022

Deque changes #822

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Deque #807

Add Deque #807

chipshort commented Sep 26, 2022 •

edited

uint left a comment

uint Sep 26, 2022

uint Sep 26, 2022

uint Sep 26, 2022

uint Sep 26, 2022

chipshort Sep 27, 2022

uint Sep 27, 2022

uint Sep 27, 2022

uint left a comment

webmaster128 commented Oct 3, 2022

uint commented Oct 3, 2022

webmaster128 commented Oct 3, 2022

webmaster128 Oct 3, 2022

chipshort Oct 4, 2022

uint Oct 4, 2022

webmaster128 Oct 4, 2022

chipshort commented Oct 4, 2022

uint commented Oct 4, 2022

		#[cfg(feature = "iterator")]
		impl<'a, T> Iterator for VecDequeIter<'a, T>

Add Deque #807

Add Deque #807

Conversation

chipshort commented Sep 26, 2022 • edited

uint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uint left a comment

Choose a reason for hiding this comment

webmaster128 commented Oct 3, 2022

uint commented Oct 3, 2022

webmaster128 commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipshort commented Oct 4, 2022

uint commented Oct 4, 2022

chipshort commented Sep 26, 2022 •

edited