Add support for PodList #92

grod220 · 2025-06-06T12:17:37Z

Currently, PodSliceMut has served as the data structure for dynamically sized list. As we expand functionality to this, it is starting to feel misnamed (as Slices generally aren't things that are resized).

Changes made

Add new PodList struct that serves as the future home for mutations involving ordered lists
Copy over methods 1:1 from PodSliceMut: unpack, init, push
Add new remove methods: remove_at, remove_first_where + new tests
Mark PodSliceMut as deprecated

grod220 · 2025-06-06T12:19:16Z

pod/src/list.rs

+    fn unpack_internal<'a>(data: &'a mut [u8], init: bool) -> Result<Self, ProgramError>
+    where
+        'a: 'data,
+    {
+        if data.len() < LENGTH_SIZE {
+            return Err(PodSliceError::BufferTooSmall.into());
+        }
+        let (length, data) = data.split_at_mut(LENGTH_SIZE);
+        let length = pod_from_bytes_mut::<PodU32>(length)?;
+        if init {
+            *length = 0.into();
+        }
+        let max_length = max_len_for_type::<T>(data.len(), u32::from(*length) as usize)?;
+        let data = pod_slice_from_bytes_mut(data)?;
+        Ok(Self {
+            length,
+            data,
+            max_length,
+        })
+    }
+
+    /// Unpack the mutable buffer into a mutable slice
+    pub fn unpack<'a>(data: &'a mut [u8]) -> Result<Self, ProgramError>
+    where
+        'a: 'data,
+    {
+        Self::unpack_internal(data, /* init */ false)
+    }
+
+    /// Unpack the mutable buffer into a mutable slice, and initialize the
+    /// slice to 0-length
+    pub fn init<'a>(data: &'a mut [u8]) -> Result<Self, ProgramError>
+    where
+        'a: 'data,
+    {
+        Self::unpack_internal(data, /* init */ true)
+    }
+
+    /// Add another item to the slice
+    pub fn push(&mut self, t: T) -> Result<(), ProgramError> {
+        let length = u32::from(*self.length);
+        if length as usize == self.max_length {
+            Err(PodSliceError::BufferTooSmall.into())
+        } else {
+            self.data[length as usize] = t;
+            *self.length = length.saturating_add(1).into();
+            Ok(())
+        }
+    }


These are 1:1 copied from PodSliceMut

I was actually hoping we could turn this into free functions and that way we don't have to maintain two at once, but instead we can just call into them from each API. What do you think?

joncinque

Mostly small points, looks good to me!

As another naming idea, what we're really providing is a View on existing data, so it's like a Vec that won't reallocate, which also similar to an arena allocator.

So we could go with ListView or NoAllocVec or ArenaList. I kind of like the last one, since it might describe the situation best.

joncinque · 2025-06-06T21:58:03Z

pod/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "spl-pod"
-version = "0.5.1"
+version = "0.6.0"


The publish script will take care of bumping the version, no need to do it here

joncinque · 2025-06-06T21:58:36Z

pod/src/slice.rs

@@ -109,7 +115,7 @@ impl<'data, T: Pod> PodSliceMut<'data, T> {
    }
 }

