Fix/accept signed transaction as input

mina-tx/src/transactions.rs

@@ -2,7 +2,7 @@
 
 use crate::{
     errors::MinaTxError,
-    signatures::Sig,
+    signatures::{Sig, TransactionSignature},
     transactions::{
         legacy_tx::LegacyTransaction,
         network_id::NetworkIdEnvelope,
@@ -88,19 +88,33 @@ impl TransactionEnvelope {
     pub fn from_str_network(s: &str, network_id: NetworkIdEnvelope) -> Result<Self, MinaTxError> {
         let s = s.trim();
 
-        // Try parsing as ZkApp transaction first
-        if let Ok(zkapp) = serde_json::from_str::<ZKAppCommand>(s) {
-            return Ok(Self::new_zkapp(network_id.0, zkapp));
+        // Try parsing as a TransactionSignature first (output from a previous signing session).
+        // This enables chained multi-group signing where the output of one session is fed as
+        // input to the next. The inner payload already has the previous signature injected.
+        if let Ok(signed) = serde_json::from_str::<TransactionSignature>(s) {
+            return Ok(signed.payload);
         }
 
-        // Try parsing as Legacy transaction
-        if let Ok(legacy) = serde_json::from_str::<LegacyTransaction>(s) {
-            return Ok(Self::new_legacy(network_id.0, legacy));
-        }
+        // Try parsing as ZkApp transaction first, then Legacy.
+        // IMPORTANT: Do NOT silently swallow parse errors here. If both fail, the caller
+        // needs to see the actual serde errors to diagnose the problem — not a generic
+        // "unknown transaction type" message that tells them nothing.
+        let zkapp_err = match serde_json::from_str::<ZKAppCommand>(s) {
+            Ok(zkapp) => return Ok(Self::new_zkapp(network_id.0, zkapp)),
+            Err(e) => e,
+        };
+
+        let legacy_err = match serde_json::from_str::<LegacyTransaction>(s) {
+            Ok(legacy) => return Ok(Self::new_legacy(network_id.0, legacy)),
+            Err(e) => e,
+        };
 
-        // Neither worked, return an error
         Err(MinaTxError::UnknownTransactionType(
-            "Unable to parse transaction. Expected a valid legacy transaction or ZkApp transaction JSON.".to_string()
+            alloc::format!(
+                "Unable to parse transaction as ZkApp or Legacy.\n  ZkApp parse error: {}\n  Legacy parse error: {}",
+                zkapp_err,
+                legacy_err
+            )
         ))
     }
 
@@ -268,6 +282,104 @@ mod tests {
         assert_eq!(envelope.network_id() as u8, 1);
     }
 
+    /// Regression test: deploy-v0.0.4-unsigned.json fails to parse because ZkappUri and
+    /// TokenSymbol use derive(Serialize, Deserialize) on Vec<u8>, which expects a JSON
+    /// array of integers. But o1js serializes these as plain strings.
+    /// e.g. "zkappUri": "https://..." and "tokenSymbol": "MOCKnE"
+    #[test]
+    fn test_parse_deploy_v004_zkapp_uri_as_string() {
+        let json = include_str!("../tests/data/deploy-v0.0.4-unsigned.json");
+        let result = TransactionEnvelope::from_str_network(
+            json,
+            NetworkIdEnvelope::from(NetworkId::TESTNET),
+        );
+        assert!(
+            result.is_ok(),
+            "Failed to parse deploy-v0.0.4-unsigned.json: {:?}",
+            result.unwrap_err()
+        );
+        let envelope = result.unwrap();
+        assert!(!envelope.is_legacy());
+    }
+
+    /// Minimal reproduction: zkappUri as a string should parse, not require a byte array
+    #[test]
+    fn test_zkapp_uri_string_field() {
+        let json = include_str!("../tests/data/deploy-contract.json");
+        // First ensure the base parses fine (zkappUri: null)
+        let base_result = serde_json::from_str::<ZKAppCommand>(json);
+        assert!(base_result.is_ok(), "Base deploy-contract.json should parse");
+
+        // Now inject a string zkappUri value like o1js produces
+        let modified = json.replace(
+            "\"zkappUri\": null",
+            "\"zkappUri\": \"https://example.com\"",
+        );
+        let result = serde_json::from_str::<ZKAppCommand>(&modified);
+        assert!(
+            result.is_ok(),
+            "ZkApp with string zkappUri should parse but got: {}",
+            result.unwrap_err()
+        );
+    }
+
+    /// ZkappUri with more than 32 characters should parse — the 32-char limit is wrong,
+    /// o1js and the Mina protocol don't enforce it.
+    #[test]
+    fn test_zkapp_uri_longer_than_32_chars() {
+        let json = include_str!("../tests/data/deploy-contract.json");
+        let long_uri = "https://github.com/nori-zk/mock-nori-bridge"; // 45 chars
+        assert!(long_uri.len() > 32);
+
+        let modified = json.replace(
+            "\"zkappUri\": null",
+            &alloc::format!("\"zkappUri\": \"{}\"", long_uri),
+        );
+        let result = serde_json::from_str::<ZKAppCommand>(&modified);
+        assert!(
+            result.is_ok(),
+            "ZkApp with >32 char zkappUri should parse but got: {}",
+            result.unwrap_err()
+        );
+    }
+
+    /// Minimal reproduction: tokenSymbol as a string should parse, not require a byte array
+    #[test]
+    fn test_token_symbol_string_field() {
+        let json = include_str!("../tests/data/deploy-contract.json");
+
+        // Inject a string tokenSymbol value like o1js produces
+        let modified = json.replace(
+            "\"tokenSymbol\": null",
+            "\"tokenSymbol\": \"MOCK\"",
+        );
+        let result = serde_json::from_str::<ZKAppCommand>(&modified);
+        assert!(
+            result.is_ok(),
+            "ZkApp with string tokenSymbol should parse but got: {}",
+            result.unwrap_err()
+        );
+    }
+
+    /// A TransactionSignature (output from a previous FROST signing session) should be
+    /// parseable as input for a subsequent signing session, enabling chained multi-group
+    /// signing of the same transaction.
+    #[test]
+    fn test_parse_signed_transaction_as_input() {
+        let json = include_str!("../tests/data/deploy-v0.0.6-admin-signed.json");
+        let result = TransactionEnvelope::from_str_network(
+            json,
+            NetworkIdEnvelope::from(NetworkId::TESTNET),
+        );
+        assert!(
+            result.is_ok(),
+            "Signed transaction should be parseable as input: {:?}",
+            result.unwrap_err()
+        );
+        let envelope = result.unwrap();
+        assert!(!envelope.is_legacy());
+    }
+
     #[test]
     fn test_from_str_network_invalid_json() {
         let result = TransactionEnvelope::from_str_network(

mina-tx/src/transactions/zkapp_tx.rs

@@ -464,9 +464,32 @@ impl AuthRequired {
     }
 }
 
-#[derive(Clone, Debug, Serialize, Deserialize, Default, PartialEq, Eq)]
+// NOTE: TokenSymbol is serialized by o1js as a JSON string (e.g. "MOCK"), NOT as a byte array.
+// derive(Serialize, Deserialize) on Vec<u8> expects a JSON array like [77,79,67,75] which
+// doesn't match what o1js produces. Custom serde handles the string <-> bytes conversion.
+#[derive(Clone, Debug, Default, PartialEq, Eq)]
 pub struct TokenSymbol(pub Vec<u8>);
 
+impl Serialize for TokenSymbol {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        let s = core::str::from_utf8(&self.0).map_err(serde::ser::Error::custom)?;
+        serializer.serialize_str(s)
+    }
+}
+
+impl<'de> Deserialize<'de> for TokenSymbol {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        let s = String::deserialize(deserializer)?;
+        s.parse().map_err(serde::de::Error::custom)
+    }
+}
+
 impl TokenSymbol {
     pub fn to_bytes(&self, bytes: &mut [u8]) {
         if self.0.is_empty() {
@@ -491,17 +514,38 @@ impl core::str::FromStr for TokenSymbol {
 
 // Default is derived for TokenSymbol
 
-#[derive(Clone, Debug, Serialize, Deserialize, Default, PartialEq, Eq)]
+// NOTE: Same issue as TokenSymbol — o1js serializes zkappUri as a plain JSON string
+// (e.g. "https://github.com/..."), not a byte array. Custom serde required.
+#[derive(Clone, Debug, Default, PartialEq, Eq)]
 pub struct ZkappUri(pub Vec<u8>);
 
+impl Serialize for ZkappUri {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        let s = core::str::from_utf8(&self.0).map_err(serde::ser::Error::custom)?;
+        serializer.serialize_str(s)
+    }
+}
+
+impl<'de> Deserialize<'de> for ZkappUri {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        let s = String::deserialize(deserializer)?;
+        s.parse().map_err(serde::de::Error::custom)
+    }
+}
+
 impl core::str::FromStr for ZkappUri {
     type Err = String;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        if s.len() <= 32 {
-            Ok(Self(s.as_bytes().to_vec()))
-        } else {
-            Err("Zkapp URI must be at most 32 characters".to_string())
-        }
+        // The previous 32-char limit was wrong — o1js doesn't enforce it and real deploy
+        // transactions routinely have longer URIs (e.g. GitHub URLs). The packing/hashing
+        // logic handles arbitrary-length URIs fine.
+        Ok(Self(s.as_bytes().to_vec()))
     }
 }