Skip to content

Commit

Permalink
Merge pull request #83 from radixdlt/bugfix/deposit-or-refund
Browse files Browse the repository at this point in the history 
Disallow account `deposit*or_refund` methods
  • Loading branch information
@0xOmarA
0xOmarA committed Dec 7, 2023
2 parents 619563e + 6b7a108 commit a349239
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 28 deletions.
5 changes: 3 additions & 2 deletions ...-toolkit/src/instruction_visitor/visitors/transaction_type/general_transaction_visitor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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,
Expand Down
52 changes: 26 additions & 26 deletions crates/radix-engine-toolkit/tests/execution.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand All @@ -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()
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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, _) =
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) =
Expand All @@ -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, ..) =
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
113 changes: 113 additions & 0 deletions crates/radix-engine-toolkit/tests/general_transaction_visitor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

0 comments on commit a349239

Please sign in to comment.