implemented push and pop

[dean-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[orizi]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

corelib/src/starknet/storage/vec.cairo line 419 at r1 (raw file):

        let last_element = self.at(vec_len - 1).read();
        self.as_ptr().write(vec_len - 1);
        Option::Some(last_element)

Suggestion:

        let len_ptr = self.as_ptr();
        let vec_len: u64 = len_ptr.read();
        if vec_len == 0 {
            return Option::None;
        }
        let entry = self.update(vec_len - 1);
        let last_element = entry.read();
        // Some code to fill the data with 0s.
        entry.scrab();
        let last_element = self.at().read();
        len_ptr.write(vec_len - 1);
        Option::Some(last_element)

---

crates/cairo-lang-starknet/cairo_level_tests/collections_test.cairo line 127 at r1 (raw file):

    assert_eq!(vec_contract_state.simple.pop(), Some(10));
    assert_eq!(vec_contract_state.simple.len(), 0);
    assert_eq!(vec_contract_state.simple.pop(), None);

Suggestion:

    let mut state = contract_with_vec::contract_state_for_testing();
    state.simple.push(10);
    state.simple.push(20);
    state.simple.push(30);
    assert_eq!(state.simple.len(), 3);
    assert_eq!(state.simple.at(0).read(), 10);
    assert_eq!(state.simple.at(1).read(), 20);
    assert_eq!(state.simple.at(2).read(), 30);
}

#[test]
fn test_simple_member_pop_from_vec() {
    let mut state = contract_with_vec::contract_state_for_testing();
    state.simple.append().write(10);
    state.simple.append().write(20);
    state.simple.append().write(30);
    assert_eq!(state.simple.pop(), Some(30));
    assert_eq!(state.simple.pop(), Some(20));
    assert_eq!(state.simple.pop(), Some(10));
    assert_eq!(state.simple.len(), 0);
    assert_eq!(state.simple.pop(), None);

---

corelib/src/prelude/v2024_07.cairo line 21 at r1 (raw file):

pub use crate::iter::{FromIterator, IntoIterator, Iterator};
pub use crate::{assert, bool, felt252, starknet, usize};
use crate::option::Option::{None, Some};

unrelated.

Code quote:

use crate::option::Option::{None, Some};

[dean-starkware]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

corelib/src/prelude/v2024_07.cairo line 21 at r1 (raw file):

Previously, orizi wrote…

unrelated.

It's due to a rebase with the new prelude PR.

---

corelib/src/starknet/storage/vec.cairo line 419 at r1 (raw file):

        let last_element = self.at(vec_len - 1).read();
        self.as_ptr().write(vec_len - 1);
        Option::Some(last_element)

Why is it better?
Why use scrab?

---

crates/cairo-lang-starknet/cairo_level_tests/collections_test.cairo line 127 at r1 (raw file):

    assert_eq!(vec_contract_state.simple.pop(), Some(10));
    assert_eq!(vec_contract_state.simple.len(), 0);
    assert_eq!(vec_contract_state.simple.pop(), None);

Done.

[orizi]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

corelib/src/prelude/v2024_07.cairo line 21 at r1 (raw file):

Previously, dean-starkware wrote…

It's due to a rebase with the new prelude PR.

so have your current PR target the preludes addition PR.
or better yet - for the time being - write the full path in this PR.
(there will be a following PR removing Option:: from around the code.)

---

corelib/src/starknet/storage/vec.cairo line 419 at r1 (raw file):

Previously, dean-starkware wrote…

Why is it better?
Why use scrab?

you do less hashes this way - .scrab() is basically the name of the function describing the comment right above it.
we have to clean the storage area of a variable that we pop.

[dean-starkware]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

corelib/src/prelude/v2024_07.cairo line 21 at r1 (raw file):

Previously, orizi wrote…

so have your current PR target the preludes addition PR.
or better yet - for the time being - write the full path in this PR.
(there will be a following PR removing Option:: from around the code.)

Done.

---

corelib/src/starknet/storage/vec.cairo line 419 at r1 (raw file):

Previously, orizi wrote…

you do less hashes this way - .scrab() is basically the name of the function describing the comment right above it.
we have to clean the storage area of a variable that we pop.

I understand the purpose of this action, but I'm getting this error:
Method scarbnot found on typecore::starknet::storage::StoragePath::<?8>. Did you import the correct trait and impl? --> corelib/src/starknet/storage/vec.cairo:421:15 entry.scarb(); ^^^^^
And I couldn't find any other instances if this .scrab() in our repo...

[orizi]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

corelib/src/starknet/storage/vec.cairo line 419 at r1 (raw file):

Previously, dean-starkware wrote…

I understand the purpose of this action, but I'm getting this error:
Method scarbnot found on typecore::starknet::storage::StoragePath::<?8>. Did you import the correct trait and impl? --> corelib/src/starknet/storage/vec.cairo:421:15 entry.scarb(); ^^^^^
And I couldn't find any other instances if this .scrab() in our repo...

It doesn't exist, but it is a prerequisite for this implementation.

[gilbens-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @dean-starkware and @orizi)

---

corelib/src/starknet/storage/vec.cairo line 334 at r3 (raw file):

    ///
    /// ```
    /// use core::starknet::storage::{Vec, MutableVecTrait, StoragePointerWriteAccess};

Is this really needed?

Code quote:

StoragePointerWriteAccess

---

corelib/src/starknet/storage/vec.cairo line 407 at r3 (raw file):

    ) {
        let storage_path = self.append();
        storage_path.write(value);

Suggestion:

        self.append().write(value);

[orizi]

Reviewed 1 of 2 files at r3.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

[orizi]

Reviewed all commit messages.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

[dean-starkware]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)

