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

Add basic storage array. #5974

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Add basic storage array. #5974

merged 1 commit into from
Jul 9, 2024

Conversation

gilbens-starkware
Copy link
Contributor

@gilbens-starkware gilbens-starkware commented Jul 7, 2024

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.


This change is Reviewable

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion

a discussion (no related file):
Draft - names and doc tbd.


Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

pub trait StorageArrayTrait<T> {

doc all.
(also below)


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

pub trait StorageArrayTrait<T> {

generate_trait?
(also below)


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

        self.as_path().append()
    }
}

emptyline.

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @ArielElp and @orizi)


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

Previously, orizi wrote…

doc all.
(also below)

Done.


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

Previously, orizi wrote…

generate_trait?
(also below)

Doesn't work, I guess because the generic arg and param are not exactly the same, I'll check it out later.


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

Previously, orizi wrote…

emptyline.

Done.

@gilbens-starkware gilbens-starkware force-pushed the spr/main/01d5fd64 branch 2 times, most recently from 0f7603e to baa2564 Compare July 7, 2024 14:29
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

a discussion (no related file):

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

Draft - names and doc tbd.

+1



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

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

Doesn't work, I guess because the generic arg and param are not exactly the same, I'll check it out later.

you are correct.


corelib/src/starknet/storage.cairo line 649 at r2 (raw file):

}

/// A type to represent an array in storage. The length of the storage is stored in the storage

maybe should be added to its own submodule file?
(and possibly for Map as well)

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @ArielElp and @orizi)


corelib/src/starknet/storage.cairo line 649 at r2 (raw file):

Previously, orizi wrote…

maybe should be added to its own submodule file?
(and possibly for Map as well)

Done. Will do Map in a separate pr.

@gilbens-starkware gilbens-starkware changed the base branch from spr/main/01d5fd64 to dev-v2.7.0 July 8, 2024 08:32
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


-- commits line 4 at r4:
rebase

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @ArielElp and @orizi)


-- commits line 4 at r4:

Previously, orizi wrote…

rebase

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


corelib/src/starknet/storage.cairo line 9 at r5 (raw file):

mod array;
pub use array::{StorageArray, StorageArrayTrait, MutableStorageArrayTrait};

are we changing to vector?
(as the interface is different enough from our array in any case)

Code quote:

mod array;
pub use array::{StorageArray, StorageArrayTrait, MutableStorageArrayTrait};

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

#[starknet::storage_node]
struct StorageArrays {
    array: StorageArray<u32>,

Array<T> is equivalent toMap<u64, T> value wise?
if so, add direct test accessing members (using contract_state_for_testing, and direct assignment and read in one and the other)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, orizi wrote…

Array<T> is equivalent toMap<u64, T> value wise?
if so, add direct test accessing members (using contract_state_for_testing, and direct assignment and read in one and the other)

(i know array has length as well, but ignore it for testing value location wise)

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


corelib/src/starknet/storage.cairo line 9 at r5 (raw file):

Previously, orizi wrote…

are we changing to vector?
(as the interface is different enough from our array in any case)

I think that we do. Vector or Vec? And what about append?


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

Previously, orizi wrote…

(i know array has length as well, but ignore it for testing value location wise)

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @ArielElp and @orizi)


corelib/src/starknet/storage.cairo line 9 at r5 (raw file):

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

I think that we do. Vector or Vec? And what about append?

Done

@gilbens-starkware gilbens-starkware marked this pull request as ready for review July 9, 2024 08:55
Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @ArielElp and @orizi)

a discussion (no related file):

Previously, orizi wrote…

+1

Done.


Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r6, 3 of 5 files at r7.
Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @ArielElp and @gilbens-starkware)


corelib/src/starknet/storage.cairo line 9 at r5 (raw file):

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

Done

if we are going for Vec no need for the disabguiation with Storage prefix. - just Vec like Map.

for full name - user can just use starknet::storage::Vec.


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

        nested: StorageVec::<StorageVec<u32>>,
    }
}

Suggestion:

mod contract_with_map {
    use starknet::storage::Map;
    #[storage]
    struct Storage {
        simple: Map<u64, u32>,
        nested: Map<u64, Map<u64, u32>>,
    }
}

#[starknet::contract]
mod contract_with_vec {
    use starknet::storage::StorageVec;
    #[storage]
    struct Storage {
        simple: StorageVec<u32>,
        nested: StorageVec<StorageVec<u32>>,
    }
}

crates/cairo-lang-starknet/cairo_level_tests/lib.cairo line 29 at r7 (raw file):

mod utils;
#[cfg(test)]
mod collections_test;

empty line

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @ArielElp and @orizi)


corelib/src/starknet/storage.cairo line 9 at r5 (raw file):

Previously, orizi wrote…

if we are going for Vec no need for the disabguiation with Storage prefix. - just Vec like Map.

for full name - user can just use starknet::storage::Vec.

Done.
Changed also the traits, e.g. starknet::storage::VecTrait but not sure if it's better.


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

        nested: StorageVec::<StorageVec<u32>>,
    }
}

Done.


crates/cairo-lang-starknet/cairo_level_tests/lib.cairo line 29 at r7 (raw file):

Previously, orizi wrote…

empty line

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r8, 5 of 5 files at r9.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @ArielElp and @gilbens-starkware)


corelib/src/starknet/storage.cairo line 9 at r5 (raw file):

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

Done.
Changed also the traits, e.g. starknet::storage::VecTrait but not sure if it's better.

in Map is it MapTrait? - if so probably revert that specifically.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp and @gilbens-starkware)

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


corelib/src/starknet/storage.cairo line 9 at r5 (raw file):

Previously, orizi wrote…

in Map is it MapTrait? - if so probably revert that specifically.

Map have only entry, so it is named StoragePathEntry as the same interface can be implemented for other types, but I don't see any such type for now so maybe It should be changed?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


corelib/src/starknet/storage/vec.cairo line 45 at r9 (raw file):

        assert!(index < vec_len, "Index out of bounds");
        self.update(index)
    }

have get instead - return option.

at would probably be through the index trait.

Code quote:

    fn at(self: StoragePath<Vec<T>>, index: u64) -> StoragePath<T> {
        let vec_len = self.len();
        assert!(index < vec_len, "Index out of bounds");
        self.update(index)
    }

commit-id:2da700e2
Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @ArielElp and @orizi)


corelib/src/starknet/storage/vec.cairo line 45 at r9 (raw file):

Previously, orizi wrote…

have get instead - return option.

at would probably be through the index trait.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArielElp)

@gilbens-starkware gilbens-starkware added this pull request to the merge queue Jul 9, 2024
Merged via the queue into dev-v2.7.0 with commit 38b4b8d Jul 9, 2024
44 checks passed
@orizi orizi deleted the spr/main/2da700e2 branch July 11, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants