Merge pull request #293 from semiotic-ai/suchapalaver/refactor-tap-receipt-unique-hash

refactor(eip712): simplify message unique id

Author: suchapalaver

tap_aggregator/src/aggregator/v1.rs

@@ -19,7 +19,7 @@ pub fn check_and_aggregate_receipts(
     wallet: &PrivateKeySigner,
     accepted_addresses: &HashSet<Address>,
 ) -> Result<Eip712SignedMessage<ReceiptAggregateVoucher>> {
-    check_signatures_unique(domain_separator, receipts)?;
+    check_signatures_unique(receipts)?;
 
     // Check that the receipts are signed by an accepted signer address
     receipts.par_iter().try_for_each(|receipt| {
@@ -93,17 +93,14 @@ fn check_allocation_id(
     Ok(())
 }
 
-fn check_signatures_unique(
-    domain_separator: &Eip712Domain,
-    receipts: &[Eip712SignedMessage<Receipt>],
-) -> Result<()> {
+fn check_signatures_unique(receipts: &[Eip712SignedMessage<Receipt>]) -> Result<()> {
     let mut receipt_signatures = HashSet::new();
     for receipt in receipts.iter() {
-        let signature = receipt.unique_id(domain_separator)?;
+        let signature = receipt.unique_id();
         if !receipt_signatures.insert(signature) {
             return Err(tap_core::Error::DuplicateReceiptSignature(format!(
                 "{:?}",
-                receipt.unique_id(domain_separator)?
+                receipt.unique_id()
             ))
             .into());
         }
@@ -221,7 +218,7 @@ mod tests {
 
         // This should return an error because the signatures are different
         // but the messages are the same, which if allowed would present a security vulnerability
-        let result = check_signatures_unique(&domain_separator, &receipts);
+        let result = check_signatures_unique(&receipts);
 
         // The result should be an error because the malleated signature is not treated as unique
         // and is detected as a duplicate
@@ -246,7 +243,7 @@ mod tests {
         receipts.push(receipt.clone());
         receipts.push(receipt);
 
-        let res = check_signatures_unique(&domain_separator, &receipts);
+        let res = check_signatures_unique(&receipts);
         assert!(res.is_err());
     }
 
@@ -273,7 +270,7 @@ mod tests {
             .unwrap(),
         ];
 
-        let res = check_signatures_unique(&domain_separator, &receipts);
+        let res = check_signatures_unique(&receipts);
         assert!(res.is_ok());
     }
 

tap_aggregator/src/aggregator/v2.rs

@@ -19,7 +19,7 @@ pub fn check_and_aggregate_receipts(
     wallet: &PrivateKeySigner,
     accepted_addresses: &HashSet<Address>,
 ) -> Result<Eip712SignedMessage<ReceiptAggregateVoucher>> {
-    check_signatures_unique(domain_separator, receipts)?;
+    check_signatures_unique(receipts)?;
 
     // Check that the receipts are signed by an accepted signer address
     receipts.par_iter().try_for_each(|receipt| {
@@ -148,17 +148,14 @@ fn check_allocation_id(
     Ok(())
 }
 
-fn check_signatures_unique(
-    domain_separator: &Eip712Domain,
-    receipts: &[Eip712SignedMessage<Receipt>],
-) -> Result<()> {
+fn check_signatures_unique(receipts: &[Eip712SignedMessage<Receipt>]) -> Result<()> {
     let mut receipt_signatures = HashSet::new();
     for receipt in receipts.iter() {
-        let signature = receipt.unique_id(domain_separator)?;
+        let signature = receipt.unique_id();
         if !receipt_signatures.insert(signature) {
             return Err(tap_core::Error::DuplicateReceiptSignature(format!(
                 "{:?}",
-                receipt.unique_id(domain_separator)?
+                receipt.unique_id()
             ))
             .into());
         }
@@ -254,7 +251,7 @@ mod tests {
         receipts.push(receipt.clone());
         receipts.push(receipt);
 
-        let res = super::check_signatures_unique(&domain_separator, &receipts);
+        let res = super::check_signatures_unique(&receipts);
         assert!(res.is_err());
     }
 
@@ -284,7 +281,7 @@ mod tests {
             .unwrap(),
         ];
 
-        let res = super::check_signatures_unique(&domain_separator, &receipts);
+        let res = super::check_signatures_unique(&receipts);
         assert!(res.is_ok());
     }
 

tap_core/src/manager/tap_manager.rs

@@ -138,13 +138,12 @@ where
         let mut failed_receipts = vec![];
 
         // check for timestamp
-        let (checking_receipts, already_failed) = TimestampCheck(min_timestamp_ns)
-            .check_batch(&self.domain_separator, checking_receipts)?;
+        let (checking_receipts, already_failed) =
+            TimestampCheck(min_timestamp_ns).check_batch(checking_receipts)?;
         failed_receipts.extend(already_failed);
 
         // check for uniqueness
-        let (checking_receipts, already_failed) =
-            UniqueCheck.check_batch(&self.domain_separator, checking_receipts)?;
+        let (checking_receipts, already_failed) = UniqueCheck.check_batch(checking_receipts)?;
         failed_receipts.extend(already_failed);
 
         for receipt in checking_receipts.into_iter() {

tap_core/tests/manager_test.rs

@@ -139,7 +139,7 @@ async fn manager_verify_and_store_varying_initial_checks(
         &signer,
     )
     .unwrap();
-    let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
+    let query_id = signed_receipt.unique_hash();
     query_appraisals.write().unwrap().insert(query_id, value);
     escrow_storage
         .write()
@@ -182,7 +182,7 @@ async fn manager_create_rav_request_all_valid_receipts(
             &signer,
         )
         .unwrap();
-        let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
+        let query_id = signed_receipt.unique_hash();
         stored_signed_receipts.push(signed_receipt.clone());
         query_appraisals.write().unwrap().insert(query_id, value);
         assert!(manager
@@ -277,7 +277,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts(
             &signer,
         )
         .unwrap();
-        let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
+        let query_id = signed_receipt.unique_hash();
         stored_signed_receipts.push(signed_receipt.clone());
         query_appraisals.write().unwrap().insert(query_id, value);
         assert!(manager
@@ -321,7 +321,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts(
         )
         .unwrap();
 
-        let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
+        let query_id = signed_receipt.unique_hash();
         stored_signed_receipts.push(signed_receipt.clone());
         query_appraisals.write().unwrap().insert(query_id, value);
         assert!(manager
@@ -389,7 +389,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts_consecutive_tim
         receipt.timestamp_ns = starting_min_timestamp + query_id + 1;
         let signed_receipt = Eip712SignedMessage::new(&domain_separator, receipt, &signer).unwrap();
 
-        let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
+        let query_id = signed_receipt.unique_hash();
         stored_signed_receipts.push(signed_receipt.clone());
         query_appraisals.write().unwrap().insert(query_id, value);
         assert!(manager
@@ -436,7 +436,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts_consecutive_tim
         let mut receipt = Receipt::new(allocation_ids[0], value).unwrap();
         receipt.timestamp_ns = starting_min_timestamp + query_id + 1;
         let signed_receipt = Eip712SignedMessage::new(&domain_separator, receipt, &signer).unwrap();
-        let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
+        let query_id = signed_receipt.unique_hash();
         stored_signed_receipts.push(signed_receipt.clone());
         query_appraisals.write().unwrap().insert(query_id, value);
         assert!(manager

tap_core/tests/received_receipt_test.rs

@@ -113,7 +113,7 @@ async fn partial_then_full_check_valid_receipt(
     )
     .unwrap();
 
-    let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
+    let query_id = signed_receipt.unique_hash();
 
     // add escrow for sender
     escrow_storage
@@ -156,7 +156,7 @@ async fn partial_then_finalize_valid_receipt(
         &signer,
     )
     .unwrap();
-    let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
+    let query_id = signed_receipt.unique_hash();
 
     // add escrow for sender
     escrow_storage
@@ -203,7 +203,7 @@ async fn standard_lifetime_valid_receipt(
     )
     .unwrap();
 
-    let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
+    let query_id = signed_receipt.unique_hash();
 
     // add escrow for sender
     escrow_storage

tap_eip712_message/src/lib.rs

@@ -25,7 +25,7 @@
 use serde::{Deserialize, Serialize};
 use thegraph_core::alloy::{
     dyn_abi::Eip712Domain,
-    primitives::{keccak256, Address, Signature},
+    primitives::{Address, Signature},
     signers::{local::PrivateKeySigner, SignerSync},
     sol_types::SolStruct,
 };
@@ -105,21 +105,8 @@ impl<M: SolStruct> Eip712SignedMessage<M> {
         Ok(recovered_address)
     }
 
-    /// Generate a uniqueness identifier using both the message content and the recovered signer
-    pub fn unique_hash(&self, domain_separator: &Eip712Domain) -> Result<MessageId, Eip712Error> {
-        // Get the hash of just the message content
-        let message_hash = self.message.eip712_hash_struct();
-
-        // Recover the signer address
-        let signer = self.recover_signer(domain_separator)?;
-
-        // Create a new hash combining both the message hash and the signer address
-        let mut input = Vec::with_capacity(32 + 20); // 32 bytes for hash, 20 bytes for address
-        input.extend_from_slice(&message_hash.0);
-        input.extend_from_slice(signer.as_ref());
-
-        let combined_hash = keccak256(&input);
-
-        Ok(MessageId(*combined_hash))
+    /// Use this as a simple key for testing
+    pub fn unique_hash(&self) -> MessageId {
+        MessageId(self.message.eip712_hash_struct().into())
     }
 }

tap_receipt/src/checks.rs

@@ -37,7 +37,6 @@ use std::{
 };
 
 use tap_eip712_message::Eip712Error;
-use thegraph_core::alloy::dyn_abi::Eip712Domain;
 
 use super::{
     state::{Checking, Failed},
@@ -97,7 +96,6 @@ type CheckBatchResponse<Rcpt> = (
 pub trait CheckBatch<Rcpt> {
     fn check_batch(
         &self,
-        domain_separator: &Eip712Domain,
         receipts: Vec<ReceiptWithState<Checking, Rcpt>>,
     ) -> Result<CheckBatchResponse<Rcpt>, Eip712Error>;
 }
@@ -157,7 +155,6 @@ where
 {
     fn check_batch(
         &self,
-        _domain_separator: &Eip712Domain,
         receipts: Vec<ReceiptWithState<Checking, Rcpt>>,
     ) -> Result<CheckBatchResponse<Rcpt>, Eip712Error> {
         let (mut checking, mut failed) = (vec![], vec![]);
@@ -189,14 +186,13 @@ where
 {
     fn check_batch(
         &self,
-        domain_separator: &Eip712Domain,
         receipts: Vec<ReceiptWithState<Checking, Rcpt>>,
     ) -> Result<CheckBatchResponse<Rcpt>, Eip712Error> {
         let mut signatures = HashSet::new();
         let (mut checking, mut failed) = (vec![], vec![]);
 
         for received_receipt in receipts.into_iter() {
-            let unique_id = received_receipt.receipt.unique_id(domain_separator)?;
+            let unique_id = received_receipt.receipt.unique_id();
             if signatures.insert(unique_id) {
                 checking.push(received_receipt);
             } else {
@@ -277,10 +273,7 @@ mod tests {
         let signed_receipt_2 = create_signed_receipt_with_custom_value(15);
         let signed_receipt_copy = signed_receipt.clone();
         let receipts_batch = vec![signed_receipt, signed_receipt_2, signed_receipt_copy];
-        let domain_separator = create_test_domain_separator();
-        let (valid_receipts, invalid_receipts) = UniqueCheck
-            .check_batch(&domain_separator, receipts_batch)
-            .unwrap();
+        let (valid_receipts, invalid_receipts) = UniqueCheck.check_batch(receipts_batch).unwrap();
         assert_eq!(valid_receipts.len(), 2);
         assert_eq!(invalid_receipts.len(), 1);
     }
@@ -290,12 +283,10 @@ mod tests {
         let signed_receipt = create_signed_receipt_with_custom_value(10);
         let signed_receipt_2 = create_signed_receipt_with_custom_value(15);
 
-        let domain_separator = create_test_domain_separator();
-
         let receipts_batch = vec![signed_receipt.clone(), signed_receipt_2];
         let min_time_stamp = signed_receipt.receipt.message.timestamp_ns + 1;
         let (valid_receipts, invalid_receipts) = TimestampCheck(min_time_stamp)
-            .check_batch(&domain_separator, receipts_batch)
+            .check_batch(receipts_batch)
             .unwrap();
         assert_eq!(valid_receipts.len(), 1);
         assert_eq!(invalid_receipts.len(), 1);

tap_receipt/src/lib.rs

@@ -27,8 +27,8 @@ pub mod state;
 
 pub use error::ReceiptError;
 pub use received_receipt::ReceiptWithState;
-use tap_eip712_message::{Eip712Error, Eip712SignedMessage, MessageId};
-use thegraph_core::alloy::{dyn_abi::Eip712Domain, sol_types::SolStruct};
+use tap_eip712_message::{Eip712SignedMessage, SignatureBytes, SignatureBytesExt};
+use thegraph_core::alloy::sol_types::SolStruct;
 
 /// Result type for receipt
 pub type ReceiptResult<T> = Result<T, ReceiptError>;
@@ -45,7 +45,7 @@ pub trait WithValueAndTimestamp {
 /// Extension that allows UniqueCheck for any SolStruct receipt
 pub trait WithUniqueId {
     type Output: Eq + std::hash::Hash;
-    fn unique_id(&self, domain_separator: &Eip712Domain) -> Result<Self::Output, Eip712Error>;
+    fn unique_id(&self) -> Self::Output;
 }
 
 impl<T> WithValueAndTimestamp for Eip712SignedMessage<T>
@@ -65,9 +65,9 @@ impl<T> WithUniqueId for Eip712SignedMessage<T>
 where
     T: SolStruct,
 {
-    type Output = MessageId;
+    type Output = SignatureBytes;
 
-    fn unique_id(&self, domain_separator: &Eip712Domain) -> Result<Self::Output, Eip712Error> {
-        self.unique_hash(domain_separator)
+    fn unique_id(&self) -> Self::Output {
+        self.signature.normalized_s().get_signature_bytes()
     }
 }