feat(byte_array): add ByteArray::span and Span::len

[giladchase]

No description provided.

[reviewable-StarkWare]

This change is 

[giladchase]

• feat(byte_array): add at and index to ByteSpan #8427
• feat(byte_array): add slice #8417
• feat(byte_array): add ByteSpan::to_byte_array #8416
• refactor(byte_array): extract ByteArray::append logic #8486
• feat(byte_array): add ByteArray::span and Span::len #8329 👈 (View in Graphite)
• refactor(test): remove fixed-size bytearray testutils #8343
• refactor(test): remove compare_spans util #8342
• refactor(test): remove compare_byte_array util and rework tests #8336
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[TomerStarkware]

@TomerStarkware reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

---

corelib/src/byte_array.cairo line 611 at r1 (raw file):

    fn len(self: @Span) -> usize {
        let raw_data_bytes = self.raw_data.len() * BYTES_IN_BYTES31;
        raw_data_bytes - *self.first_char_start_offset + *self.last_char_end_offset

casting everything to felt252 and then casting back to size would be more performant

Code quote:

raw_data_bytes - *self.first_char_start_offset + *self.last_char_end_offset

[TomerStarkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @orizi)

---

corelib/src/byte_array.cairo line 611 at r1 (raw file):

Previously, TomerStarkware wrote…

casting everything to felt252 and then casting back to size would be more performant

this is wrong

[orizi]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)

---

corelib/src/byte_array.cairo line 593 at r2 (raw file):

/// `Span` implements the `Copy` and the `Drop` traits.
#[derive(Copy, Drop)]
pub struct Span {

probably.

Suggestion:

pub struct ByteSpan {

[giladchase]

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

---

corelib/src/byte_array.cairo line 593 at r2 (raw file):

Previously, orizi wrote…

probably.

Done.

---

corelib/src/byte_array.cairo line 598 at r3 (raw file):

    /// slices.
    /// Value should be in the range [0, 30].
    pub(crate) first_char_start_offset: usize,

Replace here and other with BoundedInt

Code quote:

    /// Value should be in the range [0, 30].
    pub(crate) first_char_start_offset: usize,

[giladchase]

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

---

corelib/src/byte_array.cairo line 634 at r13 (raw file):

Previously, orizi wrote…

bounded-int arithmetics can help here to make only a single cast here.

Thanks.
Unless i'm missing something, i think this might be a bit tricky to do: bounded_sub(remainder_len , start_offset) can in practice be a negative BoundedInt<-30,30> (fore example span.slice(1,span_len() has offset=1 and remainder_len=0 ), and similarly bounded_sub(start_offset,remainder_len) can be negative.
Both cases will require another custom adder between a data_bytes: usize and whatever signed type we upcast the negative number to, which might be more trouble than an extra upcast.

---

corelib/src/byte_array.cairo line 651 at r13 (raw file):

Previously, orizi wrote…

or something of that manner.

Done.

Interestingly, this version seems to be the best performing version, in particular, it's than using data.is_empty() instead of data.len() == 0.
I verified this by (locally) changing array::Span::is_empty from it's current pop_front approach into something more similar to how Span::len is implemented (specifically array_len(self.snapshot) == 0 and gas usage decreased slightly in the relevant test in array_test.cairo.

---

corelib/src/byte_array.cairo line 64 at r13 (raw file):

/// The number of bytes in
pub type WordBytes = BoundedInt<0, BYTES_IN_BYTES31_MINUS_ONE_FELT>;

Done.
Note that once we make ByteArray use this typedef we might want to pub(crate) this typdef, since it will be used in a pub(crate) field of ByteArray (pending_word_len).

[giladchase]

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

---

corelib/src/byte_array.cairo line 651 at r13 (raw file):

Previously, giladchase wrote…

Done.

Interestingly, this version seems to be the best performing version, in particular, it's than using data.is_empty() instead of data.len() == 0.
I verified this by (locally) changing array::Span::is_empty from it's current pop_front approach into something more similar to how Span::len is implemented (specifically array_len(self.snapshot) == 0 and gas usage decreased slightly in the relevant test in array_test.cairo.

BTW the reason why start_offset isn't checked is because start_offset is always < self.remainder_len if data.len() == 0 (when start_offset >= remainder_len and data == [] then slice returns None)

[giladchase]

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

---

corelib/src/byte_array.cairo line 64 at r13 (raw file):

Previously, giladchase wrote…

Done.
Note that once we make ByteArray use this typedef we might want to pub(crate) this typdef, since it will be used in a pub(crate) field of ByteArray (pending_word_len).

The Done is a lie.
Now it's done.

[orizi]

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @TomerStarkware)

---

corelib/src/byte_array.cairo line 634 at r15 (raw file):

    fn len(self: @ByteSpan) -> usize {
        let data_bytes = self.data.len() * BYTES_IN_BYTES31;
        data_bytes + upcast(self.remainder_len) - upcast(self.first_char_start_offset)

Suggestion:

        downcast(bounded_int::sub(
            bounded_int::add(
                bounded_int::mul(self.data.len(), BYTES_IN_BYTES31),
                self.remainder_len),
            ),
            self.first_char_start_offset,
        )).unwrap()

---

corelib/src/byte_array.cairo line 651 at r15 (raw file):

