From 6a6ff5f2197239c67e2a022f4df0c1412b2db697 Mon Sep 17 00:00:00 2001 From: Lukasz Rubaszewski <117115317+lrubasze@users.noreply.github.com> Date: Tue, 17 Dec 2024 14:22:54 +0100 Subject: [PATCH 1/4] Support TakeNonFungibles in vault ops --- .../system_modules/execution_trace/module.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/radix-engine/src/system/system_modules/execution_trace/module.rs b/radix-engine/src/system/system_modules/execution_trace/module.rs index eb93555bff..105546dfa8 100644 --- a/radix-engine/src/system/system_modules/execution_trace/module.rs +++ b/radix-engine/src/system/system_modules/execution_trace/module.rs @@ -93,6 +93,7 @@ impl From<&BucketSnapshot> for ResourceSpecifier { pub enum VaultOp { Put(ResourceAddress, Decimal), // TODO: add non-fungible support Take(ResourceAddress, Decimal), + TakeNonFungibles(ResourceAddress, Decimal), TakeAdvanced(ResourceAddress, Decimal), Recall(ResourceAddress, Decimal), LockFee(Decimal, bool), @@ -725,7 +726,10 @@ impl ExecutionTraceModule { Actor::Method(actor @ MethodActor { node_id, ident, .. }) => { if VaultUtil::is_vault_blueprint(&actor.get_blueprint_id()) { match ident.as_str() { - VAULT_TAKE_IDENT | VAULT_TAKE_ADVANCED_IDENT | VAULT_RECALL_IDENT => { + VAULT_TAKE_IDENT + | VAULT_TAKE_ADVANCED_IDENT + | VAULT_RECALL_IDENT + | NON_FUNGIBLE_VAULT_TAKE_NON_FUNGIBLES_IDENT => { for (_, resource) in &resource_summary.buckets { let op = if ident == VAULT_TAKE_IDENT { VaultOp::Take(resource.resource_address(), resource.amount()) @@ -734,6 +738,11 @@ impl ExecutionTraceModule { resource.resource_address(), resource.amount(), ) + } else if ident == NON_FUNGIBLE_VAULT_TAKE_NON_FUNGIBLES_IDENT { + VaultOp::TakeNonFungibles( + resource.resource_address(), + resource.amount(), + ) } else if ident == VAULT_RECALL_IDENT { VaultOp::Recall(resource.resource_address(), resource.amount()) } else { @@ -756,8 +765,8 @@ impl ExecutionTraceModule { | FUNGIBLE_VAULT_LOCK_FUNGIBLE_AMOUNT_IDENT | FUNGIBLE_VAULT_UNLOCK_FUNGIBLE_AMOUNT_IDENT | FUNGIBLE_VAULT_CREATE_PROOF_OF_AMOUNT_IDENT => { /* no-op */ } - NON_FUNGIBLE_VAULT_TAKE_NON_FUNGIBLES_IDENT - | NON_FUNGIBLE_VAULT_GET_NON_FUNGIBLE_LOCAL_IDS_IDENT + + NON_FUNGIBLE_VAULT_GET_NON_FUNGIBLE_LOCAL_IDS_IDENT | NON_FUNGIBLE_VAULT_CONTAINS_NON_FUNGIBLE_IDENT | NON_FUNGIBLE_VAULT_RECALL_NON_FUNGIBLES_IDENT | NON_FUNGIBLE_VAULT_CREATE_PROOF_OF_NON_FUNGIBLES_IDENT @@ -904,6 +913,7 @@ pub fn calculate_resource_changes( } VaultOp::Take(resource_address, amount) | VaultOp::TakeAdvanced(resource_address, amount) + | VaultOp::TakeNonFungibles(resource_address, amount) | VaultOp::Recall(resource_address, amount) => { let entry = &mut vault_changes .entry(instruction_index) From 63b6283a5aebb8464a8f0d4b7540e7a3ee2b1149 Mon Sep 17 00:00:00 2001 From: Lukasz Rubaszewski <117115317+lrubasze@users.noreply.github.com> Date: Wed, 18 Dec 2024 12:39:31 +0100 Subject: [PATCH 2/4] Unsupport Recall from resource_changes in ExecutionTrace With `recall` it is not possible to determine the account the resources are recalled from, because recall operates with internal vault. --- .../system/system_modules/execution_trace/module.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/radix-engine/src/system/system_modules/execution_trace/module.rs b/radix-engine/src/system/system_modules/execution_trace/module.rs index 105546dfa8..fd3bfeea48 100644 --- a/radix-engine/src/system/system_modules/execution_trace/module.rs +++ b/radix-engine/src/system/system_modules/execution_trace/module.rs @@ -95,7 +95,6 @@ pub enum VaultOp { Take(ResourceAddress, Decimal), TakeNonFungibles(ResourceAddress, Decimal), TakeAdvanced(ResourceAddress, Decimal), - Recall(ResourceAddress, Decimal), LockFee(Decimal, bool), } @@ -728,7 +727,6 @@ impl ExecutionTraceModule { match ident.as_str() { VAULT_TAKE_IDENT | VAULT_TAKE_ADVANCED_IDENT - | VAULT_RECALL_IDENT | NON_FUNGIBLE_VAULT_TAKE_NON_FUNGIBLES_IDENT => { for (_, resource) in &resource_summary.buckets { let op = if ident == VAULT_TAKE_IDENT { @@ -743,8 +741,6 @@ impl ExecutionTraceModule { resource.resource_address(), resource.amount(), ) - } else if ident == VAULT_RECALL_IDENT { - VaultOp::Recall(resource.resource_address(), resource.amount()) } else { panic!("Unhandled vault method") }; @@ -760,7 +756,8 @@ impl ExecutionTraceModule { | VAULT_GET_AMOUNT_IDENT | VAULT_FREEZE_IDENT | VAULT_UNFREEZE_IDENT - | VAULT_BURN_IDENT => { /* no-op */ } + | VAULT_BURN_IDENT + | VAULT_RECALL_IDENT => { /* no-op */ } FUNGIBLE_VAULT_LOCK_FEE_IDENT | FUNGIBLE_VAULT_LOCK_FUNGIBLE_AMOUNT_IDENT | FUNGIBLE_VAULT_UNLOCK_FUNGIBLE_AMOUNT_IDENT @@ -913,8 +910,7 @@ pub fn calculate_resource_changes( } VaultOp::Take(resource_address, amount) | VaultOp::TakeAdvanced(resource_address, amount) - | VaultOp::TakeNonFungibles(resource_address, amount) - | VaultOp::Recall(resource_address, amount) => { + | VaultOp::TakeNonFungibles(resource_address, amount) => { let entry = &mut vault_changes .entry(instruction_index) .or_default() From 174b2202305ac1d12499b279c025df70fc77e90e Mon Sep 17 00:00:00 2001 From: Lukasz Rubaszewski <117115317+lrubasze@users.noreply.github.com> Date: Wed, 18 Dec 2024 12:43:20 +0100 Subject: [PATCH 3/4] Test execution tracing for non-fungible resources transfers --- .../execution_trace/src/execution_trace.rs | 51 ++++- .../tests/application/execution_trace.rs | 202 ++++++++++++------ 2 files changed, 192 insertions(+), 61 deletions(-) diff --git a/radix-engine-tests/assets/blueprints/execution_trace/src/execution_trace.rs b/radix-engine-tests/assets/blueprints/execution_trace/src/execution_trace.rs index 510f3e40bd..67ca0a0818 100644 --- a/radix-engine-tests/assets/blueprints/execution_trace/src/execution_trace.rs +++ b/radix-engine-tests/assets/blueprints/execution_trace/src/execution_trace.rs @@ -1,5 +1,8 @@ use scrypto::prelude::*; +#[derive(ScryptoSbor, NonFungibleData)] +pub struct DummyData {} + #[blueprint] mod execution_trace_test { struct ExecutionTraceBp { @@ -7,7 +10,7 @@ mod execution_trace_test { } impl ExecutionTraceBp { - pub fn transfer_resource_between_two_components( + pub fn transfer_fungible_resource_between_two_components( amount: u8, use_take_advanced: bool, ) -> ( @@ -45,6 +48,41 @@ mod execution_trace_test { (resource_address, source_component, target_component) } + pub fn transfer_non_fungible_resource_between_two_components( + amount: u8, + ) -> ( + ResourceAddress, + Global, + Global, + ) { + let mut entries = vec![]; + for i in 1..amount as u64 + 1 { + entries.push((i.into(), DummyData {})); + } + let bucket = ResourceBuilder::new_integer_non_fungible::(OwnerRole::None) + .mint_initial_supply(entries); + let resource_address = bucket.resource_address(); + + let source_component = ExecutionTraceBp { + vault: Vault::with_bucket(bucket.into()), + } + .instantiate() + .prepare_to_globalize(OwnerRole::None) + .globalize(); + + let target_component = ExecutionTraceBp { + vault: Vault::new(resource_address), + } + .instantiate() + .prepare_to_globalize(OwnerRole::None) + .globalize(); + + let transfer_bucket: Bucket = source_component.take_non_fungibles(amount); + target_component.put(transfer_bucket); + + (resource_address, source_component, target_component) + } + pub fn take(&mut self, amount: u8) -> Bucket { self.vault.take(amount) } @@ -53,6 +91,16 @@ mod execution_trace_test { self.vault.take_advanced(amount, WithdrawStrategy::Exact) } + pub fn take_non_fungibles(&mut self, amount: u8) -> Bucket { + let amount = amount as u64; + let mut ids = indexset![]; + for i in 1..amount + 1 { + ids.insert(NonFungibleLocalId::integer(i)); + } + + self.vault.as_non_fungible().take_non_fungibles(&ids).into() + } + pub fn put(&mut self, b: Bucket) { self.vault.put(b) } @@ -70,3 +118,4 @@ mod execution_trace_test { } } } + diff --git a/radix-engine-tests/tests/application/execution_trace.rs b/radix-engine-tests/tests/application/execution_trace.rs index dc3c964530..4d4ed1dc0d 100644 --- a/radix-engine-tests/tests/application/execution_trace.rs +++ b/radix-engine-tests/tests/application/execution_trace.rs @@ -7,12 +7,12 @@ use radix_transactions::{model::PreviewFlags, validation::TransactionValidator}; use scrypto_test::prelude::*; #[test] -fn test_trace_resource_transfers_using_take() { +fn test_trace_fungible_resource_transfers_within_blueprint_using_take() { run_resource_transfers_trace_test(false); } #[test] -fn test_trace_resource_transfers_using_take_advanced() { +fn test_trace_fungible_resource_transfers_within_blueprint_using_take_advanced() { run_resource_transfers_trace_test(true); } @@ -29,7 +29,7 @@ fn run_resource_transfers_trace_test(use_take_advanced: bool) { .call_function( package_address, "ExecutionTraceBp", - "transfer_resource_between_two_components", + "transfer_fungible_resource_between_two_components", manifest_args!(transfer_amount, use_take_advanced), ) .build(); @@ -47,74 +47,32 @@ fn run_resource_transfers_trace_test(use_take_advanced: bool) { ComponentAddress, ) = receipt.expect_commit(true).output(1); + let resource_changes = &receipt + .expect_commit_success() + .execution_trace + .as_ref() + .unwrap() + .resource_changes; + /* There should be three resource changes: withdrawal from the source vault, deposit to the target vault and withdrawal for the fee */ - println!( - "{:?}", - receipt - .expect_commit_success() - .execution_trace - .as_ref() - .unwrap() - .resource_changes - ); - assert_eq!( - 2, - receipt - .expect_commit_success() - .execution_trace - .as_ref() - .unwrap() - .resource_changes - .len() - ); // Two instructions - assert_eq!( - 1, - receipt - .expect_commit_success() - .execution_trace - .as_ref() - .unwrap() - .resource_changes - .get(&0) - .unwrap() - .len() - ); // One resource change in the first instruction (lock fee) - assert_eq!( - 2, - receipt - .expect_commit_success() - .execution_trace - .as_ref() - .unwrap() - .resource_changes - .get(&1) - .unwrap() - .len() - ); // One resource change in the first instruction (lock fee) + println!("{:?}", resource_changes); + assert_eq!(2, resource_changes.len()); // Two instructions + assert_eq!(1, resource_changes.get(&0).unwrap().len()); // One resource change in the first instruction (lock fee) + assert_eq!(2, resource_changes.get(&1).unwrap().len()); // Two resource changes in the second instruction (transfer_fungible_resource_between_two_components) let fee_summary = receipt.fee_summary.clone(); let total_fee_paid = fee_summary.total_cost(); // Source vault withdrawal - assert!(receipt - .expect_commit_success() - .execution_trace - .as_ref() - .unwrap() - .resource_changes + assert!(resource_changes .iter() .flat_map(|(_, rc)| rc) .any(|r| r.node_id == source_component.into() && r.amount == Decimal::from(transfer_amount).checked_neg().unwrap())); // Target vault deposit - assert!(receipt - .expect_commit_success() - .execution_trace - .as_ref() - .unwrap() - .resource_changes + assert!(resource_changes .iter() .flat_map(|(_, rc)| rc) .any( @@ -122,12 +80,79 @@ fn run_resource_transfers_trace_test(use_take_advanced: bool) { )); // Fee withdrawal - assert!(receipt + assert!(resource_changes + .iter() + .flat_map(|(_, rc)| rc) + .any(|r| r.node_id == account.into() + && r.amount == Decimal::from(total_fee_paid).checked_neg().unwrap())); +} + +#[test] +fn test_trace_non_fungible_resource_transfers_within_blueprint() { + // Arrange + let mut ledger = LedgerSimulatorBuilder::new().build(); + let (public_key, _, account) = ledger.new_allocated_account(); + let package_address = ledger.publish_package_simple(PackageLoader::get("execution_trace")); + let transfer_amount = 10u8; + + // Act + let manifest = ManifestBuilder::new() + .lock_fee(account, 500) + .call_function( + package_address, + "ExecutionTraceBp", + "transfer_non_fungible_resource_between_two_components", + manifest_args!(transfer_amount), + ) + .build(); + let receipt = ledger.preview_manifest( + manifest, + vec![public_key.clone().into()], + 0, + PreviewFlags::default(), + ); + + // Assert + let (_resource_address, source_component, target_component): ( + ResourceAddress, + ComponentAddress, + ComponentAddress, + ) = receipt.expect_commit(true).output(1); + + let resource_changes = &receipt .expect_commit_success() .execution_trace .as_ref() .unwrap() - .resource_changes + .resource_changes; + + /* There should be three resource changes: withdrawal from the source vault, + deposit to the target vault and withdrawal for the fee */ + println!("{:?}", resource_changes); + assert_eq!(2, resource_changes.len()); // Two instructions + assert_eq!(1, resource_changes.get(&0).unwrap().len()); // One resource change in the first instruction (lock fee) + assert_eq!(2, resource_changes.get(&1).unwrap().len()); // Two resource changes in the second instruction (transfer_non_fungible_resource_between_two_components) + + let fee_summary = receipt.fee_summary.clone(); + let total_fee_paid = fee_summary.total_cost(); + + // Source vault withdrawal + assert!(resource_changes + .iter() + .flat_map(|(_, rc)| rc) + .any(|r| r.node_id == source_component.into() + && r.amount == Decimal::from(transfer_amount).checked_neg().unwrap())); + + // Target vault deposit + assert!(resource_changes + .iter() + .flat_map(|(_, rc)| rc) + .any( + |r| r.node_id == target_component.into() && r.amount == Decimal::from(transfer_amount) + )); + + // Fee withdrawal + assert!(resource_changes .iter() .flat_map(|(_, rc)| rc) .any(|r| r.node_id == account.into() @@ -1047,3 +1072,60 @@ fn test_execution_trace_for_transaction_v2() { )"#; assert_eq!(format!("{:#?}", trace), expected_trace); } + +#[test] +fn test_trace_non_fungible_resource_transfers_using_manifest() { + // Arrange + let mut ledger = LedgerSimulatorBuilder::new().build(); + + let (from_public_key, _from_private_key, from_address) = ledger.new_allocated_account(); + let (_to_public_key, _to_private_key, to_address) = ledger.new_allocated_account(); + + let nf_resource_address = ledger.create_non_fungible_resource_advanced( + NonFungibleResourceRoles::default(), + from_address, + 4, + ); + let nf_global_id = + NonFungibleGlobalId::new(nf_resource_address, NonFungibleLocalId::integer(1)); + + let manifest = ManifestBuilder::new() + .lock_fee(from_address, 100) + .withdraw_non_fungible_from_account(from_address, nf_global_id) + .take_all_from_worktop(nf_resource_address, "nf_bucket") + .try_deposit_or_abort(to_address, None, "nf_bucket") + .build(); + + let receipt = ledger.preview_manifest( + manifest, + vec![from_public_key.clone().into()], + 0, + PreviewFlags::default(), + ); + let fee_summary = receipt.fee_summary.clone(); + let total_fee_paid = fee_summary.total_cost(); + + // Act & Assert: Execute the preview, followed by a normal execution. + let resource_changes = &receipt + .expect_commit_success() + .execution_trace + .as_ref() + .unwrap() + .resource_changes; + + println!("{:?}", resource_changes); + assert!(resource_changes + .iter() + .flat_map(|(_, rc)| rc) + .any(|r| r.node_id == from_address.into() && r.amount == -total_fee_paid)); + + assert!(resource_changes + .iter() + .flat_map(|(_, rc)| rc) + .any(|r| r.node_id == from_address.into() && r.amount == dec!(-1))); + + assert!(resource_changes + .iter() + .flat_map(|(_, rc)| rc) + .any(|r| r.node_id == to_address.into() && r.amount == dec!(1))); +} From a4629ce1da311cf96aef0701431b2f1134e18f6a Mon Sep 17 00:00:00 2001 From: Lukasz Rubaszewski <117115317+lrubasze@users.noreply.github.com> Date: Wed, 18 Dec 2024 12:48:45 +0100 Subject: [PATCH 4/4] Some comments --- .../assets/blueprints/execution_trace/src/execution_trace.rs | 1 - .../src/system/system_modules/execution_trace/module.rs | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/radix-engine-tests/assets/blueprints/execution_trace/src/execution_trace.rs b/radix-engine-tests/assets/blueprints/execution_trace/src/execution_trace.rs index 67ca0a0818..675ea4797f 100644 --- a/radix-engine-tests/assets/blueprints/execution_trace/src/execution_trace.rs +++ b/radix-engine-tests/assets/blueprints/execution_trace/src/execution_trace.rs @@ -118,4 +118,3 @@ mod execution_trace_test { } } } - diff --git a/radix-engine/src/system/system_modules/execution_trace/module.rs b/radix-engine/src/system/system_modules/execution_trace/module.rs index fd3bfeea48..fae75f10e3 100644 --- a/radix-engine/src/system/system_modules/execution_trace/module.rs +++ b/radix-engine/src/system/system_modules/execution_trace/module.rs @@ -91,8 +91,10 @@ impl From<&BucketSnapshot> for ResourceSpecifier { #[derive(Debug, Clone)] pub enum VaultOp { - Put(ResourceAddress, Decimal), // TODO: add non-fungible support + Put(ResourceAddress, Decimal), Take(ResourceAddress, Decimal), + // We intentionally disregard IDs and only use the amount for non-fungibles. + // This maintains backward compatibility for users of the `ResourceChange` struct. TakeNonFungibles(ResourceAddress, Decimal), TakeAdvanced(ResourceAddress, Decimal), LockFee(Decimal, bool),