cob: Improvements to COB logic

There were a few subtle issues with the apply logic of certain COBs
that should be fixed now.

Note that the underlying store guarantees exactly-once delivery, and so
it does not make sense to test idempotence at the patch level.

radicle/src/cob/patch.rs

@@ -421,8 +421,11 @@ impl store::FromHistory for Patch {
                     revision,
                     description,
                 } => {
-                    if let Some(Redactable::Present(revision)) = self.revisions.get_mut(&revision) {
+                    if let Some(redactable) = self.revisions.get_mut(&revision) {
+                        // If the revision was redacted concurrently, there's nothing to do.
+                        if let Redactable::Present(revision) = redactable {
                             revision.description.set(description, op.clock);
+                        }
                     } else {
                         return Err(Error::Missing(revision));
                     }
@@ -432,12 +435,6 @@ impl store::FromHistory for Patch {
                     base,
                     oid,
                 } => {
-                    // Since revisions are keyed by content hash, we shouldn't re-insert a revision
-                    // if it already exists, otherwise this will be resolved via the `merge`
-                    // operation of `Redactable`.
-                    if self.revisions.contains_key(&id) {
-                        continue;
-                    }
                     self.revisions.insert(
                         id,
                         Redactable::Present(Revision::new(
@@ -451,6 +448,7 @@ impl store::FromHistory for Patch {
                     );
                 }
                 Action::Redact { revision } => {
+                    // Redactions must have observed a revision to be valid.
                     if let Some(revision) = self.revisions.get_mut(&revision) {
                         revision.merge(Redactable::Redacted);
                     } else {
@@ -1347,6 +1345,7 @@ mod test {
     use crate::cob::test::Actor;
     use crate::crypto::test::signer::MockSigner;
     use crate::test;
+    use crate::test::arbitrary;
     use crate::test::arbitrary::gen;
     use crate::test::storage::MockRepository;
 
@@ -1744,9 +1743,9 @@ mod test {
     }
 
     #[test]
-    fn test_revision_redact_reinsert() {
+    fn test_revision_edit_redact() {
-        let base = git::Oid::from_str("cb18e95ada2bb38aadd8e6cef0963ce37a87add3").unwrap();
+        let base = arbitrary::oid();
-        let oid = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d1339380").unwrap();
+        let oid = arbitrary::oid();
         let repo = gen::<MockRepository>(1);
         let mut alice = Actor::new(MockSigner::default());
         let mut p1 = Patch::default();
@@ -1757,41 +1756,15 @@ mod test {
             base,
             oid,
         });
-        let a2 = alice.op(Action::Redact { revision: a1.id() });
+        let a2 = alice.op(Action::Redact { revision: a1.id });
-
+        let a3 = alice.op(Action::EditRevision {
-        p1.apply([a1.clone(), a2.clone(), a1.clone()], &repo)
+            revision: a1.id,
-            .unwrap();
+            description: String::from("Edited"),
-        p2.apply([a1.clone(), a1, a2], &repo).unwrap();
-
-        assert_eq!(p1, p2);
-    }
-
-    #[test]
-    fn test_revision_merge_reinsert() {
-        let base = git::Oid::from_str("cb18e95ada2bb38aadd8e6cef0963ce37a87add3").unwrap();
-        let oid = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d1339380").unwrap();
-        let id = gen::<Id>(1);
-        let mut alice = Actor::new(MockSigner::default());
-        let mut doc = gen::<Doc<Verified>>(1);
-        doc.delegates.push(alice.signer.public_key().into());
-        let repo = MockRepository::new(id, doc);
-
-        let mut p1 = Patch::default();
-        let mut p2 = Patch::default();
-
-        let a1 = alice.op(Action::Revision {
-            description: String::new(),
-            base,
-            oid,
-        });
-        let a2 = alice.op(Action::Merge {
-            revision: a1.id(),
-            commit: oid,
         });
 
-        p1.apply([a1.clone(), a2.clone(), a1.clone()], &repo)
+        p1.apply([a1.clone(), a2.clone(), a3.clone()], &repo)
             .unwrap();
-        p2.apply([a1.clone(), a1, a2], &repo).unwrap();
+        p2.apply([a1, a3, a2], &repo).unwrap();
 
         assert_eq!(p1, p2);
     }

radicle/src/cob/test.rs

@@ -167,7 +167,11 @@ impl<G: Signer> Actor<G> {
         clock: clock::Lamport,
         identity: Oid,
     ) -> Op<A> {
-        let data = encoding::encode(&action).unwrap();
+        let data = encoding::encode(serde_json::json!({
+            "action": action,
+            "nonce": fastrand::u64(..),
+        }))
+        .unwrap();
         let oid = git::raw::Oid::hash_object(git::raw::ObjectType::Blob, &data).unwrap();
         let id = oid.into();
         let author = *self.signer.public_key();

radicle/src/cob/thread.rs

@@ -34,6 +34,12 @@ pub enum Error {
     /// Validation error.
     #[error("validation failed: {0}")]
     Validate(&'static str),
+    /// Error with comment operation.
+    #[error("comment {0} is invalid")]
+    Comment(EntryId),
+    /// Error with edit operation.
+    #[error("edit {0} is invalid")]
+    Edit(EntryId),
 }
 
 /// Identifies a comment.
@@ -288,27 +294,40 @@ impl cob::store::FromHistory for Thread {
 
             match op.action {
                 Action::Comment { body, reply_to } => {
-                    // Since comments are keyed by content hash, we shouldn't re-insert a comment
+                    if body.is_empty() {
-                    // if it already exists, otherwise this will be resolved via the `merge`
+                        return Err(Error::Comment(op.id));
-                    // operation of `Redactable`.
-                    if self.comments.contains_key(&id) {
-                        continue;
                     }
-
+                    // Nb. If a comment is already present, it must be redacted, because the
+                    // underlying store guarantees exactly-once delivery of ops.
                     self.comments.insert(
                         id,
                         Redactable::Present(Comment::new(author, body, reply_to, timestamp)),
                     );
                 }
                 Action::Edit { id, body } => {
-                    if let Some(Redactable::Present(comment)) = self.comments.get_mut(&id) {
+                    if body.is_empty() {
+                        return Err(Error::Edit(op.id));
+                    }
+                    // It's possible for a comment to be redacted before we're able to edit it, in
+                    // case of a concurrent update.
+                    //
+                    // However, it's *not* possible for the comment to be absent. Therefore we treat
+                    // that as an error.
+                    if let Some(redactable) = self.comments.get_mut(&id) {
+                        if let Redactable::Present(comment) = redactable {
                             comment.edit(op.clock, body, timestamp);
+                        }
                     } else {
                         return Err(Error::Missing(id));
                     }
                 }
                 Action::Redact { id } => {
-                    self.comments.insert(id, Redactable::Redacted);
+                    // Redactions must have observed a comment to be valid.
+                    if let Some(comment) = self.comments.get_mut(&id) {
+                        comment.merge(Redactable::Redacted);
+                    } else {
+                        return Err(Error::Missing(id));
+                    }
                 }
                 Action::React {
                     to,
@@ -348,6 +367,7 @@ mod tests {
     use crate::cob::test;
     use crate::crypto::test::signer::MockSigner;
     use crate::crypto::Signer;
+    use crate::test::arbitrary;
     use crate::test::arbitrary::gen;
     use crate::test::storage::MockRepository;
 
@@ -451,7 +471,7 @@ mod tests {
                 );
                 comments.insert(comment.id);
 
-                Some((*clock, comment))
+                Some((clock.tick(), comment))
             })
             .variant(2, |(actor, clock, comments), rng| {
                 if comments.is_empty() {
@@ -465,8 +485,7 @@ mod tests {
                         .collect::<String>()
                         .as_str(),
                 );
-
+                Some((clock.tick(), edit))
-                Some((*clock, edit))
             })
             .variant(2, |(actor, clock, comments), rng| {
                 if comments.is_empty() {
@@ -488,7 +507,7 @@ mod tests {
                 Some((clock.tick(), redact))
             });
 
-            let mut ops = vec![Actor::<MockSigner>::default().comment("", None)];
+            let mut ops = vec![Actor::<MockSigner>::default().comment("Root", None)];
             let mut permutations: [Vec<Op<Action>>; N] = array::from_fn(|_| Vec::new());
 
             for (_, op) in gen.take(g.size()) {
@@ -661,21 +680,37 @@ mod tests {
     }
 
     #[test]
-    fn test_comment_edit_reinsert() {
+    fn test_comment_redact_missing() {
         let repo = gen::<MockRepository>(1);
-
         let mut alice = Actor::<MockSigner>::default();
-        let mut t1 = Thread::default();
+        let mut t = Thread::default();
-        let mut t2 = Thread::default();
+        let id = arbitrary::entry_id();
 
-        let a1 = alice.comment("Hello.", None);
+        t.apply([alice.redact(id)], &repo).unwrap_err();
-        let a2 = alice.edit(a1.id(), "Hello World.");
+    }
 
-        t1.apply([a1.clone(), a2.clone(), a1.clone()], &repo)
+    #[test]
-            .unwrap();
+    fn test_comment_edit_missing() {
-        t2.apply([a1.clone(), a1, a2], &repo).unwrap();
+        let repo = gen::<MockRepository>(1);
+        let mut alice = Actor::<MockSigner>::default();
+        let mut t = Thread::default();
+        let id = arbitrary::entry_id();
 
-        assert_eq!(t1, t2);
+        t.apply([alice.edit(id, "Edited")], &repo).unwrap_err();
+    }
+
+    #[test]
+    fn test_comment_edit_redacted() {
+        let repo = gen::<MockRepository>(1);
+        let mut alice = Actor::<MockSigner>::default();
+        let mut t = Thread::default();
+
+        let a1 = alice.comment("Hi", None);
+        let a2 = alice.redact(a1.id);
+        let a3 = alice.edit(a1.id, "Edited");
+
+        t.apply([a1, a2, a3], &repo).unwrap();
+        assert_eq!(t.comments().count(), 0);
     }
 
     #[test]

radicle/src/test/arbitrary.rs

@@ -9,7 +9,6 @@ use nonempty::NonEmpty;
 use qcheck::Arbitrary;
 
 use crate::collections::HashMap;
-use crate::git;
 use crate::identity::{
     doc::{Doc, Id},
     project::Project,
@@ -19,12 +18,17 @@ use crate::node::Address;
 use crate::storage;
 use crate::storage::refs::{Refs, SignedRefs};
 use crate::test::storage::{MockRepository, MockStorage};
+use crate::{cob, git};
 
 pub fn oid() -> storage::Oid {
     let oid_bytes: [u8; 20] = gen(1);
     storage::Oid::try_from(oid_bytes.as_slice()).unwrap()
 }
 
+pub fn entry_id() -> cob::EntryId {
+    self::oid().into()
+}
+
 pub fn refstring(len: usize) -> git::RefString {
     let mut buf = Vec::<u8>::new();
     for _ in 0..len {