    /// ```
    fn is_empty(self: @ByteSpan) -> bool {
        self.data.len() == 0 && *self.remainder_len == 0

can you check both: (in any case - opposite order probably marginally better)

Suggestion:

        *self.remainder_len == 0 && self.data.len() == 0
        
        |||
        
        *self.remainder_len == 0 && self.data.is_empty()

---

corelib/src/byte_array.cairo line 655 at r15 (raw file):

}

pub trait ToByteSpanTrait<C> {

doc

[giladchase]

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

---

corelib/src/byte_array.cairo line 651 at r15 (raw file):

*self.remainder_len == 0 && self.data.len() == 0

wins, the is_empty variant is a tad slower than this one, so i'm picking the len variant unless you still prefer the is_empty option.

Both of yr suggestions are faster than current code due to the order switch.

---

corelib/src/byte_array.cairo line 634 at r15 (raw file):

    fn len(self: @ByteSpan) -> usize {
        let data_bytes = self.data.len() * BYTES_IN_BYTES31;
        data_bytes + upcast(self.remainder_len) - upcast(self.first_char_start_offset)

Done.
I added the operator impls in a helpers mod like in bytes_31.cairo.
Should i define U32ByB31 in bytes_31? Wasn't sure if it's interesting enough

[orizi]

@orizi reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @giladchase and @TomerStarkware)

---

corelib/src/byte_array.cairo line 65 at r17 (raw file):

/// The number of bytes in [`ByteArray::pending_word`].
type WordBytes = BoundedInt<0, { BYTES_IN_BYTES31_MINUS_ONE.into() }>;
type BYTES_IN_BYTES31_TYPED = UnitInt<{ BYTES_IN_BYTES31.into() }>;

this is a type - not a const.

---

corelib/src/byte_array.cairo line 707 at r17 (raw file):

    }
}
use helpers::{B30AddU32ByB31, B30AddU32ByB31SubB30, U32ByB31};

if you want these just for a single function - add the use within it.
better yet - just add the len calculation as a function in the helper.
(and remove the pub from the impls)

Code quote:

use helpers::{B30AddU32ByB31, B30AddU32ByB31SubB30, U32ByB31};

[giladchase]

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

---

corelib/src/byte_array.cairo line 655 at r15 (raw file):

Previously, orizi wrote…

doc

Done.

---

corelib/src/byte_array.cairo line 65 at r17 (raw file):

Previously, orizi wrote…

this is a type - not a const.

Done.

---

corelib/src/byte_array.cairo line 707 at r17 (raw file):

Previously, orizi wrote…

if you want these just for a single function - add the use within it.
better yet - just add the len calculation as a function in the helper.
(and remove the pub from the impls)

This OK? Or also add push the downcast into it?

[orizi]

@orizi reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @giladchase and @TomerStarkware)

---

corelib/src/byte_array.cairo line 707 at r17 (raw file):

Previously, giladchase wrote…

This OK? Or also add push the downcast into it?

downcast as well IMO - would be better encapsulation.

---

corelib/src/byte_array.cairo line 692 at r18 (raw file):

    impl U32ByB31 of MulHelper<u32, BytesInBytes31Typed> {
        type Result = BoundedInt<0, { U32_MAX_TIMES_B31 }>;

Suggestion:

        type Result = BoundedInt<0, U32_MAX_TIMES_B31>;

[giladchase]

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

---

corelib/src/byte_array.cairo line 707 at r17 (raw file):

Previously, orizi wrote…

downcast as well IMO - would be better encapsulation.

Done.

[orizi]

@orizi reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @giladchase and @TomerStarkware)

---

corelib/src/byte_array.cairo line 715 at r20 (raw file):

    }
}
use helpers::calc_bytespan_len;

no need for the use IMO.

Code quote:

use helpers::calc_bytespan_len;

[giladchase]

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

---

corelib/src/byte_array.cairo line 715 at r20 (raw file):

Previously, orizi wrote…

no need for the use IMO.

Right, replaced with a qualified path.

---

corelib/src/byte_array.cairo line 692 at r18 (raw file):

    impl U32ByB31 of MulHelper<u32, BytesInBytes31Typed> {
        type Result = BoundedInt<0, { U32_MAX_TIMES_B31 }>;

Done.

[orizi]

@orizi reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @TomerStarkware)

---

corelib/src/byte_array.cairo line 650 at r21 (raw file):

    /// ```
    fn is_empty(self: ByteSpan) -> bool {
        self.remainder_len == 0 && self.data.len() == 0

doc why this is enough.

Code quote:

self.remainder_len == 0

[giladchase]

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

---

corelib/src/byte_array.cairo line 650 at r21 (raw file):

Previously, orizi wrote…

doc why this is enough.

Done.

[orizi]

@orizi reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @giladchase and @TomerStarkware)

---

corelib/src/byte_array.cairo line 650 at r21 (raw file):

Previously, giladchase wrote…

Done.

do the implementation - not the function.

[giladchase]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware)

---

corelib/src/byte_array.cairo line 650 at r21 (raw file):

Previously, orizi wrote…

do the implementation - not the function.

Done.

[orizi]

@orizi reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)

---

corelib/src/byte_array.cairo line 650 at r23 (raw file):

    /// ```
    fn is_empty(self: ByteSpan) -> bool {
        // No partial bytes31-sized words in `remainder_word`, nor any full words in `data`.

i meant that you need to doc why self.remainder_len == 0 is enough.
since you COULD have had something like "abcd".get(1..)

Code quote:

        // No partial bytes31-sized words in `remainder_word`, nor any full words in `data`.