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

Added Store impl for ByteArray. #4596

Merged
merged 1 commit into from
Dec 27, 2023
Merged

Added Store impl for ByteArray. #4596

merged 1 commit into from
Dec 27, 2023

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Dec 18, 2023

This change is Reviewable

@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch from c844d18 to c29f10f Compare December 18, 2023 12:59
@orizi orizi force-pushed the pr/orizi/refactoring/e523c677 branch from c4b247d to d10df3f Compare December 18, 2023 12:59
@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch from c29f10f to 83b54b8 Compare December 18, 2023 13:23
@orizi orizi force-pushed the pr/orizi/refactoring/e523c677 branch from d10df3f to f4bf8a7 Compare December 18, 2023 13:23
@orizi orizi changed the base branch from pr/orizi/refactoring/e523c677 to main December 18, 2023 13:48
@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch from 83b54b8 to 936c3df Compare December 18, 2023 13:48
@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch from 936c3df to 901a9db Compare December 18, 2023 13:56
Copy link
Contributor

@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, 5 unresolved discussions (waiting on @ArielElp, @liorgold2, and @orizi)


corelib/src/integer.cairo line 313 at r1 (raw file):

}

pub(crate) extern fn u8_overflowing_add(

Consider exposing also the sub for completeness.

Code quote:

pub(crate) 

corelib/src/starknet/storage_access.cairo line 679 at r1 (raw file):

    #[inline(always)]
    fn size() -> u8 {
        1

Shouldn't it include all the fragments of the array?

Code quote:

  1

corelib/src/starknet/storage_access.cairo line 696 at r1 (raw file):

        match starknet::syscalls::storage_read_syscall(address_domain, address)?.try_into() {
        Option::Some(x) => x,
        Option::None => { return starknet::SyscallResult::Err(array!['Invalid length']); },

Suggestion:

SyscallResult::Err(array!['Invalid length']); 

corelib/src/starknet/storage_access.cairo line 756 at r1 (raw file):

    let mut index_in_chunk = 0_u8;
    loop {
        let value = match full_words.pop_front() {

Suggestion:

let cur_value

corelib/src/starknet/storage_access.cairo line 768 at r1 (raw file):

            Result::Err(err) => { break Result::Err(err); },
        };
        index_in_chunk = match core::integer::u8_overflowing_add(index_in_chunk, 1) {

Doc somewhere that there are 255 values in a chunk.

Code quote:

 index_in_chunk = match core::integer::u8_overflowing_add(index_in_chunk, 1) 

@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch from 901a9db to b0a800d Compare December 19, 2023 09:34
@orizi orizi changed the base branch from main to pr/orizi/refactoring/6146bb3a December 19, 2023 09:34
Copy link
Contributor

@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.

Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @ArielElp, @liorgold2, and @orizi)


corelib/src/starknet/storage_access.cairo line 699 at r1 (raw file):

    };
    let (mut remaining_full_words, pending_word_len) = core::DivRem::div_rem(
        len, 31_usize.try_into().unwrap()

From bytes_31.cairo

Suggestion:

BYTES_IN_BYTES31

Copy link
Collaborator Author

@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: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @liorgold2)


corelib/src/integer.cairo line 313 at r1 (raw file):

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

Consider exposing also the sub for completeness.

i currently add it in an ad-hoc needed basic - this will only be exported with a proper trait - so rather not add other things i would later remove.


corelib/src/starknet/storage_access.cairo line 679 at r1 (raw file):

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

Shouldn't it include all the fragments of the array?

definitely not - this is the amount of data in the current stream of felts.


corelib/src/starknet/storage_access.cairo line 696 at r1 (raw file):

        match starknet::syscalls::storage_read_syscall(address_domain, address)?.try_into() {
        Option::Some(x) => x,
        Option::None => { return starknet::SyscallResult::Err(array!['Invalid length']); },

Done.


corelib/src/starknet/storage_access.cairo line 768 at r1 (raw file):

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

Doc somewhere that there are 255 values in a chunk.

Adding - do note it is 256 though.

@orizi orizi changed the base branch from pr/orizi/refactoring/6146bb3a to main December 19, 2023 10:08
@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch from b0a800d to b611d0a Compare December 19, 2023 10:08
Copy link
Collaborator

@liorgold2 liorgold2 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: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 683 at r2 (raw file):

}

/// Returns a pointer to the `chunk`'th chunk of the byte array at `address`.

Document the hash we're using.

Copy link
Collaborator

@liorgold2 liorgold2 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: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 683 at r2 (raw file):

Previously, liorgold2 wrote…

Document the hash we're using.

Also, add something like: address is the storage address containing the length of the array. The data of the array is stored in chunks...
(I think this function is the one that deserves the documentation of how we store byte arrays)

Copy link
Collaborator

@liorgold2 liorgold2 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: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 684 at r2 (raw file):

/// Returns a pointer to the `chunk`'th chunk of the byte array at `address`.
fn inner_byte_array_pointer(address: StorageAddress, chunk: usize) -> StorageBaseAddress {

Why not call the arg address_domain as you do in the other functions?


corelib/src/starknet/storage_access.cairo line 691 at r2 (raw file):

}

/// Reads a byte array from storage from domain `address_domain` and address `address`.

Add something like:

`address_domain` is the address containing the length of the array. See [inner_byte_array_pointer] for more details.

Copy link
Collaborator

@liorgold2 liorgold2 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: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 694 at r2 (raw file):

fn inner_read_byte_array(address_domain: u32, address: StorageAddress) -> SyscallResult<ByteArray> {
    let len: usize =
        match starknet::syscalls::storage_read_syscall(address_domain, address)?.try_into() {

(This formatting is weird) (nonblocking)

Copy link
Collaborator

@liorgold2 liorgold2 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: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 696 at r2 (raw file):

        match starknet::syscalls::storage_read_syscall(address_domain, address)?.try_into() {
        Option::Some(x) => x,
        Option::None => { return starknet::SyscallResult::Err(array!['Invalid length']); },

.

Suggestion:

Invalid ByteArray length

Copy link
Collaborator

@liorgold2 liorgold2 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: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 699 at r1 (raw file):

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

From bytes_31.cairo

We need to support nonzero constants or be able to optimize this out.
(not part of this PR, nonblocking)

Copy link
Collaborator

@liorgold2 liorgold2 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: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 725 at r2 (raw file):

            Result::Ok(x) => x,
            Result::Err(x) => {
                chunk += 1;

Nice implementation

Copy link
Collaborator

@liorgold2 liorgold2 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: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 727 at r2 (raw file):

                chunk += 1;
                chunk_base = inner_byte_array_pointer(address, chunk);
                x

// x is zero in this case.

Copy link
Collaborator

@liorgold2 liorgold2 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: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 684 at r2 (raw file):

Previously, liorgold2 wrote…

Why not call the arg address_domain as you do in the other functions?

Sorry. Ignore.

Copy link
Collaborator

@liorgold2 liorgold2 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: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 725 at r2 (raw file):

            Result::Ok(x) => x,
            Result::Err(x) => {
                chunk += 1;

Isn't it more efficient to have chuck: felt252?

@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch 4 times, most recently from cbf6a85 to 2991479 Compare December 24, 2023 15:36
@orizi orizi changed the base branch from main to pr/orizi/refactoring/d7d88701 December 24, 2023 15:37
@orizi orizi changed the base branch from pr/orizi/refactoring/d7d88701 to main December 24, 2023 15:52
@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch from 2991479 to 9920582 Compare December 24, 2023 15:52
Copy link
Contributor

@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: 10 of 14 files reviewed, 7 unresolved discussions (waiting on @ArielElp, @liorgold2, and @orizi)


-- commits line 7 at r9:
Rebase needed

Code quote:

New commits in r9 on 24/12/2023 at 17:52, probably rebased from r8:

- 48a1e41: Added proper jump and branch alignment validations.

  commit-id:d7d88701

  fixup! Added more basic validation when building program registry.

@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch from 9920582 to 0dbb044 Compare December 25, 2023 09:19
Copy link
Collaborator

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r6, 1 of 3 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: 13 of 14 files reviewed, 10 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 656 at r10 (raw file):

/// Store for a `ByteArray`.
/// The layout of a `ByteArray` in storage is as follows:

Add a blank line above.


corelib/src/starknet/storage_access.cairo line 657 at r10 (raw file):

/// Store for a `ByteArray`.
/// The layout of a `ByteArray` in storage is as follows:
/// * Written in the storage is only the length in bytes.

Suggestion:

/// * Only the length in bytes is stored in the original address where the byte array is logically stored.

corelib/src/starknet/storage_access.cairo line 659 at r10 (raw file):

/// * Written in the storage is only the length in bytes.
/// * The actual data is stored in chunks of 256 `bytes31`s in another place in storage
///   determined by a hash over:

Suggestion:

/// * The actual data is stored in chunks of 256 `bytes31`s in another place in storage
///   determined by the hash of:

corelib/src/starknet/storage_access.cairo line 696 at r10 (raw file):

/// * `address` - The address "containing" the ByteArray (but actually stores just the length).
/// * `chunk` - The index of the chunk.
/// * The short string `ByteArray` as the capacity.

Suggestion:

/// The pointer is the `Poseidon` hash of:
/// * `address` - The address "containing" the `ByteArray`.
/// * `chunk` - The index of the chunk.
/// * The short string `ByteArray` is used as the capacity argument of the sponge construction (domain separation).

Copy link
Collaborator

@liorgold2 liorgold2 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: 13 of 14 files reviewed, 10 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 694 at r10 (raw file):

/// Returns a pointer to the `chunk`'th chunk of the byte array at `address`.
/// The pointer is a `Poseidon` hash over:
/// * `address` - The address "containing" the ByteArray (but actually stores just the length).

Actually, let's go with:

Suggestion:

/// * `address` - The address of the ByteArray (where the length is stored).

Copy link
Collaborator

@liorgold2 liorgold2 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 r7.
Reviewable status: 13 of 14 files reviewed, 7 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 747 at r10 (raw file):

    }?;
    if pending_word_len != 0 {
        result

Formatting is weird. Consider adding a variable to make it look better.

Copy link
Collaborator

@liorgold2 liorgold2 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 r8.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArielElp and @orizi)


corelib/src/integer.cairo line 313 at r1 (raw file):

Previously, orizi wrote…

i currently add it in an ad-hoc needed basic - this will only be exported with a proper trait - so rather not add other things i would later remove.

(another option is to repeat this extern where needed)


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

    assert!(test_contract::__external::set_data(serialized(x)).is_empty());
    println!("write successful.");

Intended? If so write -> Write

Copy link
Collaborator

@liorgold2 liorgold2 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 r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArielElp and @orizi)

Copy link
Collaborator

@liorgold2 liorgold2 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, 8 unresolved discussions (waiting on @ArielElp and @orizi)


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

        @test_contract::__external::get_byte_arrays(serialized(())), @serialized(x), 'Wrong result'
    );
}

Add some lines that show the values in the storage by their actual keys. Something like:

let base_addr = 0x324387978432...;
assert!(storage_at(base_addr) == 0);
assert!(storage_at(base_addr + 1) == 15);
assert!(storage_at(base_addr + 2) == 36);
assert!(storage_at(base_addr + 3) == 16384);
assert!(storage_at(posiedon(base_addr + 3, 1, 'ByteArray')) == "bytes");

Copy link
Collaborator Author

@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, 7 unresolved discussions (waiting on @ArielElp and @liorgold2)


corelib/src/starknet/storage_access.cairo line 656 at r10 (raw file):

Previously, liorgold2 wrote…

Add a blank line above.

Done.


corelib/src/starknet/storage_access.cairo line 657 at r10 (raw file):

/// Store for a `ByteArray`.
/// The layout of a `ByteArray` in storage is as follows:
/// * Written in the storage is only the length in bytes.

Done.


corelib/src/starknet/storage_access.cairo line 659 at r10 (raw file):

/// * Written in the storage is only the length in bytes.
/// * The actual data is stored in chunks of 256 `bytes31`s in another place in storage
///   determined by a hash over:

Done.


corelib/src/starknet/storage_access.cairo line 694 at r10 (raw file):

Previously, liorgold2 wrote…

Actually, let's go with:

Done.


corelib/src/starknet/storage_access.cairo line 696 at r10 (raw file):

/// * `address` - The address "containing" the ByteArray (but actually stores just the length).
/// * `chunk` - The index of the chunk.
/// * The short string `ByteArray` as the capacity.

Done.


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

Previously, liorgold2 wrote…

Intended? If so write -> Write

removed.


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

Previously, liorgold2 wrote…

Add some lines that show the values in the storage by their actual keys. Something like:

let base_addr = 0x324387978432...;
assert!(storage_at(base_addr) == 0);
assert!(storage_at(base_addr + 1) == 15);
assert!(storage_at(base_addr + 2) == 36);
assert!(storage_at(base_addr + 3) == 16384);
assert!(storage_at(posiedon(base_addr + 3, 1, 'ByteArray')) == "bytes");

Done.

@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch from 0dbb044 to ca3d896 Compare December 27, 2023 07:28
Copy link
Collaborator

@liorgold2 liorgold2 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 r11, all commit messages.
Reviewable status: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


corelib/src/starknet/storage_access.cairo line 694 at r10 (raw file):

Previously, orizi wrote…

Done.

Not done yet.

Copy link
Collaborator

@liorgold2 liorgold2 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: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


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

        @test_contract::__external::get_byte_arrays(serialized(())), @serialized(x), 'Wrong result'
    );
    // Making sure the lengths was saved correctly.

Suggestion:

    // Make sure the lengths were saved correctly.

Copy link
Collaborator

@liorgold2 liorgold2 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: 13 of 14 files reviewed, 4 unresolved discussions (waiting on @ArielElp, @gilbens-starkware, and @orizi)


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

    );
    // Making sure the lengths was saved correctly.
    let base_address = starknet::storage_base_address_from_felt252(selector!("byte_arrays"));

Nice. Didn't know we had selector!.

Copy link
Collaborator

@liorgold2 liorgold2 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 r11.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArielElp and @orizi)


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

    assert!(starknet::Store::read_at_offset(0, base_address, 2).unwrap() == 36_usize);
    assert!(starknet::Store::read_at_offset(0, base_address, 3).unwrap() == 16384_usize);
    // Making sure the internal data was saved correctly.

.

Suggestion:

    // Make sure the internal data was saved correctly.

@orizi orizi force-pushed the pr/orizi/refactoring/03f478da branch from ca3d896 to 57c1de6 Compare December 27, 2023 13:17
Copy link
Collaborator Author

@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: 12 of 14 files reviewed, 4 unresolved discussions (waiting on @ArielElp and @liorgold2)


corelib/src/starknet/storage_access.cairo line 694 at r10 (raw file):

Previously, liorgold2 wrote…

Not done yet.

Done.


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

        @test_contract::__external::get_byte_arrays(serialized(())), @serialized(x), 'Wrong result'
    );
    // Making sure the lengths was saved correctly.

Done.


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

Previously, liorgold2 wrote…

Nice. Didn't know we had selector!.

Done.


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

Previously, liorgold2 wrote…

.

Done.

Copy link
Collaborator

@liorgold2 liorgold2 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 2 of 2 files at r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArielElp)

@orizi orizi added this pull request to the merge queue Dec 27, 2023
Merged via the queue into main with commit ee00efb Dec 27, 2023
@orizi orizi deleted the pr/orizi/refactoring/03f478da branch December 27, 2023 14:10
@ArielElp ArielElp mentioned this pull request Dec 28, 2023
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.

3 participants