Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: fallback for vault swap if no broker provided #5507

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bouncer/commands/run_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import { testDeltaBasedIngress } from '../tests/delta_based_ingress';
import { testCancelOrdersBatch } from '../tests/create_and_delete_multiple_orders';
import { depositChannelCreation } from '../tests/request_swap_deposit_address_with_affiliates';
import { testDCASwaps } from '../tests/DCA_test';
import { testVaultSwapFeeCollection } from '../tests/vault_swap_fee_collection';
import { testVaultSwapFeeCollection } from '../tests/vault_swap_tests';
import { testBrokerLevelScreening } from '../tests/broker_level_screening';
import { testSolanaVaultSettingsGovernance } from '../tests/solana_vault_settings_governance';

Expand Down
2 changes: 1 addition & 1 deletion bouncer/tests/all_concurrent_tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { testDCASwaps } from './DCA_test';
import { testBrokerLevelScreening } from './broker_level_screening';
import { checkSolEventAccountsClosure } from '../shared/sol_vault_swap';
import { legacyEvmVaultSwaps } from './legacy_vault_swap';
import { testVaultSwapFeeCollection } from './vault_swap_fee_collection';
import { testVaultSwapFeeCollection } from './vault_swap_tests';

async function runAllConcurrentTests() {
// Specify the number of nodes via providing an argument to this script.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
import { getEarnedBrokerFees } from './broker_fee_collection';
import { openPrivateBtcChannel, registerAffiliate } from '../shared/btc_vault_swap';
import { setupBrokerAccount } from '../shared/setup_account';
import { performVaultSwap } from '../shared/perform_swap';
import { executeVaultSwap, performVaultSwap } from '../shared/perform_swap';
import { prepareSwap } from '../shared/swapping';
import { getChainflipApi } from '../shared/utils/substrate';
import { getBalance } from '../shared/get_balance';
Expand All @@ -28,6 +28,51 @@ export const testVaultSwapFeeCollection = new ExecutableTest(
// Fee to use for the broker and affiliates
const commissionBps = 100;

async function testRefundVaultSwap() {
testVaultSwapFeeCollection.log('Starting refund vault swap test...');

const inputAsset = Assets.Btc;
const destAsset = Assets.Usdc;
const balanceObserveTimeout = 60;
const depositAmount = defaultAssetAmounts(inputAsset);
const destAddress = await newAddress('Usdc', 'BTC_VAULT_SWAP_REFUND' + Math.random() * 100);
const refundAddress = await newAddress('Btc', 'BTC_VAULT_SWAP_REFUND' + Math.random() * 100);
const foKParams = {
retryDurationBlocks: 100,
refundAddress,
minPriceX128: '0',
};

testVaultSwapFeeCollection.log('Sending vault swap...');

await executeVaultSwap(
inputAsset,
destAsset,
destAddress,
undefined,
depositAmount,
0, // boostFeeBps
foKParams,
);

testVaultSwapFeeCollection.log('Waiting for refund...');

let btcBalance = false;

for (let i = 0; i < balanceObserveTimeout; i++) {
const refundAddressBalance = await getBalance(Assets.Btc, refundAddress);
if (refundAddressBalance !== '0') {
btcBalance = true;
break;
}
await sleep(1000);
}

assert(btcBalance, `Vault swap refund failed 🙅‍♂️.`);

testVaultSwapFeeCollection.log('Refund vault swap completed ✅.');
}

async function testWithdrawCollectedAffiliateFees(
broker: KeyringPair,
affiliateAccountId: string,
Expand Down Expand Up @@ -149,4 +194,5 @@ async function main() {
// Test the affiliate withdrawal functionality
const [broker, affiliateId, refundAddress] = await testFeeCollection(Assets.Btc);
await testWithdrawCollectedAffiliateFees(broker, affiliateId, refundAddress);
await testRefundVaultSwap();
}
4 changes: 2 additions & 2 deletions state-chain/cf-integration-tests/src/solana.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use cf_chains::{
ExecutexSwapAndCallError, ForeignChainAddress, RequiresSignatureRefresh, SetAggKeyWithAggKey,
SetAggKeyWithAggKeyError, Solana, SwapOrigin, TransactionBuilder,
};
use cf_primitives::{AccountRole, AuthorityCount, ForeignChain, SwapRequestId};
use cf_primitives::{AccountRole, AuthorityCount, Beneficiary, ForeignChain, SwapRequestId};
use cf_test_utilities::{assert_events_match, assert_has_matching_event};
use cf_utilities::bs58_array;
use codec::Encode;
Expand Down Expand Up @@ -378,7 +378,7 @@ fn vault_swap_deposit_witness(
deposit_metadata,
tx_id: Default::default(),
deposit_details: (),
broker_fee: None,
broker_fee: Some(Beneficiary { account: BROKER.into(), bps: 0 }),
affiliate_fees: Default::default(),
refund_params: Some(REFUND_PARAMS),
dca_params: None,
Expand Down
8 changes: 4 additions & 4 deletions state-chain/cf-integration-tests/src/swapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
fund_authorities_and_join_auction, new_account, register_refund_addresses,
setup_account_and_peer_mapping, Cli, Network,
},
witness_call, witness_ethereum_rotation_broadcast, witness_rotation_broadcasts,
witness_call, witness_ethereum_rotation_broadcast, witness_rotation_broadcasts, BROKER,
};
use cf_amm::{
math::{price_at_tick, Price, Tick},
Expand All @@ -23,8 +23,8 @@ use cf_chains::{
RetryPolicy, SwapOrigin, TransactionBuilder, TransferAssetParams,
};
use cf_primitives::{
AccountId, AccountRole, Asset, AssetAmount, AuthorityCount, SwapId, FLIPPERINOS_PER_FLIP,
GENESIS_EPOCH, STABLE_ASSET, SWAP_DELAY_BLOCKS,
AccountId, AccountRole, Asset, AssetAmount, AuthorityCount, Beneficiary, SwapId,
FLIPPERINOS_PER_FLIP, GENESIS_EPOCH, STABLE_ASSET, SWAP_DELAY_BLOCKS,
};
use cf_test_utilities::{assert_events_eq, assert_events_match, assert_has_matching_event};
use cf_traits::{AdjustedFeeEstimationApi, AssetConverter, BalanceApi, EpochInfo, SwapType};
Expand Down Expand Up @@ -530,7 +530,7 @@ fn vault_swap_deposit_witness(
deposit_metadata: Some(ccm_deposit_metadata_mock()),
tx_id: Default::default(),
deposit_details: DepositDetails { tx_hashes: None },
broker_fee: None,
broker_fee: Some(Beneficiary { account: BROKER.into(), bps: 0 }),
affiliate_fees: Default::default(),
refund_params: Some(ETH_REFUND_PARAMS),
dca_params: None,
Expand Down
42 changes: 41 additions & 1 deletion state-chain/pallets/cf-ingress-egress/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use cf_chains::{
ChannelRefundParametersDecoded, ConsolidateCall, DepositChannel,
DepositDetailsToTransactionInId, DepositOriginType, ExecutexSwapAndCall, FetchAssetParams,
ForeignChainAddress, IntoTransactionInIdForAnyChain, RejectCall, SwapOrigin,
TransferAssetParams,
TransferAssetParams, TransferFallback,
};
use cf_primitives::{
AccountRole, AffiliateShortId, Affiliates, Asset, AssetAmount, BasisPoints, Beneficiaries,
Expand Down Expand Up @@ -920,6 +920,9 @@ pub mod pallet {
NetworkFeeDeductionFromBoostSet {
deduction_percent: Percent,
},
VaultSwapRefunded {
tx_id: TransactionInIdFor<T, I>,
},
}

#[derive(CloneNoBound, PartialEqNoBound, EqNoBound)]
Expand Down Expand Up @@ -1987,6 +1990,17 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
Ok(())
}

/// Returns true if the vault swap should be rejected.
///
/// A vault swap should be rejected if the broker fee is not set or the account is not a broker.
fn should_reject_vault_swap(broker_fee: &Option<Beneficiary<T::AccountId>>) -> bool {
match broker_fee {
Some(Beneficiary { account, .. }) =>
!T::AccountRoleRegistry::has_account_role(account, AccountRole::Broker),
_ => true,
}
}

fn assemble_broker_fees(
broker_fee: Option<Beneficiary<T::AccountId>>,
affiliate_fees: Affiliates<AffiliateShortId>,
Expand Down Expand Up @@ -2161,6 +2175,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
boost_fee,
}: VaultDepositWitness<T, I>,
) {
if Self::should_reject_vault_swap(&broker_fee) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only check this when we pre-witness. Why not on full deposits? What about chains where we don't have pre-witnessing - am I missing something?

Copy link
Contributor Author

@Janislav Janislav Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should always have the prewitness step, right? The reason why we do it here is boost. Otherwise, we would first send out the funds and then later also refund it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure we only prewitness for Bitcoin. Also, sometimes we miss the pre-witness (for example if there's a reorg).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but checking it only on full deposit is not an option since it could be a boosted BTC transaction. We have to check it on both stages but only refund it on full deposit

return;
}

let destination_address_internal =
match T::AddressConverter::decode_and_validate_address_for_asset(
destination_address.clone(),
Expand Down Expand Up @@ -2438,6 +2456,28 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
boost_fee,
} = vault_deposit_witness.clone();

if Self::should_reject_vault_swap(&broker_fee) {
// TODO: We are going to make refund_params mandatory so we can remove this if let check
// as soon the other PR is merged.
if let Some(refund_params) = refund_params {
if let Ok(api_call) =
<T::ChainApiCall as TransferFallback<T::TargetChain>>::new_unsigned(
TransferAssetParams {
asset: source_asset,
amount: deposit_amount,
to: refund_params.refund_address,
},
) {
T::Broadcaster::threshold_sign_and_broadcast(api_call);
Self::deposit_event(Event::VaultSwapRefunded { tx_id: tx_id.clone() });
} else {
log_or_panic!("Failed to create refund api call for vault swap.");
}
};

return;
}

let boost_status =
BoostedVaultTransactions::<T, I>::get(&tx_id).unwrap_or(BoostStatus::NotBoosted);

Expand Down
86 changes: 66 additions & 20 deletions state-chain/pallets/cf-ingress-egress/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ use frame_support::{
traits::{Hooks, OriginTrait},
weights::Weight,
};
use sp_core::{bounded_vec, H160};
use sp_core::{bounded_vec, H160, U256};
use sp_runtime::{DispatchError, DispatchResult, Percent};

const ALICE_ETH_ADDRESS: EthereumAddress = H160([100u8; 20]);
Expand Down Expand Up @@ -1975,25 +1975,11 @@ fn charge_no_broker_fees_on_unknown_primary_broker() {
0
));

// The request is recorded as not having any broker fees:
assert_eq!(
MockSwapRequestHandler::<Test>::get_swap_requests(),
vec![MockSwapRequest {
input_asset: INPUT_ASSET,
output_asset: OUTPUT_ASSET,
input_amount: INPUT_AMOUNT,
swap_type: SwapRequestType::Regular { output_address, ccm_deposit_metadata: None },
broker_fees: Default::default(),
origin: SwapOrigin::Vault {
tx_id: cf_chains::TransactionInIdForAnyChain::Evm(H256::default()),
broker_id: Some(NOT_A_BROKER),
},
},]
);

assert_has_event::<Test>(RuntimeEvent::IngressEgress(PalletEvent::UnknownBroker {
broker_id: NOT_A_BROKER,
assert_has_event::<Test>(RuntimeEvent::IngressEgress(PalletEvent::VaultSwapRefunded {
tx_id: H256::default(),
}));

assert!(MockSwapRequestHandler::<Test>::get_swap_requests().is_empty());
});
}

Expand Down Expand Up @@ -2131,7 +2117,7 @@ fn failed_ccm_deposit_can_deposit_event() {
Some(ccm_deposit_metadata.clone()),
Default::default(),
DepositDetails { tx_hashes: None },
Beneficiary { account: 0, bps: 0 },
Beneficiary { account: BROKER, bps: 0 },
Default::default(),
ETH_REFUND_PARAMS,
None,
Expand Down Expand Up @@ -2281,3 +2267,63 @@ fn ignore_change_of_minimum_deposit_if_deposit_is_not_boosted() {
.is_ok());
});
}

#[test]
fn gets_refunded_if_vault_transaction_was_aborted() {
new_test_ext().execute_with(|| {
let tx_id = H256::default();

let vault_swap = VaultDepositWitness {
input_asset: Asset::Eth.try_into().unwrap(),
deposit_address: Default::default(),
channel_id: Some(0),
deposit_amount: 100,
deposit_details: Default::default(),
output_asset: Asset::Eth,
destination_address: EncodedAddress::Eth(Default::default()),
deposit_metadata: Default::default(),
tx_id,
broker_fee: None,
affiliate_fees: Default::default(),
refund_params: Some(ChannelRefundParameters {
retry_duration: 0,
min_price: U256::from(0),
refund_address: H160::default(),
}),
dca_params: None,
boost_fee: 0,
};

IngressEgress::process_vault_swap_request_prewitness(0, vault_swap.clone());

IngressEgress::process_vault_swap_request_full_witness(0, vault_swap);

assert_has_matching_event!(
Test,
RuntimeEvent::IngressEgress(Event::VaultSwapRefunded { tx_id: _ })
);

assert!(
MockSwapRequestHandler::<Test>::get_swap_requests().is_empty(),
"No swaps should have been triggered!"
);

assert!(
MockEgressBroadcaster::get_pending_api_calls().len() == 1,
"Refund broadcast should have been scheduled!"
);
});
}

#[test]
fn reject_requirements_test() {
new_test_ext().execute_with(|| {
let should_fail = Some(Beneficiary { account: ALICE, bps: 0 });
let should_succeed = Some(Beneficiary { account: BROKER, bps: 0 });

assert!(IngressEgress::should_reject_vault_swap(&should_fail));
assert!(IngressEgress::should_reject_vault_swap(&None));

assert!(!IngressEgress::should_reject_vault_swap(&should_succeed));
});
}
Loading