resolve review comments

radixdlt · Dec 13, 2024 · e44b019 · e44b019
1 parent cabe7d6
commit e44b019
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 10 deletions.
diff --git a/crates/radix-engine-toolkit-uniffi/src/transaction_v1/manifest.rs b/crates/radix-engine-toolkit-uniffi/src/transaction_v1/manifest.rs
@@ -360,8 +360,10 @@ impl ToNative for AccountDefaultDepositRule {
 pub enum ReservedInstruction {
     AccountLockFee,
     AccountSecurify,
+    AccountLockOwnerKeysMetadataField,
     AccountUpdateOwnerKeysMetadataField,
     IdentitySecurify,
+    IdentityLockOwnerKeysMetadataField,
     IdentityUpdateOwnerKeysMetadataField,
     AccessControllerMethod,
 }
@@ -372,9 +374,15 @@ impl From<ReservedInstruction> for CoreReservedInstruction {
             ReservedInstruction::AccountUpdateOwnerKeysMetadataField => {
                 Self::AccountUpdateOwnerKeysMetadataField
             }
+            ReservedInstruction::AccountLockOwnerKeysMetadataField => {
+                Self::AccountLockOwnerKeysMetadataField
+            }
             ReservedInstruction::IdentityUpdateOwnerKeysMetadataField => {
                 Self::IdentityUpdateOwnerKeysMetadataField
             }
+            ReservedInstruction::IdentityLockOwnerKeysMetadataField => {
+                Self::IdentityLockOwnerKeysMetadataField
+            }
             ReservedInstruction::AccessControllerMethod => {
                 Self::AccessControllerMethod
             }
@@ -391,9 +399,15 @@ impl From<CoreReservedInstruction> for ReservedInstruction {
             CoreReservedInstruction::AccountUpdateOwnerKeysMetadataField => {
                 Self::AccountUpdateOwnerKeysMetadataField
             }
+            CoreReservedInstruction::AccountLockOwnerKeysMetadataField => {
+                Self::AccountLockOwnerKeysMetadataField
+            }
             CoreReservedInstruction::IdentityUpdateOwnerKeysMetadataField => {
                 Self::IdentityUpdateOwnerKeysMetadataField
             }
+            CoreReservedInstruction::IdentityLockOwnerKeysMetadataField => {
+                Self::IdentityLockOwnerKeysMetadataField
+            }
             CoreReservedInstruction::AccessControllerMethod => {
                 Self::AccessControllerMethod
             }

diff --git a/...s/radix-engine-toolkit/src/transaction_types/traverser/auxiliary/reserved_instructions.rs b/...s/radix-engine-toolkit/src/transaction_types/traverser/auxiliary/reserved_instructions.rs
@@ -96,6 +96,24 @@ impl StaticAnalysisCallback for ReservedInstructionsDetector {
                     {
                         self.reserved_instructions.insert(ReservedInstruction::IdentityUpdateOwnerKeysMetadataField);
                     }
+                } else if method_name == METADATA_LOCK_IDENT {
+                    // Attempt to decode the args as a metadata set call. If we
+                    // fail then we have not technically detected a violation of
+                    // the reserved instructions and we can just ignore this.
+                    let Some(MetadataLockInput { key, .. }) =
+                        manifest_encode(args)
+                            .ok()
+                            .and_then(|encoded| manifest_decode(&encoded).ok())
+                    else {
+                        return;
+                    };
+                    let is_owner_keys_metadata_key = key == "owner_keys";
+                    if is_account(address) && is_owner_keys_metadata_key {
+                        self.reserved_instructions.insert(ReservedInstruction::AccountLockOwnerKeysMetadataField);
+                    } else if is_identity(address) && is_owner_keys_metadata_key
+                    {
+                        self.reserved_instructions.insert(ReservedInstruction::IdentityLockOwnerKeysMetadataField);
+                    }
                 }
             }
             _ => {}

diff --git a/crates/radix-engine-toolkit/src/transaction_types/traverser/types/general_subintent.rs b/crates/radix-engine-toolkit/src/transaction_types/traverser/types/general_subintent.rs
@@ -24,13 +24,13 @@ use radix_engine_interface::blueprints::account::*;
 use crate::transaction_types::*;
 
 pub struct GeneralSubintentDetector {
-    is_valid: bool,
+    is_all_instructions_allowed: bool,
     is_yield_to_parent_present: bool,
 }
 
 impl GeneralSubintentDetector {
     pub fn is_valid(&self) -> bool {
-        self.is_valid && self.is_yield_to_parent_present
+        self.is_all_instructions_allowed && self.is_yield_to_parent_present
     }
 
     pub fn output(self) -> Option<()> {
@@ -39,17 +39,11 @@ impl GeneralSubintentDetector {
 }
 
 impl StaticAnalysisCallback for GeneralSubintentDetector {
-    fn on_finish(&mut self, instructions_count: usize) {
-        if instructions_count == 0 {
-            self.is_valid = false
-        }
-    }
-
     fn on_instruction(&mut self, instruction: &InstructionV2, _: usize) {
         // Control whether or not this is allowed or not based on:
         // 1. Whether the instruction is allowed.
         // 2. Whether the instruction contents are allowed.
-        self.is_valid &= match instruction {
+        self.is_all_instructions_allowed &= match instruction {
             /* Maybe Permitted - Need more info */
             InstructionV2::CallMethod(CallMethod {
                 address,
@@ -176,7 +170,7 @@ impl GeneralSubintentDetector {
 impl Default for GeneralSubintentDetector {
     fn default() -> Self {
         Self {
-            is_valid: true,
+            is_all_instructions_allowed: true,
             is_yield_to_parent_present: false,
         }
     }

diff --git a/crates/radix-engine-toolkit/src/transaction_types/types.rs b/crates/radix-engine-toolkit/src/transaction_types/types.rs
@@ -486,8 +486,10 @@ pub struct NewEntities {
 pub enum ReservedInstruction {
     AccountLockFee,
     AccountSecurify,
+    AccountLockOwnerKeysMetadataField,
     AccountUpdateOwnerKeysMetadataField,
     IdentitySecurify,
+    IdentityLockOwnerKeysMetadataField,
     IdentityUpdateOwnerKeysMetadataField,
     AccessControllerMethod,
 }

diff --git a/crates/radix-engine-toolkit/tests/transaction_types.rs b/crates/radix-engine-toolkit/tests/transaction_types.rs
@@ -2543,6 +2543,35 @@ fn subintent_manifest_of_transfer_is_a_general_subintent() {
     )
 }
 
+#[test]
+fn subintent_manifest_of_transfer_and_verify_parent_is_a_general_subintent() {
+    let account =
+        ComponentAddress::new_or_panic([EntityType::GlobalAccount as u8; 30]);
+    subintent_manifest_classification_test(
+        ManifestBuilder::new_subintent_v2()
+            .lock_fee(account, dec!(10))
+            .withdraw_from_account(account, XRD, 10)
+            .take_all_from_worktop(XRD, "bucket")
+            .try_deposit_or_abort(account, None, "bucket")
+            .verify_parent(rule!(allow_all))
+            .yield_to_parent(())
+            .build(),
+        true,
+    )
+}
+
+#[test]
+fn subintent_manifest_with_yield_to_child_is_not_a_general_subintent() {
+    subintent_manifest_classification_test(
+        ManifestBuilder::new_subintent_v2()
+            .use_child("example", SubintentHash(Hash([0; 32])))
+            .yield_to_child("example", ())
+            .yield_to_parent(())
+            .build(),
+        false,
+    )
+}
+
 #[test]
 fn subintent_manifest_of_transfer_with_metadata_update_is_not_a_general_subintent()
  {