If we want mandatory replace requests, we can simplify replaces a lot by removing the request_replace and withdraw_replace extrinsics, and modifying accept_replace such that it can be called by the old vault (and probably rename the function at that point)

Yes, I think that is ideal.

Following is the proposal for the mandatory replace request

1. accept_replace extrinsic
   • Parameters
     • old_vault:AccountID
     • new_vault:VaultID
     • amount: BalanceOf<T> // amount in IBTC
     • currency_pair: DefaultVaultCurrencyPair<T>

Flow Diagram

I think the following steps need to be done. A problem is that by making replace request originate from the old_vault, we can't supply a btc address as argument. Instead, we'll have to generate one like we do for issue, which does mean the client needs changes as well.

• remove the existing request_replace and withdraw_replace extrinsics

• Rename the current accept_replace to request_replace

• Change the new request_replace such that it is called by the old_vault rather than new_vault

• remove the collateral argument - instead we'll determine the collateral amount based on the amount_btc

• Remove btc_address - we'll have to auto-generate a deposit address like we do for issue

• remove replace_collateral from the vault type, it's no longer used

• On the client side, we'll have to scan for new replace deposit addresses the same way we do for issue

It's quite an involved change.. I'd like to have a think about if we can re-use the issue/redeem logic, as essentially the old-vault is making an issue with the new_vault, while the new_vault is making a redeem with the old_vault

We can't supply a btc address as argument

Why not? Do you mean from a UX perspective? In that case we can just use the master key which is already in the Vault's wallet.

Otherwise the change is as you describe, basically refactor accept_replace to request_replace. I would be happy for @nakul1010 to make that change already and then we can review after if it is possible to refactor further.

Why not? Do you mean from a UX perspective? In that case we can just use the master key which is already in the Vault's wallet.

The master key is per account_id, not per vault_id though, so it's not part of the vault's currency-specific wallet

Had a discussion with @sander2, we believe the best way forward is to re-use the issue and redeem logic. @nakul1010 hold-off on the implementation for now until we've had a chance to review the design.

We will refactor the replace pallet to reuse issue and redeem events / structs. Let's assume Vault A (old_vault) now wants to move their BTC to Vault B (new_vault) we can do the following:

• Vault A calls request_replace against Vault B
• Issue generates BtcAddress for Vault B
• Redeem request is generated for Vault A to send BTC to Vault B (using generated address)
  • Don't reserve wrapped tokens (since Vault A doesn't have any)
• Vault A sends BTC to Vault B (address + OP_RETURN)
• Vault A calls execute_replace with BTC transaction
  • We need to disallow execute_issue and execute_redeem
• Redeem request is executed
• Issue request is completed
  • Don't mint wrapped tokens

Some very rough code as a guide:

#[pallet::storage]
pub(super) type Requests<T: Config> = StorageMap<_, Blake2_128Concat, H256, H256>;

fn request_replace(
  old_vault_id: DefaultVaultId<T>,
  new_vault_id: DefaultVaultId<T>,
  amount: BalanceOf<T>,
) {
  // Vault B increases to_be_issued
  let (issue_id, btc_address) = Issue::request_issue(
    new_vault_id, // account to receive BTC
    amount, // amount of BTC
  );
  // Vault A increases to_be_redeemed
  let redeem_id = Redeem::request_redeem(
    old_vault_id, // account to send BTC
    btc_address, // owned by Vault B
    amount, // amount of BTC
  );
  Requests::insert(redeem_id, issue_id);
}

fn execute_replace(
  redeem_id: H256,
  unchecked_transaction: FullTransactionProof,
) {
  // Vault A decreases to_be_redeemed (and issued_tokens)
  Redeem::execute_redeem(redeem_id, unchecked_transaction);
  let issue_id = Requests::take(redeem_id);
  // Vault B decreases to_be_issued
  // Vault B increases issued_tokens
  Issue::complete_issue(issue_id);
}

Both extrinsics are submitted by the old_vault.

We should hence remove any unused replace code including storage items, events and extrinsics. We can also remove to_be_replaced_tokens, replace_collateral, and active_replace_collateral on the Vault struct. Please also consider any necessary migrations.

To disallow the other "execute" extrinsics (and generally make merging this into issue / redeem easier) it might be worth considering altering the IssueRequest and RedeemRequest structs, for example requester / redeemer could become:

enum AccountOrVault<AccountId, CurrencyId> {
  Account(AccountId),
  Vault(VaultId<AccountId, CurrencyId>)
}

Hmm I'm not completely convinced by this approach - both on the parachain side and on the client side this needs special handling. If we go with the issue/redeem approach, then I think for the vault client they should appear as normal issue/redeems. So the old_vault would execute by calling execute_redeem rather than execute_replace. The parachain can then apply replace logic if the AccountOrVault type is a Vault

Otherwise if we're ok with special handling on vault side then we might as well keep the current approach of having a special replace type, since arguably that keeps the parachain simpler

We had a call to discuss the approach and I think we are in agreement. Let's still remove the replace pallet (and Vault struct fields) but modify the following structs:

pub struct IssueRequest {
  ...
  pub requester: AccountOrVault,
}

pub struct RedeemRequest {
  ...
  pub redeemer: AccountOrVault,
  pub issue_id: Option<H256>,
}

We can add the request_replace extrinsic to the redeem pallet, but we should modify execute_redeem to include an additional code path for when issue_id is set. As stated before we should disallow the normal execution if the requester or redeemer is a Vault. Note that this approach creates an issue dependency on the redeem crate.

Yup agreed, just a small note that we don't need the pub redeemer: AccountOrVault, change - the request already contains the vaultid so if issue_id is set, you can just use that one