From 380dd98fe1880e2c2f5da97db07f8c0faddf8695 Mon Sep 17 00:00:00 2001
From: Xun Li <lxfind@gmail.com>
Date: Fri, 18 Mar 2022 22:41:09 -0700
Subject: [PATCH] Improve a few error messages (#954)

---
 sui_core/src/authority.rs      | 10 ++++++++--
 sui_core/src/gateway_state.rs  |  1 +
 sui_core/tests/staged/sui.yaml |  6 +++++-
 sui_types/src/crypto.rs        | 15 +++++++++++----
 sui_types/src/error.rs         |  7 ++++---
 5 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/sui_core/src/authority.rs b/sui_core/src/authority.rs
index 11842bd09b8e5..f3598675a8db0 100644
--- a/sui_core/src/authority.rs
+++ b/sui_core/src/authority.rs
@@ -145,6 +145,7 @@ impl AuthorityState {
                     SuiError::UnexpectedSequenceNumber {
                         object_id,
                         expected_sequence: object.version(),
+                        given_sequence: sequence_number,
                     }
                 );
 
@@ -175,14 +176,18 @@ impl AuthorityState {
                         // Check the owner is the transaction sender.
                         fp_ensure!(
                             transaction.sender_address() == owner,
-                            SuiError::IncorrectSigner
+                            SuiError::IncorrectSigner {
+                                error: format!("Object {:?} is owned by account address {:?}, but signer address is {:?}", object.id(), owner, transaction.sender_address()),
+                            }
                         );
                     }
                     Owner::ObjectOwner(owner) => {
                         // Check that the object owner is another mutable object in the input.
                         fp_ensure!(
                             owned_object_authenticators.contains(&owner),
-                            SuiError::IncorrectSigner
+                            SuiError::IncorrectSigner {
+                                error: format!("Object {:?} is owned by object {:?}, which is not in the input", object.id(), owner),
+                            }
                         );
                     }
                     Owner::SharedMutable => {
@@ -420,6 +425,7 @@ impl AuthorityState {
                         Some(SuiError::UnexpectedSequenceNumber {
                             object_id: *object_id,
                             expected_sequence: shared_locks[object_id],
+                            given_sequence: *version,
                         })
                     } else {
                         None
diff --git a/sui_core/src/gateway_state.rs b/sui_core/src/gateway_state.rs
index ff269ad6ce2e0..bc30746171211 100644
--- a/sui_core/src/gateway_state.rs
+++ b/sui_core/src/gateway_state.rs
@@ -517,6 +517,7 @@ where
                 SuiError::UnexpectedSequenceNumber {
                     object_id,
                     expected_sequence: next_sequence_number,
+                    given_sequence: object_kind.version(),
                 }
                 .into()
             );
diff --git a/sui_core/tests/staged/sui.yaml b/sui_core/tests/staged/sui.yaml
index d2983b3e6624f..328b6883f38d6 100644
--- a/sui_core/tests/staged/sui.yaml
+++ b/sui_core/tests/staged/sui.yaml
@@ -439,7 +439,9 @@ SuiError:
         STRUCT:
           - error: STR
     10:
-      IncorrectSigner: UNIT
+      IncorrectSigner:
+        STRUCT:
+          - error: STR
     11:
       UnknownSigner: UNIT
     12:
@@ -451,6 +453,8 @@ SuiError:
               TYPENAME: ObjectID
           - expected_sequence:
               TYPENAME: SequenceNumber
+          - given_sequence:
+              TYPENAME: SequenceNumber
     14:
       ConflictingTransaction:
         STRUCT:
diff --git a/sui_types/src/crypto.rs b/sui_types/src/crypto.rs
index 189e0aa68e572..f717001c63d4b 100644
--- a/sui_types/src/crypto.rs
+++ b/sui_types/src/crypto.rs
@@ -29,7 +29,7 @@ impl KeyPair {
         })
     }
 
-    // TODO: eradicate uneeded uses (i.e. most)
+    // TODO: eradicate unneeded uses (i.e. most)
     /// Avoid implementing `clone` on secret keys to prevent mistakes.
     #[must_use]
     pub fn copy(&self) -> KeyPair {
@@ -216,7 +216,12 @@ impl Signature {
             .expect("byte lengths match");
         let received_addr = SuiAddress::from(&PublicKeyBytes(public_key_bytes));
         if received_addr != author {
-            return Err(SuiError::IncorrectSigner);
+            return Err(SuiError::IncorrectSigner {
+                error: format!(
+                    "Signature check failure. Author is {}, received address is {}",
+                    author, received_addr
+                ),
+            });
         }
 
         // is this a cryptographically correct public key?
@@ -263,7 +268,9 @@ impl Signature {
             .expect("byte lengths match");
         let received_addr = SuiAddress::from(&PublicKeyBytes(public_key_bytes));
         if received_addr != author {
-            return Err(SuiError::IncorrectSigner);
+            return Err(SuiError::IncorrectSigner {
+                error: format!("Signature get_verification_inputs() failure. Author is {}, received address is {}", author, received_addr)
+            });
         }
 
         // deserialize the signature
@@ -303,7 +310,7 @@ impl signature::Signature for AuthoritySignature {
 
 impl AuthoritySignature {
     /// Signs with the provided Signer
-    ///    
+    ///
     pub fn new<T>(value: &T, secret: &dyn signature::Signer<AuthoritySignature>) -> Self
     where
         T: Signable<Vec<u8>>,
diff --git a/sui_types/src/error.rs b/sui_types/src/error.rs
index 64fc19e5ee07a..b3bdf60845385 100644
--- a/sui_types/src/error.rs
+++ b/sui_types/src/error.rs
@@ -54,19 +54,20 @@ pub enum SuiError {
     // Signature verification
     #[error("Signature is not valid: {}", error)]
     InvalidSignature { error: String },
-    #[error("Value was not signed by the correct sender")]
-    IncorrectSigner,
+    #[error("Value was not signed by the correct sender: {}", error)]
+    IncorrectSigner { error: String },
     #[error("Value was not signed by a known authority")]
     UnknownSigner,
     // Certificate verification
     #[error("Signatures in a certificate must form a quorum")]
     CertificateRequiresQuorum,
     #[error(
-        "The given sequence number must match the next expected sequence ({expected_sequence:?}) number of the object ({object_id:?})"
+        "The given sequence number ({given_sequence:?}) must match the next expected sequence ({expected_sequence:?}) number of the object ({object_id:?})"
     )]
     UnexpectedSequenceNumber {
         object_id: ObjectID,
         expected_sequence: SequenceNumber,
+        given_sequence: SequenceNumber,
     },
     #[error("Conflicting transaction already received: {pending_transaction:?}")]
     ConflictingTransaction {