cob: Consolidate `from_history` impls

By implementing `FromHistory::apply`, COBs don't need to implement
`from_history`.

Signed-off-by: Alexis Sellier <self@cloudhead.io>

radicle/src/cob/issue.rs

@@ -1,4 +1,4 @@
-use std::ops::{ControlFlow, Deref};
+use std::ops::Deref;
 use std::str::FromStr;
 
 use once_cell::sync::Lazy;
@@ -10,6 +10,7 @@ use radicle_crdt::{LWWReg, LWWSet, Max, Semilattice};
 
 use crate::cob;
 use crate::cob::common::{Author, Reaction, Tag};
+use crate::cob::store::FromHistory as _;
 use crate::cob::store::Transaction;
 use crate::cob::thread;
 use crate::cob::thread::{CommentId, Thread};
@@ -17,8 +18,6 @@ use crate::cob::{store, ActorId, ObjectId, OpId, TypeName};
 use crate::crypto::{PublicKey, Signer};
 use crate::storage::git as storage;
 
-use super::op::Ops;
-
 /// Issue operation.
 pub type Op = cob::Op<Action>;
 
@@ -111,27 +110,44 @@ impl Default for Issue {
 
 impl store::FromHistory for Issue {
     type Action = Action;
+    type Error = Error;
 
     fn type_name() -> &'static TypeName {
         &*TYPENAME
     }
 
-    fn from_history(
+    fn apply(&mut self, ops: impl IntoIterator<Item = Op>) -> Result<(), Error> {
-        history: &radicle_cob::History,
+        for op in ops {
-    ) -> Result<(Self, clock::Lamport), store::Error> {
+            match op.action {
-        let obj = history.traverse(Self::default(), |mut acc, entry| {
+                Action::Assign { add, remove } => {
-            if let Ok(Ops(ops)) = Ops::try_from(entry) {
+                    for assignee in add {
-                if let Err(err) = acc.apply(ops) {
+                        self.assignees.insert(assignee, op.clock);
-                    log::warn!("Error applying op to issue state: {err}");
+                    }
-                    return ControlFlow::Break(acc);
+                    for assignee in remove {
+                        self.assignees.remove(assignee, op.clock);
+                    }
+                }
+                Action::Edit { title } => {
+                    self.title.set(title, op.clock);
+                }
+                Action::Lifecycle { state } => {
+                    self.state.set(state, op.clock);
+                }
+                Action::Tag { add, remove } => {
+                    for tag in add {
+                        self.tags.insert(tag, op.clock);
+                    }
+                    for tag in remove {
+                        self.tags.remove(tag, op.clock);
+                    }
+                }
+                Action::Thread { action } => {
+                    self.thread
+                        .apply([cob::Op::new(action, op.author, op.timestamp, op.clock)])?;
                 }
-            } else {
-                return ControlFlow::Break(acc);
             }
-            ControlFlow::Continue(acc)
+        }
-        });
+        Ok(())
-
-        Ok((obj, history.clock().into()))
     }
 }
 
@@ -166,40 +182,6 @@ impl Issue {
     pub fn comments(&self) -> impl Iterator<Item = (&CommentId, &thread::Comment)> {
         self.thread.comments()
     }
-
-    pub fn apply(&mut self, ops: impl IntoIterator<Item = Op>) -> Result<(), Error> {
-        for op in ops {
-            match op.action {
-                Action::Assign { add, remove } => {
-                    for assignee in add {
-                        self.assignees.insert(assignee, op.clock);
-                    }
-                    for assignee in remove {
-                        self.assignees.remove(assignee, op.clock);
-                    }
-                }
-                Action::Edit { title } => {
-                    self.title.set(title, op.clock);
-                }
-                Action::Lifecycle { state } => {
-                    self.state.set(state, op.clock);
-                }
-                Action::Tag { add, remove } => {
-                    for tag in add {
-                        self.tags.insert(tag, op.clock);
-                    }
-                    for tag in remove {
-                        self.tags.remove(tag, op.clock);
-                    }
-                }
-                Action::Thread { action } => {
-                    self.thread
-                        .apply([cob::Op::new(action, op.author, op.timestamp, op.clock)])?;
-                }
-            }
-        }
-        Ok(())
-    }
 }
 
 impl Deref for Issue {

radicle/src/cob/patch.rs

@@ -1,6 +1,5 @@
 #![allow(clippy::too_many_arguments)]
 use std::fmt;
-use std::ops::ControlFlow;
 use std::ops::Deref;
 use std::ops::Range;
 use std::str::FromStr;
@@ -14,7 +13,7 @@ use radicle_crdt::{GMap, LWWReg, LWWSet, Max, Redactable, Semilattice};
 
 use crate::cob;
 use crate::cob::common::{Author, Tag, Timestamp};
-use crate::cob::op::Ops;
+use crate::cob::store::FromHistory as _;
 use crate::cob::store::Transaction;
 use crate::cob::thread;
 use crate::cob::thread::CommentId;
@@ -227,9 +226,17 @@ impl Patch {
     pub fn is_archived(&self) -> bool {
         matches!(self.state.get().get(), &State::Archived)
     }
+}
 
-    /// Apply a list of operations to the state.
+impl store::FromHistory for Patch {
-    pub fn apply(&mut self, ops: impl IntoIterator<Item = Op>) -> Result<(), ApplyError> {
+    type Action = Action;
+    type Error = ApplyError;
+
+    fn type_name() -> &'static TypeName {
+        &*TYPENAME
+    }
+
+    fn apply(&mut self, ops: impl IntoIterator<Item = Op>) -> Result<(), ApplyError> {
         for op in ops {
             let id = op.id();
             let author = Author::new(op.author);
@@ -313,32 +320,6 @@ impl Patch {
     }
 }
 
-impl store::FromHistory for Patch {
-    type Action = Action;
-
-    fn type_name() -> &'static TypeName {
-        &*TYPENAME
-    }
-
-    fn from_history(
-        history: &radicle_cob::History,
-    ) -> Result<(Self, clock::Lamport), store::Error> {
-        let obj = history.traverse(Self::default(), |mut acc, entry| {
-            if let Ok(Ops(ops)) = Ops::try_from(entry) {
-                if let Err(err) = acc.apply(ops) {
-                    log::warn!("Error applying op to patch state: {err}");
-                    return ControlFlow::Break(acc);
-                }
-            } else {
-                return ControlFlow::Break(acc);
-            }
-            ControlFlow::Continue(acc)
-        });
-
-        Ok((obj, history.clock().into()))
-    }
-}
-
 /// A patch revision.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Revision {

radicle/src/cob/store.rs

@@ -2,14 +2,15 @@
 #![allow(clippy::large_enum_variant)]
 #![allow(clippy::type_complexity)]
 use std::marker::PhantomData;
+use std::ops::ControlFlow;
 
 use nonempty::NonEmpty;
 use radicle_crdt::Lamport;
-use serde::Serialize;
+use serde::{Deserialize, Serialize};
 
 use crate::cob;
 use crate::cob::common::Author;
-use crate::cob::op::OpId;
+use crate::cob::op::{Op, OpId, Ops};
 use crate::cob::CollaborativeObject;
 use crate::cob::{ActorId, Create, History, ObjectId, TypeName, Update};
 use crate::crypto::PublicKey;
@@ -24,14 +25,35 @@ pub const HISTORY_TYPE: &str = "radicle";
 
 /// A type that can be materialized from an event history.
 /// All collaborative objects implement this trait.
-pub trait FromHistory: Sized {
+pub trait FromHistory: Sized + Default {
-    // TODO(finto): Action not being used smells fishy to me
+    /// The underlying action composing each operation.
-    type Action;
+    type Action: for<'de> Deserialize<'de>;
+    /// Error returned by `apply` function.
+    type Error: std::error::Error;
 
     /// The object type name.
     fn type_name() -> &'static TypeName;
+
+    /// Apply a list of operations to the state.
+    fn apply(&mut self, ops: impl IntoIterator<Item = Op<Self::Action>>)
+        -> Result<(), Self::Error>;
+
     /// Create an object from a history.
-    fn from_history(history: &History) -> Result<(Self, Lamport), Error>;
+    fn from_history(history: &History) -> Result<(Self, Lamport), Error> {
+        let obj = history.traverse(Self::default(), |mut acc, entry| {
+            if let Ok(Ops(ops)) = Ops::try_from(entry) {
+                if let Err(err) = acc.apply(ops) {
+                    log::warn!("Error applying op to `{}` state: {err}", Self::type_name());
+                    return ControlFlow::Break(acc);
+                }
+            } else {
+                return ControlFlow::Break(acc);
+            }
+            ControlFlow::Continue(acc)
+        });
+
+        Ok((obj, history.clock().into()))
+    }
 }
 
 /// Store error.

radicle/src/cob/thread.rs

@@ -1,6 +1,8 @@
 use std::cmp::Ordering;
 use std::ops::{Deref, DerefMut};
+use std::str::FromStr;
 
+use once_cell::sync::Lazy;
 use radicle_crdt as crdt;
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
@@ -13,6 +15,11 @@ use crate::crypto::Signer;
 use crdt::clock::Lamport;
 use crdt::{GMap, LWWSet, Max, Redactable, Semilattice};
 
+/// Type name of a thread, as well as the domain for all thread operations.
+/// Note that threads are not usually used standalone. They are embeded into other COBs.
+pub static TYPENAME: Lazy<cob::TypeName> =
+    Lazy::new(|| FromStr::from_str("xyz.radicle.thread").expect("type name is valid"));
+
 /// Error applying an operation onto a state.
 #[derive(Error, Debug)]
 pub enum OpError {
@@ -228,7 +235,26 @@ impl Thread {
             .map(|(a, r)| (a, r))
     }
 
-    pub fn apply(&mut self, ops: impl IntoIterator<Item = Op<Action>>) -> Result<(), OpError> {
+    pub fn comments(&self) -> impl Iterator<Item = (&CommentId, &Comment)> + '_ {
+        self.comments.iter().filter_map(|(id, comment)| {
+            if let Redactable::Present(c) = comment {
+                Some((id, c))
+            } else {
+                None
+            }
+        })
+    }
+}
+
+impl cob::store::FromHistory for Thread {
+    type Action = Action;
+    type Error = OpError;
+
+    fn type_name() -> &'static radicle_cob::TypeName {
+        &*TYPENAME
+    }
+
+    fn apply(&mut self, ops: impl IntoIterator<Item = Op<Action>>) -> Result<(), OpError> {
         for op in ops.into_iter() {
             let id = op.id();
             let author = op.author;
@@ -270,16 +296,6 @@ impl Thread {
         }
         Ok(())
     }
-
-    pub fn comments(&self) -> impl Iterator<Item = (&CommentId, &Comment)> + '_ {
-        self.comments.iter().filter_map(|(id, comment)| {
-            if let Redactable::Present(c) = comment {
-                Some((id, c))
-            } else {
-                None
-            }
-        })
-    }
 }
 
 /// An object that can be used to create and sign changes.
@@ -346,13 +362,9 @@ impl<G> DerefMut for Actor<G> {
 #[cfg(test)]
 mod tests {
     use std::collections::BTreeSet;
-    use std::ops::ControlFlow;
-    use std::str::FromStr;
     use std::{array, iter};
 
-    use cob::op::Ops;
     use nonempty::NonEmpty;
-    use once_cell::sync::Lazy;
     use pretty_assertions::assert_eq;
     use qcheck::{Arbitrary, TestResult};
 
@@ -360,34 +372,9 @@ mod tests {
 
     use super::*;
     use crate as radicle;
+    use crate::cob::store::FromHistory;
     use crate::crypto::test::signer::MockSigner;
 
-    /// Type name of a thread.
-    pub static TYPENAME: Lazy<cob::TypeName> =
-        Lazy::new(|| FromStr::from_str("xyz.radicle.thread").expect("type name is valid"));
-
-    impl cob::store::FromHistory for Thread {
-        type Action = Action;
-
-        fn type_name() -> &'static radicle_cob::TypeName {
-            &*TYPENAME
-        }
-
-        fn from_history(history: &cob::History) -> Result<(Self, Lamport), cob::store::Error> {
-            let obj = history.traverse(Thread::default(), |mut acc, entry| {
-                if let Ok(Ops(ops)) = Ops::try_from(entry) {
-                    if acc.apply(ops).is_err() {
-                        return ControlFlow::Break(acc);
-                    }
-                } else {
-                    return ControlFlow::Break(acc);
-                }
-                ControlFlow::Continue(acc)
-            });
-            Ok((obj, history.clock().into()))
-        }
-    }
-
     #[derive(Clone)]
     struct Changes<const N: usize> {
         permutations: [Vec<Op<Action>>; N],