-fn max_len_for_type<T>(data_len: usize, length_val: usize) -> Result<usize, ProgramError> {
+pub fn max_len_for_type<T>(data_len: usize, length_val: usize) -> Result<usize, ProgramError> {


Let's make this pub(crate) if it's needed elsewhere

joncinque · 2025-06-06T21:59:42Z

tlv-account-resolution/src/state.rs

@@ -1,13 +1,14 @@
 //! State transition types

+use spl_pod::list::PodList;


nit: it looks like we never added the format config to CI, but this should go in the big use statement

febo · 2025-06-07T00:46:45Z

pod/src/list.rs

+/// ## Memory Layout
+///
+/// The structure assumes the underlying byte buffer is formatted as follows:
+/// 1.  **Length**: A `u32` value (`PodU32`) at the beginning of the buffer,


Having 4 bytes as a "header" shifts the offset for the array of Ts to only accept types with alignments 1, 2, or 4 in the case that the byte array has alignment 8. It would be more flexible to have 8 bytes (PodU64) so the offset is then compatible with 1, 2, 4 and 8. Another alternative is to have the length as a generic type.

It should probably be a generic, so that we can offer a way for existing implementations dependent on PodSlice/PodSliceMut to reliably read their data, which is going to be serialized with a u32 length prefix.

Sounds good!

febo · 2025-06-07T00:48:26Z

pod/src/list.rs

+/// 2.  **Data**: The remaining part of the buffer, which is treated as a slice
+///     of `T` elements. The capacity of the collection is the number of `T`
+///     elements that can fit into this data portion.
+pub struct PodList<'data, T: Pod> {


Having Pod in the name is a bit misleading since PodList is not a Pod type.

febo · 2025-06-07T00:52:46Z

pod/src/list.rs

+impl<'data, T: Pod> PodList<'data, T> {
+    /// Unpack the mutable buffer into a mutable slice, with the option to
+    /// initialize the data
+    fn unpack_internal<'a>(data: &'a mut [u8], init: bool) -> Result<Self, ProgramError>


You could add a #[inline(always)] here since this is wrapped in other methods.

febo · 2025-06-07T00:57:50Z

pod/src/list.rs

+        self.data.copy_within(tail_start..len, index);
+
+        // Zero-fill the now-unused slot at the end
+        let last = len.checked_sub(1).ok_or(ProgramError::ArithmeticOverflow)?;


Since the len is already validated, you might not need to do a checked_sub.

febo · 2025-06-07T01:04:25Z

pod/src/list.rs

+
+    /// Find the first element that satisfies `predicate` and remove it,
+    /// returning the element.
+    pub fn remove_first_where<P>(&mut self, mut predicate: P) -> Result<T, ProgramError>


Does it need to be a mut predicate?

febo

Looks very nice – left a few comments. The main one is about the alignment. It would be nice if we could make it more flexible to support types 8-byte aligned.

febo · 2025-06-07T01:35:14Z

So we could go with ListView or NoAllocVec or ArenaList. I kind of like the last one, since it might describe the situation best.

I have a slight preference for ListView – I am not sure if users will make the connection between arena allocator and ArenaList. 😊

buffalojoec · 2025-06-07T04:20:12Z

So we could go with ListView or NoAllocVec or ArenaList. I kind of like the last one, since it might describe the situation best.

I have a slight preference for ListView – I am not sure if users will make the connection between arena allocator and ArenaList. 😊

Agreed, I personally have never heard the term "arena" in this context before, and I assume most developers will whiff on the underlying meaning at first. However, "view" implies readonly, and that's not the case here.

Here are a few ideas:

PodList (I think it's actually ok)
StackList
BoxList
Dynamic + any of the above
PodSpan

buffalojoec · 2025-06-07T04:21:13Z

pod/src/list.rs

+    fn unpack_internal<'a>(data: &'a mut [u8], init: bool) -> Result<Self, ProgramError>
+    where
+        'a: 'data,
+    {
+        if data.len() < LENGTH_SIZE {
+            return Err(PodSliceError::BufferTooSmall.into());
+        }
+        let (length, data) = data.split_at_mut(LENGTH_SIZE);
+        let length = pod_from_bytes_mut::<PodU32>(length)?;
+        if init {
+            *length = 0.into();
+        }
+        let max_length = max_len_for_type::<T>(data.len(), u32::from(*length) as usize)?;
+        let data = pod_slice_from_bytes_mut(data)?;
+        Ok(Self {
+            length,
+            data,
+            max_length,
+        })
+    }
+
+    /// Unpack the mutable buffer into a mutable slice
+    pub fn unpack<'a>(data: &'a mut [u8]) -> Result<Self, ProgramError>
+    where
+        'a: 'data,
+    {
+        Self::unpack_internal(data, /* init */ false)
+    }
+
+    /// Unpack the mutable buffer into a mutable slice, and initialize the
+    /// slice to 0-length
+    pub fn init<'a>(data: &'a mut [u8]) -> Result<Self, ProgramError>
+    where
+        'a: 'data,
+    {
+        Self::unpack_internal(data, /* init */ true)
+    }
+
+    /// Add another item to the slice
+    pub fn push(&mut self, t: T) -> Result<(), ProgramError> {
+        let length = u32::from(*self.length);
+        if length as usize == self.max_length {
+            Err(PodSliceError::BufferTooSmall.into())
+        } else {
+            self.data[length as usize] = t;
+            *self.length = length.saturating_add(1).into();
+            Ok(())
+        }
+    }


