Make MemoPlaintext have an opaque constructor

This also has the side effect of making PCLI more robust to users
placing a plaintext that's too large.

crates/bin/pcli/src/command/ceremony.rs

@@ -54,10 +54,10 @@ async fn handle_bid(app: &mut App, to: Address, from: AddressIndex, bid: &str) -
         return Ok(());
     }
 
-    let memo_plaintext = MemoPlaintext {
-        return_address: app.config.full_viewing_key.payment_address(from).0,
-        text: "E PLURIBUS UNUM".to_owned(),
-    };
+    let memo_plaintext = MemoPlaintext::new(
+        app.config.full_viewing_key.payment_address(from).0,
+        "E PLURIBUS UNUM".into(),
+    )?;
 
     let mut planner = Planner::new(OsRng);
     planner.set_gas_prices(gas_prices);

crates/bin/pcli/src/command/tx.rs

@@ -292,10 +292,8 @@ impl TxCmd {
                     .payment_address((*from).into())
                     .0;
 
-                let memo_plaintext = MemoPlaintext {
-                    return_address,
-                    text: memo.clone().unwrap_or_default(),
-                };
+                let memo_plaintext =
+                    MemoPlaintext::new(return_address, memo.clone().unwrap_or_default())?;
 
                 let mut planner = Planner::new(OsRng);
                 planner.set_gas_prices(gas_prices);

crates/core/transaction/src/memo.rs

@@ -22,6 +22,16 @@ pub const MEMO_CIPHERTEXT_LEN_BYTES: usize = 528;
 // This is the `MEMO_CIPHERTEXT_LEN_BYTES` - MAC size (16 bytes).
 pub const MEMO_LEN_BYTES: usize = 512;
 
+// This is the largest text length we can support
+const MAX_TEXT_LEN: usize = MEMO_LEN_BYTES - ADDRESS_LEN_BYTES;
+
+/// A method which reads out bytes in a lossy way, and trims out null bytes
+fn raw_bytes_to_text(data: &[u8]) -> String {
+    String::from_utf8_lossy(data)
+        .trim_end_matches(0u8 as char)
+        .to_string()
+}
+
 #[derive(Clone, Debug)]
 pub struct MemoCiphertext(pub [u8; MEMO_CIPHERTEXT_LEN_BYTES]);
 
@@ -33,8 +43,35 @@ impl EffectingData for MemoCiphertext {
 
 #[derive(Clone, Debug, PartialEq)]
 pub struct MemoPlaintext {
-    pub return_address: Address,
-    pub text: String,
+    return_address: Address,
+    text: String,
+}
+
+impl MemoPlaintext {
+    /// Create a new MemoPlaintext, checking that the text isn't long enough.
+    ///
+    /// The text being too long is the only reason this function will fail.
+    pub fn new(return_address: Address, text: String) -> anyhow::Result<Self> {
+        if text.len() > MAX_TEXT_LEN {
+            anyhow::bail!(
+                "memo text length must be <= {}, found {}",
+                MAX_TEXT_LEN,
+                text.len()
+            );
+        }
+        Ok(Self {
+            return_address,
+            text,
+        })
+    }
+
+    pub fn return_address(&self) -> Address {
+        self.return_address
+    }
+
+    pub fn text(&self) -> &str {
+        self.text.as_str()
+    }
 }
 
 impl From<&MemoPlaintext> for Vec<u8> {
@@ -55,20 +92,9 @@ impl TryFrom<Vec<u8>> for MemoPlaintext {
         }
         let return_address_bytes = &bytes[..80];
         let return_address: Address = return_address_bytes.try_into()?;
-        let text = String::from_utf8_lossy(&bytes[80..])
-            .trim_end_matches(0u8 as char)
-            .to_string();
-        if (text).len() > MEMO_LEN_BYTES - ADDRESS_LEN_BYTES {
-            anyhow::bail!(
-                "provided memo text exceeds {} bytes",
-                MEMO_LEN_BYTES - ADDRESS_LEN_BYTES
-            );
-        }
+        let text = raw_bytes_to_text(&bytes[80..]);
 
-        Ok(MemoPlaintext {
-            return_address,
-            text,
-        })
+        MemoPlaintext::new(return_address, text)
     }
 }
 
@@ -115,14 +141,9 @@ impl MemoCiphertext {
 
         let return_address_bytes = &plaintext_bytes[..80];
         let return_address: Address = return_address_bytes.try_into()?;
-        let text = String::from_utf8_lossy(&plaintext_bytes[80..])
-            .trim_end_matches(0u8 as char)
-            .to_string();
+        let text = raw_bytes_to_text(&plaintext_bytes[80..]);
 
-        Ok(MemoPlaintext {
-            return_address: return_address,
-            text,
-        })
+        MemoPlaintext::new(return_address, text)
     }
 
     /// Decrypt a [`MemoCiphertext`] to generate a fixed-length slice of bytes.
@@ -167,14 +188,9 @@ impl MemoCiphertext {
 
         let return_address_bytes = &plaintext_bytes[..80];
         let return_address: Address = return_address_bytes.try_into()?;
-        let text = String::from_utf8_lossy(&plaintext_bytes[80..])
-            .trim_end_matches(0u8 as char)
-            .to_string();
+        let text = raw_bytes_to_text(&plaintext_bytes[80..]);
 
-        Ok(MemoPlaintext {
-            return_address: return_address,
-            text,
-        })
+        MemoPlaintext::new(return_address, text)
     }
 }
 
@@ -294,7 +310,7 @@ mod tests {
     }
 
     #[test]
