feat: mandatory replace #1169

nakul1010 · 2023-08-09T06:57:50Z

The PR adds the following extrinsic

Add Extrinsic: request_replace

Parameters:
origin: old vault
currency_pair: the currency id of the funds
amount: the amount of BTC to replace
new_vault_id: The address of the new Vault involved in this replace request
griefing_currency: The collateral amount provided by the old vault as griefing protection

Postconditions

Increase new vault to_be_issued tokens, done by request_issue
Increase old vault to_be_redeem tokens, done by request_redeem

Dry Run,
Bob(old vault) wants to replace 5 BTC with Alice(new_vault)

Bob calls request_replace with the amount as 5 BTC he is not replacing the full available lock collateral hence bob can ba fees

Alice -> to_be_issued + 5
Bob -> to_be_redeem + 4.9 (0.1 as fee - fee calculation is done by get_redeem_fee)

Bob calls execute_redeem

Bob -> to_be_redeem - 4.9
Bob -> issued - 4.9
Alice -> to_be_issued - 5
Alice -> issued + 5
Mint and distribute 0.1 as a replacement fee

Modify Extrinsic: execute_redeem

Postconditions in the case of is_redeem_a_replace_request gets a trigger

Decrease old vault to_be_redeem tokens
Decrease old vault issued tokens
Decrease new vault to_be_issued tokens
Increase new vault issued tokens
mint & distribute rewards

Modify Extrinsic: cancel_redeem

Postconditions in case of _cancel_replace_request gets trigger

Decrease new vault to_be_issued tokens
Decrease old vault to_be_redeem tokens
Cancel ReplaceRequest
Cancel IssueRequest
Punish the old_vault by transferring punishment fee to new_vault

Dry Run,
Bob(old vault) wants to cancel the earlier 5 BTC request_replace call with Alice(new_vault)

Bob calls cancel_redeem with the amount as 5 BTC

Alice -> to_be_issued - 5
Bob -> to_be_redeem - 4.9

The cancel_redeem handles cancel_replace logic
The ReplacePeriod is set to RedeemPeriod
Removed replace pallet
Removed unused fields in Vault struct
Update replace pallet runtime test cases
If old_vault is fully replacing the fees is set as zero.

Migrations Procedure

Use TxPause to filter calls to request_replace.

sander2

looks good from a quick glance, just a reminder that we should probably disallow execute_issue for replaces.

Also will be good to see updated tests in https://github.com/interlay/interbtc/blob/master/parachain/runtime/runtime-tests/src/parachain/replace.rs

crates/redeem/src/lib.rs

…ku/feat_mandatory_replace_request # Conflicts: # parachain/runtime/interlay/src/weights/replace.rs # parachain/runtime/kintsugi/src/weights/replace.rs

crates/issue/src/lib.rs

crates/redeem/src/lib.rs

primitives/src/lib.rs

parachain/runtime/interlay/src/lib.rs

parachain/runtime/kintsugi/src/lib.rs

crates/vault-registry/src/types.rs

crates/issue/src/migration.rs

crates/redeem/src/lib.rs

gregdhill

We'll also need to add benchmarks for some of the additional conditional flows for example in _cancel_issue

crates/issue/src/lib.rs

parachain/runtime/kintsugi/src/lib.rs

parachain/runtime/interlay/src/lib.rs

crates/issue/src/lib.rs

crates/redeem/src/lib.rs

nakul1010 · 2023-08-23T10:41:32Z

We'll also need to add benchmarks for some of the additional conditional flows for example in _cancel_issue

Have added benchmarking for the following pallet code
Issue Pallet

cancel_issue_for_replace path call through cancel_issue ext or cancel_redeem ext

Redeem pallet

cancel_replace path call through cancel_redeem ext
execute_replace path call through execute_redeem ext
request_replace ext

The benchmarking has been redone for the issue and redeem pallet code.

crates/issue/src/lib.rs

crates/vault-registry/src/types.rs

crates/issue/src/lib.rs

gregdhill · 2023-08-30T19:59:20Z

crates/issue/src/lib.rs

