chore: Remove Clone from EventInsertable

[stbrody]

Builds on #480

[stbrody]

this feels a bit hacky and probably not the most efficient. The other idea I had was to replace the pop().unwrap() with remove(0), but I was worried that removing from the front of the array repeatedly would be even less efficient than reversing at the end.

It really feels like there should be a better way to move elements out of one vector into two others in an efficient manor...

[stbrody]

Adding @nathanielc to see if you have any ideas for how to do this more efficiently

[dbcfd]

VecDeque and push front.

[stbrody]

What does this mean? I have a vector, I want to move the elements from that vector out into two other vectors, deciding which vector to move into based on a property of the values in the input vector. How can a VecDeque help with that?

[dbcfd]

Instead of using a vector to store events, use a VecDeque. You push front instead of back. You then return an object with a VecDeque.

[nathanielc]

Make sure I understand. We have three vectors:

• candidate_events
• deliverable
• remaining_candidates

We want to move the values from candidate_events into one of the other two vectors and we want to do it in order? When we are done candidate_events is empty?

In this case a simple for loop should suffice.

for event in candidate_events {
   if ... {deliverable.push(event)} else {remaining_candidates.push(event) }
}

Note since we are not using a reference to candidate_events in the for loop the values are moved out of the vector into the loop.

Aside there is a one liner for this in nightly rust https://doc.rust-lang.org/std/vec/struct.Vec.html#method.extract_if

[stbrody]

Nice! For some reason I thought we couldn't just move objects out of a vector like that without some sort of explicit "removal" operation.

I was worried about what kind of state this would leave the Vec in, but I guess because the for loop didn't take a reference but took the Vec by value it took ownership of it and now it's illegal to make any further references to candidate_events. Very cool!

[dav1do]

Yeah, the order isn't important here until we start appending non-init events to the deliverable list (since they need to be delivered after). I just used a stack/pop because it's fastest on a Vec. We could pass in the two vectors (deliverable, unknown) if we prefer but I originally just kept knowing that an init is deliverable in here. The remaining candidates don't need to be in any particular order as they will be sorted by prev chain separately so no need to reverse them.

Two things I noticed that were of my making.. I allocate an unnecessary vec when we could just have:

if remaining_candidates.is_empty() {
            return Ok(OrderEvents {
                deliverable,
                missing_history: remaining_candidates, // Vec::new(),
            });
        }

And we could do this in one pass if we make deliverable a public field.. it gets to Danny's point about style, originally I wanted to keep that an internal detail but it really doesn't make a lot of sense to use the setter.. so with that change we could have:

    pub async fn try_new(
        pool: &SqlitePool,
        candidate_events: Vec<(EventInsertable, EventMetadata)>,
    ) -> Result<Self> {
        let mut deliverable = Vec::with_capacity(candidate_events.len());
        let mut remaining_candidates = Vec::with_capacity(candidate_events.len());

        let mut new_cids: HashMap<Cid, bool> =
            HashMap::from_iter(candidate_events.into_iter().map(|(mut e, meta)| {
                // all init events are deliverable so we mark them as such before we do anything else
                let cid = e.cid();
                let deliverable = if matches!(meta, EventMetadata::Init { .. }) {
                    e.deliverable = true;
                    deliverable.push((e, meta));
                    true
                } else {
                    remaining_candidates.push((e, meta));
                    false
                };
                (cid, deliverable)
            }));

        if remaining_candidates.is_empty() {
            return Ok(OrderEvents {
                deliverable,
                missing_history: Vec::new(),
            });
        }

[stbrody]

This looks like a nice cleanup, but I'd like to do that in a separate PR to keep this one more focused.

[stbrody]

Made a ticket to revisit this: https://linear.app/3boxlabs/issue/AES-309/cleanup-ordereventstry-new

[nathanielc]

this change is O(n) when previously it was constant time. Not sure it matters but thought I would point it out.

[stbrody]

yeah, but this is test code, and we know the vector is only 10 long, so I'm not too worried about it

[nathanielc]

Make sure I understand. We have three vectors:

• candidate_events
• deliverable
• remaining_candidates

We want to move the values from candidate_events into one of the other two vectors and we want to do it in order? When we are done candidate_events is empty?

In this case a simple for loop should suffice.

for event in candidate_events {
   if ... {deliverable.push(event)} else {remaining_candidates.push(event) }
}

Note since we are not using a reference to candidate_events in the for loop the values are moved out of the vector into the loop.

Aside there is a one liner for this in nightly rust https://doc.rust-lang.org/std/vec/struct.Vec.html#method.extract_if

[nathanielc]

How are the two vectors different? Should we make a small struct to give them names?

[stbrody]

There's context at the call site, but as far as this function is concerned it doesn't care, it's just making two vectors

[dav1do]

I'm good with this as is, I noticed two things that could be nicer (both originally my mistakes) so you can take them or leave them.

[dav1do]

Yeah, the order isn't important here until we start appending non-init events to the deliverable list (since they need to be delivered after). I just used a stack/pop because it's fastest on a Vec. We could pass in the two vectors (deliverable, unknown) if we prefer but I originally just kept knowing that an init is deliverable in here. The remaining candidates don't need to be in any particular order as they will be sorted by prev chain separately so no need to reverse them.

Two things I noticed that were of my making.. I allocate an unnecessary vec when we could just have:

if remaining_candidates.is_empty() {
            return Ok(OrderEvents {
                deliverable,
                missing_history: remaining_candidates, // Vec::new(),
            });
        }

And we could do this in one pass if we make deliverable a public field.. it gets to Danny's point about style, originally I wanted to keep that an internal detail but it really doesn't make a lot of sense to use the setter.. so with that change we could have:

    pub async fn try_new(
        pool: &SqlitePool,
        candidate_events: Vec<(EventInsertable, EventMetadata)>,
    ) -> Result<Self> {
        let mut deliverable = Vec::with_capacity(candidate_events.len());
        let mut remaining_candidates = Vec::with_capacity(candidate_events.len());

        let mut new_cids: HashMap<Cid, bool> =
            HashMap::from_iter(candidate_events.into_iter().map(|(mut e, meta)| {
                // all init events are deliverable so we mark them as such before we do anything else
                let cid = e.cid();
                let deliverable = if matches!(meta, EventMetadata::Init { .. }) {
                    e.deliverable = true;
                    deliverable.push((e, meta));
                    true
                } else {
                    remaining_candidates.push((e, meta));
                    false
                };
                (cid, deliverable)
            }));

        if remaining_candidates.is_empty() {
            return Ok(OrderEvents {
                deliverable,
                missing_history: Vec::new(),
            });
        }