Some cleanups around input object checking

sui_core/src/authority.rs

@@ -204,7 +204,7 @@ impl AuthorityState {
             let shared_ids: HashSet<_> = inputs
                 .iter()
                 .filter_map(|(kind, obj)| match kind {
-                    InputObjectKind::SharedMoveObject(..) if obj.owner.is_shared_mutable() => {
+                    InputObjectKind::MutSharedMoveObject(..) if obj.owner.is_shared_mutable() => {
                         Some((obj.id(), obj.version()))
                     }
                     _ => None,

sui_core/src/authority/temporary_store.rs

@@ -50,14 +50,16 @@ impl<S> AuthorityTemporaryStore<S> {
                 .iter()
                 .filter_map(|(kind, object)| match kind {
                     InputObjectKind::MovePackage(_) => None,
-                    InputObjectKind::OwnedMoveObject(object_ref) => {
+                    InputObjectKind::ImmOrOwnedMoveObject(object_ref) => {
                         if object.is_read_only() {
                             None
                         } else {
                             Some(*object_ref)
                         }
                     }
-                    InputObjectKind::SharedMoveObject(_) => Some(object.compute_object_reference()),
+                    InputObjectKind::MutSharedMoveObject(_) => {
+                        Some(object.compute_object_reference())
+                    }
                 })
                 .collect(),
             written: BTreeMap::new(),

sui_core/src/transaction_input_checker.rs

@@ -75,7 +75,7 @@ pub fn check_locks(
         return Err(SuiError::LockErrors { errors });
     }
     fp_ensure!(!all_objects.is_empty(), SuiError::ObjectInputArityViolation);
-    check_gas_requirement(transaction, &all_objects)?;
+    check_tx_requirement(transaction, &all_objects)?;
     Ok(all_objects)
 }
 
@@ -84,14 +84,14 @@ pub fn filter_owned_objects(all_objects: Vec<(InputObjectKind, Object)>) -> Vec<
         .into_iter()
         .filter_map(|(object_kind, object)| match object_kind {
             InputObjectKind::MovePackage(_) => None,
-            InputObjectKind::OwnedMoveObject(object_ref) => {
+            InputObjectKind::ImmOrOwnedMoveObject(object_ref) => {
                 if object.is_read_only() {
                     None
                 } else {
                     Some(object_ref)
                 }
             }
-            InputObjectKind::SharedMoveObject(..) => None,
+            InputObjectKind::MutSharedMoveObject(..) => None,
         })
         .collect();
 
@@ -120,7 +120,11 @@ fn check_one_lock(
                 }
             );
         }
-        InputObjectKind::OwnedMoveObject((object_id, sequence_number, object_digest)) => {
+        InputObjectKind::ImmOrOwnedMoveObject((object_id, sequence_number, object_digest)) => {
+            fp_ensure!(
+                !object.is_package(),
+                SuiError::MovePackageAsObject { object_id }
+            );
             fp_ensure!(
                 sequence_number <= SequenceNumber::MAX,
                 SuiError::InvalidSequenceNumber
@@ -154,7 +158,7 @@ fn check_one_lock(
                     fp_ensure!(
                         transaction.sender_address() == owner,
                         SuiError::IncorrectSigner {
-                            error: format!("Object {:?} is owned by account address {:?}, but signer address is {:?}", object.id(), owner, transaction.sender_address()),
+                            error: format!("Object {:?} is owned by account address {:?}, but signer address is {:?}", object_id, owner, transaction.sender_address()),
                         }
                     );
                 }
@@ -178,7 +182,7 @@ fn check_one_lock(
                 }
             };
         }
-        InputObjectKind::SharedMoveObject(..) => {
+        InputObjectKind::MutSharedMoveObject(..) => {
             // When someone locks an object as shared it must be shared already.
             fp_ensure!(object.is_shared(), SuiError::NotSharedObjectError);
         }
@@ -196,7 +200,7 @@ fn check_one_lock(
 ///   This can help reduce DDos attacks.
 /// 3. Check that the objects used in transfers are mutable. We put the check here
 ///   because this is the most convenient spot to check.
-fn check_gas_requirement(
+fn check_tx_requirement(
     transaction: &Transaction,
     input_objects: &[(InputObjectKind, Object)],
 ) -> SuiResult {
@@ -207,10 +211,7 @@ fn check_gas_requirement(
             SingleTransactionKind::Transfer(_) => {
                 // Index access safe because the inputs were constructed in order.
                 let transfer_object = &input_objects[idx].1;
-                fp_ensure!(
-                    !transfer_object.is_read_only(),
-                    SuiError::TransferImmutableError
-                );
+                transfer_object.is_transfer_eligible()?;
                 // TODO: Make Transfer transaction to also contain gas_budget.
                 // By @gdanezis: Now his is the only part of this function that requires
                 // an input object besides the gas object. It would be a major win if we
@@ -237,5 +238,11 @@ fn check_gas_requirement(
     }
     // The last element in the inputs is always gas object.
     let gas_object = &input_objects.last().unwrap().1;
+    fp_ensure!(
+        !gas_object.is_shared(),
+        SuiError::InsufficientGas {
+            error: format!("Gas object cannot be shared: {:?}", gas_object.id())
+        }
+    );
     gas::check_gas_balance(gas_object, total_cost)
 }

sui_core/src/unit_tests/authority_tests.rs

@@ -271,7 +271,7 @@ async fn test_handle_transfer_transaction_ok() {
 }
 
 #[tokio::test]
-async fn test_transfer_immutable() {
+async fn test_transfer_package() {
     let (sender, sender_key) = get_key_pair();
     let recipient = dbg_addr(2);
     let object_id = ObjectID::random();
@@ -294,7 +294,39 @@ async fn test_transfer_immutable() {
     let result = authority_state
         .handle_transaction(transfer_transaction.clone())
         .await;
-    assert_eq!(result.unwrap_err(), SuiError::TransferImmutableError);
+    assert!(matches!(result.unwrap_err(), SuiError::LockErrors { .. }));
+}
+
+// This test attempts to use an immutable gas object to pay for gas.
+// We expect it to fail early during transaction handle phase.
+#[tokio::test]
+async fn test_immutable_gas() {
+    let (sender, sender_key) = get_key_pair();
+    let recipient = dbg_addr(2);
+    let mut_object_id = ObjectID::random();
+    let authority_state = init_state_with_ids(vec![(sender, mut_object_id)]).await;
+    let imm_object_id = ObjectID::random();
+    let imm_object = Object::immutable_with_id_for_testing(imm_object_id);
+    authority_state.insert_object(imm_object.clone()).await;
+    let mut_object = authority_state
+        .get_object(&mut_object_id)
+        .await
+        .unwrap()
+        .unwrap();
+    let transfer_transaction = init_transfer_transaction(
+        sender,
+        &sender_key,
+        recipient,
+        mut_object.compute_object_reference(),
+        imm_object.compute_object_reference(),
+    );
+    let result = authority_state
+        .handle_transaction(transfer_transaction.clone())
+        .await;
+    assert!(matches!(
+        result.unwrap_err(),
+        SuiError::InsufficientGas { .. }
+    ));
 }
 
 #[tokio::test]