I was actually hoping we could turn this into free functions and that way we don't have to maintain two at once, but instead we can just call into them from each API. What do you think?

buffalojoec · 2025-06-07T04:22:10Z

pod/src/list.rs

+#[cfg(test)]
+mod tests {


Building on my other comment about the reusable free functions, we could probably either test just the free functions (in one place) or make the tests generic over each collection API.

buffalojoec · 2025-06-07T04:23:04Z

pod/src/slice.rs

+#[deprecated(
+    since = "0.6.0",
+    note = "This struct will be removed in the next major release (1.0.0). Please use `PodList` instead."
+)]


I wonder if deprecation is really necessary here. I don't really see any harm in leaving the slice-based ones alone, even if the mutable one isn't super aptly-named. I could be swayed here, though.

Note that if we're deprecating then I suppose we don't need the free functions, but it may not be a bad idea regardless, especially if we ever build more list-based APIs.

febo · 2025-06-07T10:18:11Z

So we could go with ListView or NoAllocVec or ArenaList. I kind of like the last one, since it might describe the situation best.

I have a slight preference for ListView – I am not sure if users will make the connection between arena allocator and ArenaList. 😊

Agreed, I personally have never heard the term "arena" in this context before, and I assume most developers will whiff on the underlying meaning at first. However, "view" implies readonly, and that's not the case here.

Here are a few ideas:

PodList (I think it's actually ok)

StackList

BoxList

Dynamic + any of the above

PodSpan

Agree with your point on the "View" implying read-only, but while the type has a "dynamic" size its capacity is fixed. In that sense it is a view. 😊

The only concern that I have with using Pod* is that the type itself is not Pod, so it might look a bit odd.

grod220 commented Jun 6, 2025

View reviewed changes

grod220 force-pushed the podslice-remove-methods branch 2 times, most recently from 730f79e to 4f972f1 Compare June 6, 2025 12:29

Add support for PodList

817eb9f

grod220 force-pushed the podslice-remove-methods branch from 4f972f1 to 817eb9f Compare June 6, 2025 12:33

grod220 requested review from febo, joncinque and buffalojoec June 6, 2025 12:36

joncinque reviewed Jun 6, 2025

View reviewed changes

febo reviewed Jun 7, 2025

View reviewed changes

buffalojoec reviewed Jun 7, 2025

View reviewed changes

		@@ -1,13 +1,14 @@
		//! State transition types

		use spl_pod::list::PodList;

		#[cfg(test)]
		mod tests {

Add support for PodList #92

Are you sure you want to change the base?

Add support for PodList #92

Uh oh!

Conversation

grod220 commented Jun 6, 2025

Changes made

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

febo Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

febo left a comment

Choose a reason for hiding this comment

Uh oh!

febo commented Jun 7, 2025

Uh oh!

buffalojoec commented Jun 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

febo commented Jun 7, 2025

Uh oh!

Uh oh!

febo Jun 7, 2025 •

edited

Loading