From f4559351dfe2f979e6ef7b0f9913c0f24aabf68c Mon Sep 17 00:00:00 2001 From: Omar Date: Thu, 7 Dec 2023 19:11:49 +0300 Subject: [PATCH 1/2] Disallow refund methods --- .../general_transaction_visitor.rs | 5 +- .../tests/general_transaction_visitor.rs | 113 ++++++++++++++++++ 2 files changed, 116 insertions(+), 2 deletions(-) diff --git a/crates/radix-engine-toolkit/src/instruction_visitor/visitors/transaction_type/general_transaction_visitor.rs b/crates/radix-engine-toolkit/src/instruction_visitor/visitors/transaction_type/general_transaction_visitor.rs index 6c428bd4..31ca382e 100644 --- a/crates/radix-engine-toolkit/src/instruction_visitor/visitors/transaction_type/general_transaction_visitor.rs +++ b/crates/radix-engine-toolkit/src/instruction_visitor/visitors/transaction_type/general_transaction_visitor.rs @@ -828,8 +828,6 @@ fn construct_fn_rules(entity_type: EntityType) -> FnRules { /* All deposit methods */ ACCOUNT_DEPOSIT_IDENT, ACCOUNT_DEPOSIT_BATCH_IDENT, - ACCOUNT_TRY_DEPOSIT_OR_REFUND_IDENT, - ACCOUNT_TRY_DEPOSIT_BATCH_OR_REFUND_IDENT, ACCOUNT_TRY_DEPOSIT_OR_ABORT_IDENT, ACCOUNT_TRY_DEPOSIT_BATCH_OR_ABORT_IDENT, /* All proof creation methods */ @@ -851,6 +849,9 @@ fn construct_fn_rules(entity_type: EntityType) -> FnRules { ACCOUNT_REMOVE_RESOURCE_PREFERENCE_IDENT, ACCOUNT_ADD_AUTHORIZED_DEPOSITOR, ACCOUNT_REMOVE_AUTHORIZED_DEPOSITOR, + /* Deposit or Refund */ + ACCOUNT_TRY_DEPOSIT_OR_REFUND_IDENT, + ACCOUNT_TRY_DEPOSIT_BATCH_OR_REFUND_IDENT, /* All fee locking methods */ ACCOUNT_LOCK_FEE_IDENT, ACCOUNT_LOCK_CONTINGENT_FEE_IDENT, diff --git a/crates/radix-engine-toolkit/tests/general_transaction_visitor.rs b/crates/radix-engine-toolkit/tests/general_transaction_visitor.rs index 5a3311cc..0eb5afd4 100644 --- a/crates/radix-engine-toolkit/tests/general_transaction_visitor.rs +++ b/crates/radix-engine-toolkit/tests/general_transaction_visitor.rs @@ -135,3 +135,116 @@ fn account_add_authorized_depositor_method_is_disallowed_in_general_transaction( // Assert assert!(visitor.output().is_none()) } + +#[test] +fn deposit_or_abort_is_valid_for_general_transaction_type() { + // Arrange + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); + let (public_key1, _, account1) = test_runner.new_account(true); + let (public_key2, _, account2) = test_runner.new_account(true); + + let manifest = ManifestBuilder::new() + .withdraw_from_account(account1, XRD, dec!("10")) + .take_from_worktop(XRD, dec!("10"), "bucket") + .try_deposit_or_abort(account2, None, "bucket") + .build(); + let receipt = test_runner.preview_manifest( + manifest.clone(), + vec![public_key1.into(), public_key2.into()], + 0, + PreviewFlags { + use_free_credit: true, + assume_all_signature_proofs: true, + skip_epoch_check: true, + }, + ); + receipt.expect_commit_success(); + + // Act + let mut visitor = GeneralTransactionTypeVisitor::new( + receipt + .expect_commit_success() + .execution_trace + .as_ref() + .unwrap(), + ); + traverse(&manifest.instructions, &mut [&mut visitor]).unwrap(); + + // Assert + assert!(visitor.output().is_some()); +} + +#[test] +fn deposit_or_refund_invalidates_general_transaction_type() { + // Arrange + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); + let (public_key1, _, account1) = test_runner.new_account(true); + let (public_key2, _, account2) = test_runner.new_account(true); + + let manifest = ManifestBuilder::new() + .withdraw_from_account(account1, XRD, dec!("10")) + .take_from_worktop(XRD, dec!("10"), "bucket") + .try_deposit_or_refund(account2, None, "bucket") + .build(); + let receipt = test_runner.preview_manifest( + manifest.clone(), + vec![public_key1.into(), public_key2.into()], + 0, + PreviewFlags { + use_free_credit: true, + assume_all_signature_proofs: true, + skip_epoch_check: true, + }, + ); + receipt.expect_commit_success(); + + // Act + let mut visitor = GeneralTransactionTypeVisitor::new( + receipt + .expect_commit_success() + .execution_trace + .as_ref() + .unwrap(), + ); + traverse(&manifest.instructions, &mut [&mut visitor]).unwrap(); + + // Assert + assert!(visitor.output().is_none()); +} + +#[test] +fn deposit_batch_or_refund_invalidates_general_transaction_type() { + // Arrange + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); + let (public_key1, _, account1) = test_runner.new_account(true); + let (public_key2, _, account2) = test_runner.new_account(true); + + let manifest = ManifestBuilder::new() + .withdraw_from_account(account1, XRD, dec!("10")) + .try_deposit_entire_worktop_or_refund(account2, None) + .build(); + let receipt = test_runner.preview_manifest( + manifest.clone(), + vec![public_key1.into(), public_key2.into()], + 0, + PreviewFlags { + use_free_credit: true, + assume_all_signature_proofs: true, + skip_epoch_check: true, + }, + ); + receipt.expect_commit_success(); + + // Act + let mut visitor = GeneralTransactionTypeVisitor::new( + receipt + .expect_commit_success() + .execution_trace + .as_ref() + .unwrap(), + ); + traverse(&manifest.instructions, &mut [&mut visitor]).unwrap(); + + // Assert + assert!(visitor.output().is_none()); +} From 6b7a108eb6f6d3ea45e83adfe53fcfe9d3510c01 Mon Sep 17 00:00:00 2001 From: Omar Date: Thu, 7 Dec 2023 19:12:05 +0300 Subject: [PATCH 2/2] Remove kernel trace from tests --- .../radix-engine-toolkit/tests/execution.rs | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/crates/radix-engine-toolkit/tests/execution.rs b/crates/radix-engine-toolkit/tests/execution.rs index 53ecc64b..d2503746 100644 --- a/crates/radix-engine-toolkit/tests/execution.rs +++ b/crates/radix-engine-toolkit/tests/execution.rs @@ -34,7 +34,7 @@ use radix_engine_toolkit::instruction_visitor::visitors::transaction_type::unsta #[test] fn simple_transfer_is_picked_up_as_a_simple_account_transfer_transaction() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (public_key1, _, account1) = test_runner.new_account(true); let (public_key2, _, account2) = test_runner.new_account(true); @@ -79,7 +79,7 @@ fn simple_transfer_is_picked_up_as_a_simple_account_transfer_transaction() { #[test] fn transfer_is_picked_up_as_an_account_transfer_transaction() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (public_key1, _, account1) = test_runner.new_account(true); let (public_key2, _, account2) = test_runner.new_account(true); let (public_key3, _, account3) = test_runner.new_account(true); @@ -131,7 +131,7 @@ fn transfer_is_picked_up_as_an_account_transfer_transaction() { #[test] fn complex_transfer_is_picked_up_as_an_general_transaction() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (public_key1, _, account1) = test_runner.new_account(true); let (public_key2, _, account2) = test_runner.new_account(true); let (public_key3, _, account3) = test_runner.new_account(true); @@ -267,7 +267,7 @@ fn general_transaction_handles_take_non_fungible_ids_from_worktop_correctly() { #[test] pub fn deposit_and_deposit_batch_of_nothing_should_not_result_in_an_error() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account) = test_runner.new_account(true); let manifest = ManifestBuilder::new().deposit_batch(account).build(); @@ -294,7 +294,7 @@ fn test_manifest_with_lock_fee( arguments: impl ResolvableArguments, ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (pk, _, account) = test_runner.new_account(true); let manifest = ManifestBuilder::new() @@ -347,7 +347,7 @@ fn manifest_with_a_lock_contingent_fee_should_not_be_conforming() { #[test] fn simple_stake_transaction_is_detected_by_the_stake_visitor() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit1, _) = new_registered_validator(&mut test_runner); @@ -382,7 +382,7 @@ fn simple_stake_transaction_is_detected_by_the_stake_visitor() { fn simple_stake_transaction_using_take_all_from_worktop_deposit_is_detected_by_the_stake_visitor( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit1, _) = new_registered_validator(&mut test_runner); @@ -418,7 +418,7 @@ fn simple_stake_transaction_using_take_all_from_worktop_deposit_is_detected_by_t fn stake_with_multi_withdraw_and_multi_deposits_is_detected_as_stake_by_stake_visitor( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit1, _) = @@ -472,7 +472,7 @@ fn stake_with_multi_withdraw_and_multi_deposits_is_detected_as_stake_by_stake_vi fn staking_from_one_account_to_multiple_validators_is_detected_as_a_stake_transaction( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit1, _) = new_registered_validator(&mut test_runner); @@ -520,7 +520,7 @@ fn staking_from_one_account_to_multiple_validators_is_detected_as_a_stake_transa #[test] fn staking_of_zero_xrd_is_considered_valid_by_the_stake_visitor() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit1, _) = new_registered_validator(&mut test_runner); @@ -555,7 +555,7 @@ fn staking_of_zero_xrd_is_considered_valid_by_the_stake_visitor() { fn staking_transaction_that_used_take_all_from_worktop_is_considered_valid_by_the_stake_visitor( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit1, _) = new_registered_validator(&mut test_runner); @@ -590,7 +590,7 @@ fn staking_transaction_that_used_take_all_from_worktop_is_considered_valid_by_th fn staking_transaction_that_used_take_all_from_worktop_is_considered_valid_by_the_stake_visitor2( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit1, _) = new_registered_validator(&mut test_runner); @@ -638,7 +638,7 @@ fn staking_transaction_that_used_take_all_from_worktop_is_considered_valid_by_th #[test] fn staking_but_not_using_all_withdrawn_xrd_invalidates_staking_transaction() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, _, _) = new_registered_validator(&mut test_runner); @@ -659,7 +659,7 @@ fn staking_but_not_using_all_withdrawn_xrd_invalidates_staking_transaction() { fn staking_and_withdrawing_from_one_account_and_depositing_into_another_invalidates_stake_transaction( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, account2) = test_runner.new_account(false); let (_, _, validator1, _, _) = new_registered_validator(&mut test_runner); @@ -680,7 +680,7 @@ fn staking_and_withdrawing_from_one_account_and_depositing_into_another_invalida #[test] fn simple_unstaking_is_recognized_by_unstaking_visitor() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit_resource, claim_nft_resource) = new_registered_validator(&mut test_runner); @@ -719,7 +719,7 @@ fn simple_unstaking_is_recognized_by_unstaking_visitor() { fn unstaking_with_take_from_worktop_by_amount_is_recognized_by_unstaking_visitor( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit_resource, claim_nft_resource) = new_registered_validator(&mut test_runner); @@ -758,7 +758,7 @@ fn unstaking_with_take_from_worktop_by_amount_is_recognized_by_unstaking_visitor fn unstaking_with_take_from_worktop_by_amount_of_claim_nft_is_recognized_by_unstaking_visitor( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit_resource, claim_nft_resource) = new_registered_validator(&mut test_runner); @@ -798,7 +798,7 @@ fn unstaking_with_take_from_worktop_by_amount_of_claim_nft_is_recognized_by_unst fn unstaking_with_take_all_from_worktop_of_claim_nft_is_recognized_by_unstaking_visitor( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit_resource, claim_nft_resource) = new_registered_validator(&mut test_runner); @@ -838,7 +838,7 @@ fn unstaking_with_take_all_from_worktop_of_claim_nft_is_recognized_by_unstaking_ fn unstaking_and_depositing_claim_nft_into_another_account_is_not_allowed_by_unstake_visitor( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, account2) = test_runner.new_account(false); let (_, _, validator1, stake_unit_resource, claim_nft_resource) = @@ -863,7 +863,7 @@ fn unstaking_and_depositing_claim_nft_into_another_account_is_not_allowed_by_uns fn unstaking_and_depositing_claim_nft_into_another_account_is_not_allowed_by_unstake_visitor2( ) { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, account2) = test_runner.new_account(false); let (_, _, validator1, stake_unit_resource, ..) = @@ -886,7 +886,7 @@ fn unstaking_and_depositing_claim_nft_into_another_account_is_not_allowed_by_uns #[test] fn multiple_unstakes_is_recognized_by_unstaking_visitor() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, stake_unit_resource, claim_nft_resource) = new_registered_validator(&mut test_runner); @@ -945,7 +945,7 @@ fn multiple_unstakes_is_recognized_by_unstaking_visitor() { #[test] fn simple_claim_transaction_can_be_caught_by_claim_visitor() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, _, claim_nft_resource) = new_registered_validator(&mut test_runner); @@ -991,7 +991,7 @@ fn simple_claim_transaction_can_be_caught_by_claim_visitor() { #[test] fn stake_claim_with_multiple_nfts_can_be_caught_by_claim_visitor() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, _, claim_nft_resource) = new_registered_validator(&mut test_runner); @@ -1059,7 +1059,7 @@ fn stake_claim_with_multiple_nfts_can_be_caught_by_claim_visitor() { #[test] fn simple_claim_transaction_can_be_caught_by_claim_visitor1() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, _, claim_nft_resource) = new_registered_validator(&mut test_runner); @@ -1105,7 +1105,7 @@ fn simple_claim_transaction_can_be_caught_by_claim_visitor1() { #[test] fn simple_claim_transaction_can_be_caught_by_claim_visitor2() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, _, claim_nft_resource) = new_registered_validator(&mut test_runner); @@ -1151,7 +1151,7 @@ fn simple_claim_transaction_can_be_caught_by_claim_visitor2() { #[test] fn simple_claim_transaction_can_be_caught_by_claim_visitor3() { // Arrange - let mut test_runner = TestRunnerBuilder::new().build(); + let mut test_runner = TestRunnerBuilder::new().without_trace().build(); let (_, _, account1) = test_runner.new_account(false); let (_, _, validator1, _, claim_nft_resource) = new_registered_validator(&mut test_runner);