-    fn test_memo_encryption_and_sender_decryption() {
+    fn test_memo_encryption_and_sender_decryption() -> anyhow::Result<()> {
         let mut rng = OsRng;
 
         let seed_phrase = SeedPhrase::generate(rng);
@@ -315,10 +331,7 @@ mod tests {
 
         // On the sender side, we have to encrypt the memo to put into the transaction-level,
         // and also the memo key to put on the action-level (output).
-        let memo = MemoPlaintext {
-            return_address: dest,
-            text: String::from("Hello, friend"),
-        };
+        let memo = MemoPlaintext::new(dest, "Hello, friend".into())?;
         let memo_key = PayloadKey::random_key(&mut OsRng);
         let ciphertext =
             MemoCiphertext::encrypt(memo_key.clone(), &memo).expect("can encrypt memo");
@@ -348,6 +361,8 @@ mod tests {
         .expect("can decrypt memo");
 
         assert_eq!(plaintext, memo);
+
+        Ok(())
     }
 
     proptest! {
@@ -364,9 +379,10 @@ mod tests {
             let memo_key = PayloadKey::random_key(&mut rng);
             let memo_address = Address::dummy(&mut rng);
             let memo_text = s;
-            let memo = MemoPlaintext {
-                return_address: memo_address,
-                text: memo_text,
+            let memo = {
+                let memo = MemoPlaintext::new(memo_address, memo_text);
+                assert!(memo.is_ok());
+                memo.unwrap()
             };
             let ciphertext_result = MemoCiphertext::encrypt(memo_key.clone(), &memo);
             if memo.to_vec().len() > MEMO_LEN_BYTES {

crates/core/transaction/src/plan.rs

@@ -345,9 +345,7 @@ impl TransactionPlan {
         }
 
         if !clue_plans.is_empty() {
-            self.detection_data = Some(DetectionDataPlan {
-                clue_plans: clue_plans,
-            });
+            self.detection_data = Some(DetectionDataPlan { clue_plans });
         } else {
             self.detection_data = None;
         }
@@ -477,10 +475,7 @@ mod tests {
 
         let mut rng = OsRng;
 
-        let memo_plaintext = MemoPlaintext {
-            return_address: Address::dummy(&mut rng),
-            text: "".to_string(),
-        };
+        let memo_plaintext = MemoPlaintext::new(Address::dummy(&mut rng), "".to_string()).unwrap();
         let plan = TransactionPlan {
             // Put outputs first to check that the auth hash
             // computation is not affected by plan ordering.

crates/core/transaction/src/plan/memo.rs

@@ -34,8 +34,8 @@ impl DomainType for MemoPlan {
 
 impl From<MemoPlan> for pb::MemoPlan {
     fn from(msg: MemoPlan) -> Self {
-        let return_address = Some(msg.plaintext.return_address.into());
-        let text = msg.plaintext.text;
+        let return_address = Some(msg.plaintext.return_address().into());
+        let text = msg.plaintext.text().to_owned();
         Self {
             plaintext: Some(pb::MemoPlaintext {
                 return_address,
@@ -67,10 +67,7 @@ impl TryFrom<pb::MemoPlan> for MemoPlan {
         let key = PayloadKey::try_from(msg.key.to_vec())?;
 
         Ok(Self {
-            plaintext: MemoPlaintext {
-                return_address: sender,
-                text,
-            },
+            plaintext: MemoPlaintext::new(sender, text)?,
             key,
         })
     }

crates/core/transaction/src/transaction.rs

@@ -301,8 +301,8 @@ impl Transaction {
             Some(ciphertext) => match memo_plaintext {
                 Some(plaintext) => {
                     let plaintext_view: MemoPlaintextView = MemoPlaintextView {
-                        return_address: txp.view_address(plaintext.return_address),
-                        text: plaintext.text,
+                        return_address: txp.view_address(plaintext.return_address()),
+                        text: plaintext.text().to_owned(),
                     };
                     Some(MemoView::Visible {
                         plaintext: plaintext_view,

crates/view/src/service.rs

@@ -793,7 +793,7 @@ impl ViewProtocolService for ViewService {
                     let memo = tx.decrypt_memo(&fvk).map_err(|_| {
                         tonic::Status::internal("Error decrypting memo for OutputView")
                     })?;
-                    address_views.insert(memo.return_address, fvk.view_address(address));
+                    address_views.insert(memo.return_address(), fvk.view_address(address));
                 }
                 ActionView::Swap(SwapView::Visible { swap_plaintext, .. }) => {
                     let address = swap_plaintext.claim_address;

crates/view/src/storage.rs

@@ -1539,7 +1539,7 @@ impl Storage {
                 let tx_hash_owned = sha2::Sha256::digest(&tx_bytes);
                 let tx_hash = tx_hash_owned.as_slice();
                 let tx_block_height = filtered_block.height as i64;
-                let return_address = transaction.decrypt_memo(&fvk).map_or(None, |x| Some(x.return_address.to_vec()));
+                let return_address = transaction.decrypt_memo(&fvk).map_or(None, |x| Some(x.return_address().to_vec()));
 
                 tracing::debug!(tx_hash = ?hex::encode(tx_hash), "recording extended transaction");
 

crates/wasm/src/tx.rs

@@ -323,7 +323,7 @@ pub async fn transaction_info_inner(
 
                 // Also add an AddressView for the return address in the memo.
                 let memo = tx.decrypt_memo(&fvk)?;
-                address_views.insert(memo.return_address, fvk.view_address(address));
+                address_views.insert(memo.return_address(), fvk.view_address(address));
             }
             ActionView::Swap(SwapView::Visible { swap_plaintext, .. }) => {
                 let address = swap_plaintext.claim_address;