---

corelib/src/starknet/storage/vec.cairo line 334 at r3 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Is this really needed?

Why wouldn't it?

[orizi]

Reviewed 1 of 1 files at r2, 1 of 5 files at r4.
Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

corelib/src/starknet/storage_access.cairo line 207 at r4 (raw file):

    fn size() -> u8;

    /// Clears the storage area by writing zeroes to it.

match the rest of the documentation.

Code quote:

    /// Clears the storage area by writing zeroes to it.

---

crates/cairo-lang-starknet/cairo_level_tests/storage_access.cairo line 330 at r4 (raw file):

    assert_eq!(state.simple.len(), 0);
    assert_eq!(state.simple.pop(), None);
}

test it not part of vec.

Code quote:

fn test_scrub_clears_memory() {
    let mut state = contract_with_vec::contract_state_for_testing();
    state.simple.push(100);
    state.simple.push(200);
    state.simple.push(300);
    assert_eq!(state.simple.len(), 3);
    assert_eq!(state.simple.at(0).read(), 100);
    assert_eq!(state.simple.at(1).read(), 200);
    assert_eq!(state.simple.at(2).read(), 300);
    assert_eq!(state.simple.pop(), Some(300));
    assert_eq!(state.simple.append().read(), 0);
    assert_eq!(state.simple.pop(), Some(200));
    assert_eq!(state.simple.append().read(), 0);
    assert_eq!(state.simple.pop(), Some(100));
    assert_eq!(state.simple.append().read(), 0);
    assert_eq!(state.simple.len(), 0);
    assert_eq!(state.simple.pop(), None);
}

[gilbens-starkware]

Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @dean-starkware and @orizi)

---

corelib/src/starknet/storage/vec.cairo line 334 at r3 (raw file):

Previously, dean-starkware wrote…

Why wouldn't it?

Because you aren't 'write'ing anywhere in this example.

---

corelib/src/starknet/storage/vec.cairo line 356 at r4 (raw file):

/// Implement `MutableVecTrait` for any type that implements StorageAsPath into a storage
/// path that implements MutableVecTrait.

Revert.

Code quote:

/// Implement `MutableVecTrait` for any type that implements StorageAsPath into a storage
/// path that implements MutableVecTrait.

[dean-starkware]

Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

crates/cairo-lang-starknet/cairo_level_tests/storage_access.cairo line 330 at r4 (raw file):

Previously, orizi wrote…

test it not part of vec.

Should I make it a different PR?

[orizi]

Reviewed all commit messages.
Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

crates/cairo-lang-starknet/cairo_level_tests/storage_access.cairo line 330 at r4 (raw file):