+        // its a replace issue call if requester is vault
+        let slashed_collateral = if is_replace_request {
+            // only cancel replace request if issue is expired
+            ensure!(issue_expired, Error::<T>::TimeNotExpired);
+            // for replace request griefing collateral is set as zero
+            issue.griefing_collateral()
+        } else {
+            let to_be_slashed_collateral = if issue_expired {


Suggested change

// its a replace issue call if requester is vault

let slashed_collateral = if is_replace_request {

// only cancel replace request if issue is expired

ensure!(issue_expired, Error::<T>::TimeNotExpired);

// for replace request griefing collateral is set as zero

issue.griefing_collateral()

} else {

let to_be_slashed_collateral = if issue_expired {

// its a replace issue call if requester is vault

let to_be_slashed_collateral = if is_replace_request {

// only cancel replace request if issue is expired

ensure!(issue_expired, Error::<T>::TimeNotExpired);

// for replace request griefing collateral is set as zero

issue.griefing_collateral()

} else if issue_expired {

I think we can flatten the conditionals right?

not really, if it's not a replace request to_be_slashed_collateral is also used to call vault-registery methods and decrease to-be-issued tokens which is only require for cancel_issue and not cancel_replace, hence i don't think we can flatten the condition

crates/issue/src/migration.rs

crates/redeem/src/lib.rs

crates/issue/src/lib.rs

crates/redeem/src/lib.rs

sander2 · 2023-08-31T10:25:32Z

crates/redeem/src/lib.rs

+                // the inclusion fee is paid by new vault itself hence we can set it as zero
+                Amount::zero(vault_id.wrapped_currency()),


This could result in the vault being unable to pay inclusion fees. If we want to be able to fully replace all issued tokens, I guess there's not really a way around this. But we should make it very clear in the UI that the vault is responsible for making sure it can pay these fees @tomjeatt cc @gregdhill

crates/redeem/src/lib.rs

sander2 · 2023-08-31T14:43:09Z

parachain/runtime/runtime-tests/src/parachain/nomination.rs

 fn integration_test_nominated_collateral_prevents_replace_requests() {
    test_with_nomination_enabled_and_vault_opted_in(|vault_id| {
        assert_nominate_collateral(&vault_id, account_of(USER), default_nomination(&vault_id));
-        assert_noop!(
-            RuntimeCall::Replace(ReplaceCall::request_replace {
-                currency_pair: vault_id.currencies.clone(),
-                amount: 0,
-            })
-            .dispatch(origin_of(vault_id.account_id.clone())),
-            ReplaceError::VaultHasEnabledNomination
-        );
-    });
-}


@gregdhill this test was probably a left-over from when rewards were proportional to issued tokens, right? If so, this test can be removed altogether

sander2 · 2023-08-31T14:43:50Z

parachain/runtime/runtime-tests/src/parachain/redeem.rs

@@ -643,7 +628,7 @@ mod spec_based_tests {
            test_with(|vault_id| {
                set_redeem_period(1000);
                let redeem_id = request_redeem(&vault_id);
-                mine_blocks(12);
+                mine_blocks(100);


Is this change required? If so, I'd like to know why

Revert back for test case integration_test_redeem_expiry_with_period_increase, can't revert back for integration_test_redeem_can_only_be_cancelled_by_redeemer
Reasons

cancel_redeem flow changed

the old version use to check UnauthorizedRedeemer ensure first, check out here

The new version TimeNotExpired ensure needs to pass before UnauthorizedRedeemer as UnauthorizedRedeemer is not required for replace cancel.

parachain/runtime/runtime-tests/src/parachain/replace.rs

sander2 · 2023-08-31T15:17:16Z

@gregdhill we have a lot of issues and redeems to migrate, I'm worried we might not have enough capacity in a single block. I've heard stories of chain getting stuck because of overweight migrations. Should we implement a multi-block migration?

…orts

…ku/feat_mandatory_replace_request # Conflicts: # Cargo.lock

sander2 · 2023-09-01T10:19:23Z

As discussed, we are parking this for now. I just wanted to record one thought I had on this pr: issue and redeem periods are different, we need to make sure that the issue can't get cancelled while the redeem is opened

nakul1010 added the inProgress The PR is still in progress label Aug 9, 2023

nakul1010 self-assigned this Aug 9, 2023

nakul1010 marked this pull request as draft August 9, 2023 07:39

sander2 reviewed Aug 9, 2023

View reviewed changes

crates/redeem/src/lib.rs Outdated Show resolved Hide resolved

feat: add mandatory replace

93d5b17

nakul1010 force-pushed the naku/feat_mandatory_replace_request branch from 66151f4 to 93d5b17 Compare August 14, 2023 04:48

Merge branch 'master' of https://github.com/interlay/interbtc into na…

676fb32

…ku/feat_mandatory_replace_request # Conflicts: # parachain/runtime/interlay/src/weights/replace.rs # parachain/runtime/kintsugi/src/weights/replace.rs

nakul1010 added ReviewNeeded The PR can be reviewed by peers and removed inProgress The PR is still in progress labels Aug 14, 2023

nakul1010 marked this pull request as ready for review August 14, 2023 05:05

nakul1010 requested review from sander2 and gregdhill August 14, 2023 05:05

nakul1010 added inProgress The PR is still in progress and removed ReviewNeeded The PR can be reviewed by peers labels Aug 14, 2023

nakul1010 marked this pull request as draft August 14, 2023 05:13

fix: test cases

b217ece

nakul1010 force-pushed the naku/feat_mandatory_replace_request branch from 1ab5cbd to b217ece Compare August 15, 2023 13:45

nakul1010 added ReviewNeeded The PR can be reviewed by peers and removed inProgress The PR is still in progress labels Aug 16, 2023

nakul1010 marked this pull request as ready for review August 16, 2023 05:18

sander2 reviewed Aug 16, 2023

View reviewed changes

nakul1010 requested a review from sander2 August 17, 2023 11:38

gregdhill reviewed Aug 17, 2023

View reviewed changes

gregdhill reviewed Aug 21, 2023

View reviewed changes

nakul1010 requested a review from gregdhill August 22, 2023 12:35

nakul1010 mentioned this pull request Aug 24, 2023

Remove griefing collateral from mandatory replace request #1182

Open

nakul1010 added inProgress The PR is still in progress and removed ReviewNeeded The PR can be reviewed by peers inProgress The PR is still in progress labels Aug 24, 2023

nakul1010 added the ReviewNeeded The PR can be reviewed by peers label Aug 29, 2023

nakul1010 force-pushed the naku/feat_mandatory_replace_request branch from de9b057 to ca767e9 Compare August 29, 2023 07:06

gregdhill reviewed Aug 30, 2023

View reviewed changes

nakul1010 force-pushed the naku/feat_mandatory_replace_request branch from 7bb6766 to 2c37913 Compare August 31, 2023 06:37

nakul1010 requested a review from gregdhill August 31, 2023 07:24

sander2 reviewed Aug 31, 2023

View reviewed changes

fix: test cases, simply branch for request_redeem, removed unused imp…

033392b

…orts

nakul1010 force-pushed the naku/feat_mandatory_replace_request branch from dd0ac31 to 033392b Compare September 1, 2023 05:18

nakul1010 added 4 commits September 1, 2023 10:48

Merge branch 'master' of https://github.com/interlay/interbtc into na…

f0d45ab

…ku/feat_mandatory_replace_request # Conflicts: # Cargo.lock

fix: removed unused feature gate

6017888

fix: set mine blocks to issue period

9ac38f7

fix: update mine blocks

b4c7e84

nakul1010 requested a review from sander2 September 1, 2023 06:18

gregdhill removed the ReviewNeeded The PR can be reviewed by peers label Sep 1, 2023

nakul1010 removed request for sander2 and gregdhill September 1, 2023 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: mandatory replace #1169

feat: mandatory replace #1169

nakul1010 commented Aug 9, 2023 •

edited

Loading

sander2 left a comment •

edited

Loading

gregdhill left a comment

nakul1010 commented Aug 23, 2023 •

edited

Loading

gregdhill Aug 30, 2023

gregdhill Aug 30, 2023

nakul1010 Aug 31, 2023 •

edited

Loading

sander2 Aug 31, 2023

sander2 Aug 31, 2023

sander2 Aug 31, 2023

nakul1010 Sep 1, 2023

sander2 commented Aug 31, 2023

sander2 commented Sep 1, 2023

		// the inclusion fee is paid by new vault itself hence we can set it as zero
		Amount::zero(vault_id.wrapped_currency()),

feat: mandatory replace #1169

Are you sure you want to change the base?

feat: mandatory replace #1169

Conversation

nakul1010 commented Aug 9, 2023 • edited Loading

sander2 left a comment • edited Loading

Choose a reason for hiding this comment

gregdhill left a comment

Choose a reason for hiding this comment

nakul1010 commented Aug 23, 2023 • edited Loading

gregdhill Aug 30, 2023

Choose a reason for hiding this comment

gregdhill Aug 30, 2023

Choose a reason for hiding this comment

nakul1010 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

sander2 Aug 31, 2023

Choose a reason for hiding this comment

sander2 Aug 31, 2023

Choose a reason for hiding this comment

sander2 Aug 31, 2023

Choose a reason for hiding this comment

nakul1010 Sep 1, 2023

Choose a reason for hiding this comment

sander2 commented Aug 31, 2023

sander2 commented Sep 1, 2023

nakul1010 commented Aug 9, 2023 •

edited

Loading

sander2 left a comment •

edited

Loading

nakul1010 commented Aug 23, 2023 •

edited

Loading

nakul1010 Aug 31, 2023 •

edited

Loading