[sui framework] eliminate polymorphic delete in Transfer::delete_chil…

…d_object

- Get rid of `Transfer::delete_child_object_internal`, which allowed deletion of a Move object without the `drop` ability (bad)
- Adjust `Transfer::delete_child_object` to take the `VersionedID` of the child object rather than the object itself. This allows a Move programmer to delete a child object and `ChildRef` in the same transaction, but without exposing a polymorphic delete.
- Refactored test of this function. This was the only call site
- Changed the dynamic checks for dangling reference detection in the adapter. This is the biggest change here--please review carefully. The dynamic check previously complained if a child was deleted in the same tx as its parent, but that is too strict for the new implementation of `Transfer::delete_child_object`.

crates/sui-core/src/unit_tests/data/object_owner/sources/ObjectOwner.move

@@ -74,7 +74,8 @@ module ObjectOwner::ObjectOwner {
         let Parent { id, child: child_ref_opt } = parent;
         let child_ref = Option::extract(&mut child_ref_opt);
         Option::destroy_none(child_ref_opt);
-        Transfer::delete_child_object(child, child_ref);
+        let Child { id: child_id } = child;
+        Transfer::delete_child_object(child_id, child_ref);
         ID::delete(id);
     }
 }

crates/sui-core/src/unit_tests/move_integration_tests.rs

@@ -99,7 +99,6 @@ async fn test_object_wrapping_unwrapping() {
     assert_eq!(parent_object_ref.1, OBJECT_START_VERSION);
 
     // Extract the child out of the parent.
-    println!("before this call");
     let effects = call_move(
         &authority,
         &gas,

crates/sui-framework/sources/Transfer.move

@@ -115,22 +115,20 @@ module Sui::Transfer {
     /// Transfer a child object to an account address. This is one of the two ways that can
     /// consume a ChildRef. No new ChildRef will be created, as the object is no longer
     /// owned by an object.
-    // TODO: Figure out a way to make it easier to destroy a child object in one call.
-    // Currently one has to first transfer it to an address, and then delete it.
     public fun transfer_child_to_address<T: key>(child: T, child_ref: ChildRef<T>, recipient: address) {
         let ChildRef { child_id } = child_ref;
         assert!(&child_id == ID::id(&child), EChildIDMismatch);
         transfer(child, recipient)
     }
 
-    /// Delete the child object along with a ownership reference that shows this object
-    /// is owned by another object. Deleting both the child object and the reference
-    /// is safe because the ownership will also be destroyed, and hence there won't
-    /// be dangling reference to the child object through ownership.
-    public fun delete_child_object<T: key>(child: T, child_ref: ChildRef<T>) {
-        let ChildRef { child_id } = child_ref;
-        assert!(&child_id == ID::id(&child), EChildIDMismatch);
-        delete_child_object_internal(child);
+    /// Delete `child_ref`, which must point at `child_id`.
+    /// This is the second way to consume a `ChildRef`.
+    /// Passing ownership of `child_id` to this function implies that the child object
+    /// has been unpacked, so it is now safe to delete `child_ref`.
+    public fun delete_child_object<T: key>(child_id: VersionedID, child_ref: ChildRef<T>) {
+        let ChildRef { child_id: child_ref_id } = child_ref;
+        assert!(&child_ref_id == ID::inner(&child_id), EChildIDMismatch);
+        delete_child_object_internal(ID::id_address(&child_ref_id), child_id)
     }
 
     /// Freeze `obj`. After freezing `obj` becomes immutable and can no
@@ -150,5 +148,6 @@ module Sui::Transfer {
 
     native fun transfer_internal<T: key>(obj: T, recipient: address, to_object: bool);
 
-    native fun delete_child_object_internal<T: key>(child: T);
+    // delete `child_id`, emit a system `DeleteChildObject(child)` event
+    native fun delete_child_object_internal(child: address, child_id: VersionedID);
 }

crates/sui-framework/src/natives/transfer.rs

@@ -1,7 +1,7 @@
 // Copyright (c) 2022, Mysten Labs, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
-use crate::{natives::get_object_id, EventType};
+use crate::EventType;
 use move_binary_format::errors::PartialVMResult;
 use move_core_types::account_address::AccountAddress;
 use move_vm_runtime::native_functions::NativeContext;
@@ -100,12 +100,13 @@ pub fn delete_child_object_internal(
     ty_args: Vec<Type>,
     mut args: VecDeque<Value>,
 ) -> PartialVMResult<NativeResult> {
-    debug_assert!(ty_args.len() == 1);
-    debug_assert!(args.len() == 1);
+    debug_assert!(ty_args.is_empty());
+    // first args is an object ID that we will emit in a DeleteChildObject event
+    // second arg is VersionedID that we want to ignore
+    debug_assert!(args.len() == 2);
 
-    let obj = args.pop_back().unwrap();
+    let obj_id = args.pop_front().unwrap();
     let event_type = EventType::DeleteChildObject;
-    let obj_id = get_object_id(obj)?;
     // TODO: Decide the cost.
     let cost = native_gas(context.cost_table(), NativeCostIndex::EMIT_EVENT, 1);
     if context.save_event(vec![], event_type as u64, Type::Address, obj_id)? {

crates/sui-verifier/src/id_leak_verifier.rs

@@ -163,18 +163,31 @@ impl<'a> TransferFunctions for IDLeakAnalysis<'a> {
 
 impl<'a> AbstractInterpreter for IDLeakAnalysis<'a> {}
 
-/// Sui::ID::delete function is allowed to take an ID by value.
+/// Certain Sui Framework functions can safely take a `VersionedID` by value
 fn is_call_safe_to_leak(verifier: &IDLeakAnalysis, function_handle: &FunctionHandle) -> bool {
     let m = verifier
         .binary_view
         .module_handle_at(function_handle.module);
-    verifier.binary_view.address_identifier_at(m.address) == &SUI_FRAMEWORK_ADDRESS
-        && verifier.binary_view.identifier_at(m.name).as_str() == "ID"
+    let is_framework =
+        verifier.binary_view.address_identifier_at(m.address) == &SUI_FRAMEWORK_ADDRESS;
+    if !is_framework {
+        return false;
+    }
+
+    // Sui::ID::delete
+    (verifier.binary_view.identifier_at(m.name).as_str() == "ID"
         && verifier
             .binary_view
             .identifier_at(function_handle.name)
             .as_str()
-            == "delete"
+            == "delete") ||
+    // Sui::Transfer::delete_child_object
+    (verifier.binary_view.identifier_at(m.name).as_str() == "Transfer"
+            && verifier
+                .binary_view
+                .identifier_at(function_handle.name)
+                .as_str()
+                == "delete_child_object")
 }
 
 fn call(verifier: &mut IDLeakAnalysis, function_handle: &FunctionHandle) -> SuiResult {