Previously, dean-starkware wrote…

Should I make it a different PR?

probably a good idea.

[dean-starkware]

Reviewable status: 1 of 6 files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

corelib/src/starknet/storage/vec.cairo line 334 at r3 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Because you aren't 'write'ing anywhere in this example.

I am writing though...

---

corelib/src/starknet/storage/vec.cairo line 356 at r4 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Revert.

Done.

---

corelib/src/starknet/storage_access.cairo line 207 at r4 (raw file):

Previously, orizi wrote…

match the rest of the documentation.

Done.

---

crates/cairo-lang-starknet/cairo_level_tests/storage_access.cairo line 330 at r4 (raw file):

Previously, orizi wrote…

probably a good idea.

Done.

[orizi]

Reviewed 4 of 5 files at r6, all commit messages.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

corelib/src/starknet/storage/vec.cairo line 429 at r6 (raw file):

        Option::Some(last_element)
    }
}

Suggestion:

    fn pop<+Drop<Self::ElementType>, +starknet::Store<Self::ElementType>>(
        self: StoragePath<Mutable<Vec<T>>>,
    ) -> Option<Self::ElementType> {
        let len_ptr = self.as_ptr();
        let vec_len: u64 = len_ptr.read();
        if vec_len == 0 {
            return None;
        }
        let entry: StoragePath<Mutable<T>> = self.update(vec_len - 1);
        let last_element = entry.read();
        // Remove the element's data from the storage.
        let entry_ptr = entry.as_ptr();
        starknet::SyscallResultTrait::unwrap_syscall(
            starknet::Store::<
                Self::ElementType,
            >::scrub(0, entry_ptr.__storage_pointer_address__, 0),
        );
        len_ptr.write(vec_len - 1);
        Some(last_element)
    }
}

[dean-starkware]

Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

corelib/src/starknet/storage/vec.cairo line 429 at r6 (raw file):

        Option::Some(last_element)
    }
}

Done.

[orizi]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware)

---

corelib/src/starknet/storage/vec.cairo line 334 at r3 (raw file):

Previously, dean-starkware wrote…

I am writing though...

you aren't using the write method in the caller code.

[dean-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware)

---

corelib/src/starknet/storage/vec.cairo line 334 at r3 (raw file):

Previously, orizi wrote…

you aren't using the write method in the caller code.

Done.

[orizi]

Reviewed 5 of 5 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware)

[gilbens-starkware]

Reviewed 1 of 1 files at r2, 1 of 5 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dean-starkware)

[orizi]

Reviewable status: 5 of 19 files reviewed, 1 unresolved discussion (waiting on @dean-starkware and @gilbens-starkware)

---

-- commits line 2 at r9:
rebase went wild - fix please.

[dean-starkware]

Reviewable status: 5 of 19 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)

---

-- commits line 2 at r9:

Previously, orizi wrote…

rebase went wild - fix please.

fixed it.
Tell me if there's still any problem.

[orizi]

Reviewable status: 3 of 19 files reviewed, 1 unresolved discussion (waiting on @dean-starkware and @gilbens-starkware)

---

-- commits line 2 at r9:

Previously, dean-starkware wrote…

fixed it.
Tell me if there's still any problem.

the exact same problem seems to be there.

[dean-starkware]

Reviewable status: 3 of 19 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)

---

-- commits line 2 at r9:

Previously, orizi wrote…

the exact same problem seems to be there.

Done.

[orizi]

Reviewed 1 of 14 files at r9, 16 of 16 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dean-starkware)

---

crates/cairo-lang-starknet/cairo_level_tests/storage_access.cairo line 4 at r12 (raw file):

use core::integer::BoundedInt;
use core::num::traits::Zero;
use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess, Vec};

this change seems unrelated.

Code quote:

use starknet::storage::{StoragePointerReadAccess, StoragePointerWriteAccess, Vec};

[dean-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

---

crates/cairo-lang-starknet/cairo_level_tests/storage_access.cairo line 4 at r12 (raw file):

Previously, orizi wrote…

this change seems unrelated.

Done.

[orizi]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dean-starkware)