Add multi currency vaults #415

bvotteler · 2022-05-26T17:36:28Z

Add KINT as collateral

Updated tests/code to work with upgraded docker images (parachain version 1.13.1, clients version 1.11.2)
Added new collateral currency, KINT
Updated tests to run for KSM as well as KINT collateral currencies
Enabled and fixed test checking vault theft and liquidation
Update @polkadot/api to version 8.6.2

Breaking changes to exposed API:

VaultsAPI.reportVaultTheft now takes vault ID (InterbtcPrimitivesVaultId) instead of only account ID
VaultsAPI.isVaultFlaggedForTheft now takes vault ID (InterbtcPrimitivesVaultId) instead of account ID and collateral currency literal ID
Removed ConstantsAPI.getTransactionByteFee(); constant no longer exists

…onger exists

…rk with changed signature

…rencies

…ults

…aults

…ore set

…ialized

src/types/currency.ts

…eplace requests

…r the vault' Edge case of 0 wrapped tokens, require 0 collateral. Therefore, compare that collateral is greater than or equal to wrapped (backed) tokens as opposed to only greater than.

…failure

…collateral vaults

…vaults.test.ts

…erbtcPrimitivesVaultId instead of only AccountId

…s test covering vault theft and liquidation

Minor version change due to breaking changes in `VaultsAPI.reportVaultTheft` signature.

…tion (escrow tests)

daniel-savu · 2022-06-06T09:16:37Z

test/integration/parachain/release/redeem.test.ts

+            await waitForBlockFinalization(bitcoinCoreClient, userInterBtcAPI.btcRelay);
+
+            // report theft
+            await reporterInterBtcAPI.vaults.reportVaultTheft(vaultToLiquidateId, btcTxId);


Nit: Anyone can report theft, so you could have reused userInterBtcAPI instead of instantiating reporterInterBtcAPI

daniel-savu · 2022-06-06T09:36:44Z

test/integration/parachain/staging/sequential/refund.test.ts

+                "0", 
+                `Expected non-zero amount for refund request with vault 3 (${currencyTicker})`
+            );
+            const refundRequests = await interBtcAPI.refund.list();


The length of this list should be checked after the for-loop ends, with an expected length of at least vault_3_ids.length. We can merge without this change though, since the assert on the line above already checks this essentially.

Good spot, thanks. I have a follow-up task to address this.

daniel-savu · 2022-06-06T09:39:20Z

test/integration/parachain/staging/sequential/replace.test.ts

+                interBtcAPI.setAccount(vault_3);
+                await interBtcAPI.replace.request(
+                    replaceAmount, 
+                    currencyIdToMonetaryCurrency(vault_3_id.currencies.collateral) as CollateralCurrency
+                );


Would be cleaner to refactor this to use the callWith utility.

Ah, nice! Thank you for that. Added these to the follow-up task, too.

daniel-savu · 2022-06-06T09:39:42Z

test/integration/parachain/staging/sequential/replace.test.ts

+                interBtcAPI.setAccount(vault_2);
+
+                // fetch tokens held by vault
+                const tokensInVault = await interBtcAPI.vaults.getIssuedAmount(
+                    newAccountId(api, vault_2.address),
+                    currencyIdToLiteral(vault_2_id.currencies.collateral)
                );
-                assert.fail("Expected error to be thrown due to lack of issued tokens for vault, but call completed.");
-            } catch (e) {
-                assert.isTrue(e instanceof Error, "Expected replace request to fail with Error");
+


Would be cleaner to refactor this to use the callWith utility.

daniel-savu · 2022-06-06T09:43:16Z

test/integration/parachain/staging/sequential/replace.test.ts

+                    await interBtcAPI.replace.request(
+                        replaceAmount, 
+                        currencyIdToMonetaryCurrency(vault_2_id.currencies.collateral) as CollateralCurrency
+                    );
+                    assert.fail(`Expected error to be thrown due to lack of issued tokens
+                        for vault (collateral: ${currencyTicker}), but call completed.`);


Would be cleaner to check that the Promise is rejected, also not blocking

Also added to the follow-up task.
Thank you for the suggestion.

daniel-savu · 2022-06-06T09:47:27Z

For later: We should think of a way to make it clear that the tests are running on new parachain and clients versions - maybe tag parachain-launch versions. The yarn docker-parachain-start script just pulls the master branch of the parachain-launch script, so reviewers of this PR would be unaware of any changes.

daniel-savu

LGTM, all my comments can be fixed later

bvotteler · 2022-06-06T10:14:21Z

We should think of a way to make it clear that the tests are running on new parachain and clients versions - maybe tag parachain-launch versions. The yarn docker-parachain-start script just pulls the master branch of the parachain-launch script, so reviewers of this PR would be unaware of any changes.

Yes! I was thinking along those lines, too.
Certainly a fan of tagging. Using separate versions for our fork of the official parachain-launch can get tricky. So I want to think about that for a bit.

I also want to at least check if we can possibly go back to the upstream version of parachain-launch rather than maintaining a fork, and keep the configuration details in the lib to then be generated as part of yarn build.

gregdhill

Overall this looks good, though it is hard to comment on whether there is enough to catch flaws in our multi-collateral architecture. There are a fair number of skipped and commented tests but I assume these are for future work? We should definitely sync on what can be moved into the parachain / client tests or even removed to make your life easier. I like the overly verbose nature of the assertions and the reduced reliance on test ordering - idempotent tests make for easier debugging. Anyway, none of my comments are particularly critical to block merging this as-is so feel free address in future PRs.

gregdhill · 2022-06-06T09:25:26Z

test/integration/parachain/release/redeem.test.ts

+            // report theft
+            await reporterInterBtcAPI.vaults.reportVaultTheft(vaultToLiquidateId, btcTxId);
+
+            const finalizedPromise = new Promise<void>((resolve, _) => userInterBtcAPI.system.subscribeToFinalizedBlockHeads(


Nit: could await here

Fair point, there are two places where I do that.

gregdhill · 2022-06-06T09:26:10Z

test/integration/parachain/release/redeem.test.ts

-            assert.isTrue(flaggedForTheft);
+            assert.isTrue(
+                flaggedForTheft,
+                `Expected vault (collateral: ${collateralCurrency.ticker}) to be flagged for theft, but it was not.`


👍 for useful expectation messages

Thank you.
It would drive me bonkers if I saw a failure and wouldn't know which collateral variation failed without manually running the test with breakpoints. :)

gregdhill · 2022-06-06T09:38:40Z

test/integration/parachain/release/redeem.test.ts

+                    await issueAndRedeem(
+                        userInterBtcAPI, 
+                        userBitcoinCoreClient, 
+                        userAccount, 
+                        vaultToBanId, 
+                        issueAmount, 
+                        redeemAmount, 
+                        false, 
+                        ExecuteRedeem.False
+                    );


For future ref: #416

Oh yes. I hope to have some time to add more builder patterns soon™ :)

gregdhill · 2022-06-06T09:39:40Z

test/integration/parachain/release/redeem.test.ts

+                await new Promise<void>((resolve, _) => {
+                    // redeemAPI.subscribeToRedeemExpiry(newAccountId(api, userAccount.address), (requestId) => {
+                    //     if (stripHexPrefix(redeemRequest.id.toString()) === stripHexPrefix(requestId.toString())) {
+                    //         resolve();
+                    //     }
+                    // });
+                });


Any reason not to implement this now?

We had a chat last week with @nud3l to pare down the scope of tests to add/fix to get multi-collateral vaults unblocked sooner rather than later.
This test fell victim to that.

gregdhill · 2022-06-06T10:08:46Z

test/integration/parachain/staging/setup/initialize.test.ts

@@ -103,8 +98,7 @@ describe("Initialize parachain state", () => {
        userInterBtcAPI = new DefaultInterBtcApi(api, "regtest", userAccount, ESPLORA_BASE_PATH);
        sudoInterBtcAPI = new DefaultInterBtcApi(api, "regtest", sudoAccount, ESPLORA_BASE_PATH);

-        wrappedCurrency = userInterBtcAPI.getWrappedCurrency();
-        collateralCurrency = getCorrespondingCollateralCurrency(userInterBtcAPI.getGovernanceCurrency());
+        collateralCurrency = getCorrespondingCollateralCurrencies(userInterBtcAPI.getGovernanceCurrency())[0];


Does this implicitly assume KSM? What happens to the tests if I change the return value of getCorrespondingCollateralCurrency?

It only needs one collateral currency, whether that is KSM, or KINT doesn't matter here.
The currency is used in conjunction with vault account ids to wait for them to be available - as means to delay test execution until the parachain / docker images are good to go with vaults registered.

gregdhill · 2022-06-06T10:21:19Z

test/integration/parachain/staging/sequential/issue.test.ts

+                    await new Promise<void>((resolve, _) => {
+                        // userInterBtcAPI.issue.subscribeToIssueExpiry(newAccountId(api, userAccount.address), (requestId) => {
+                        //     if (stripHexPrefix(requestResult.id.toString()) === stripHexPrefix(requestId.toString())) {
+                        //         resolve();
+                        //     }
+                        // });
+                    });


Same here as above, for a future PR?

Correct, I just added the looping for the time being.

gregdhill · 2022-06-06T10:26:31Z

test/integration/parachain/staging/sequential/issue.test.ts

+                } catch (e) {
+                    // Set issue period back to its initial value to minimize side effects.
+                    await sudo(userInterBtcAPI, () => userInterBtcAPI.issue.setIssuePeriod(initialIssuePeriod));
+                    throw e;
+                }


Good idea to reset the state, though I wonder if we should have some common routine that runs after most tests to "re-genesis" from a blank state. However I guess that may dramatically slowdown the pipeline if Substrate doesn't have an easy way to do that though.

Once we have instant seal, we might be able to afford re-genesis delays, assuming it can be done.
I'm certainly open to the idea.

gregdhill · 2022-06-06T10:37:40Z

test/integration/parachain/staging/sequential/oracle.test.ts

+        for(const collateralCurrency of collateralCurrencies) {
+            const bitcoinAmount = BitcoinAmount.from.BTC(100);
+            const exchangeRate = await interBtcAPI.oracle.getExchangeRate(collateralCurrency);
+            const expectedCollateral = exchangeRate.toBig(undefined).mul(bitcoinAmount.toBig(BitcoinUnit.BTC)).round(0, 0);


Why undefined?

Good catch! That was supposed to be exchangeRate.toBig() - looks like me stripping the previously defined, single collateralCurrency, auto-refactored that into undefined.
Awkward, but at least not breaking.

gregdhill · 2022-06-06T10:39:21Z

test/integration/parachain/staging/sequential/vaults.test.ts

+            for (const [vaultId, amount] of premiumRedeemVaults) {
+                if (encodeVaultId(vaultId) === encodeVaultId(vault_3_id)) {
+                    premiumRedeemAmount = amount;
+                    break;
+                }
+            }


There is probably a more functional way to write this logic but that's probably more a nit from my side

Right. A version of array.find().map() would do the trick.
Keeping this for my clean up task.

gregdhill · 2022-06-06T10:40:50Z

test/integration/parachain/staging/sequential/vaults.test.ts

+        const expectedThresholdByTicker: Map<string, string> = new Map([
+            [Polkadot.ticker, "1.1"],
+            [Kusama.ticker, "1.1"],
+            [Kintsugi.ticker, "2"]
+        ]);


Are these set during initialization? If not we should definitely review our use of magic numbers in tests so us changing the genesis on the parachain doesn't randomly break things here.

No, those are magic numbers.
Happy to set during initialization - I couldn't find a way when working the tests.

I think that piece of work would fit in well with the proposed changes by Dom here: #415 (comment)

bvotteler · 2022-06-06T11:37:40Z

There are a fair number of skipped and commented tests but I assume these are for future work? We should definitely sync on what can be moved into the parachain / client tests or even removed to make your life easier.

As for the skipped/commented tests: Yes, they are future work and haven't been deemed critical for the goal of getting multi-collateral vaults out.

As for reducing integration test numbers: Yes, I absolutely would love to get rid of the ones that the parachain and/or clients test themselves.
That, and the introduction of instant-seal should make these tests much, much faster.

bvotteler added 17 commits May 25, 2022 13:44

chore: upgrade parachain state fro version 1.13.1

d26ec16

chore: remove ConstantsAPI.getTransactionByteFee - that constant no l…

c3bc7ad

…onger exists

chore: remove unused import

f9b87f8

chore: updated collateral currencies getter method, fixed tests to wo…

28edb7e

…rk with changed signature

fix (test): remove unused imports

155ee8d

chore (test): update fee.test.ts to use multi-collateral currencies

43fcd05

chore (test): update nomination.test.ts for multi-collateral vaults

b46f505

chore (test): update oracle.test.ts to work with multi-collateral cur…

3aedfe5

…rencies

chore (test): update redeem.test.ts to work with multi-collateral vaults

b89d0e3

chore (test): updated refund.test.ts to work with multi-collateral va…

c8dab4a

…ults

chore (test): updated replace.test.ts to work with multi-collateral v…

e4879d8

…aults

chore (test): update vaults.test.ts to work with multi-collateral vaults

fa262f1

chore (test): update redeem.test.ts to work with multi-collateral vaults

835d452

chore (test): adjust timeouts to account for additional vaults to test

618d52e

fix (test): simply double timeouts to account for more vaults

17a0852

fix (test): fix broken types, conversions, and variables accessed bef…

d84fb24

…ore set

fix (test): fix type errors and accessing variables before being init…

929ab58

…ialized

nud3l reviewed May 27, 2022

View reviewed changes

src/types/currency.ts Show resolved Hide resolved

bvotteler added 12 commits May 30, 2022 10:29

chore (test): add notes to suggested fixes do be done later

9958990

fix (test): initialize map before adding entries

1e423d4

fix (test): fix test assertions to work with multi-collateral vault r…

44c2bcc

…eplace requests

fix (test): fix comparator for 'should get the required collateral fo…

a5789b3

…r the vault' Edge case of 0 wrapped tokens, require 0 collateral. Therefore, compare that collateral is greater than or equal to wrapped (backed) tokens as opposed to only greater than.

fix (package): fix versions for ts-node and shelljs

8b19b67

chore (build): print versions for yarn, node, and ts-node

8c8f1bb

chore (test): add more information about causes for a potential test …

61ffbb4

…failure

chore (test): update todo description in fee.test.ts

84cd4c0

fix (test): fix broken test in vaults.test.ts since introducing KINT …

84151b3

…collateral vaults

fix (test): add missing assumptions for KINT (and DOT) thresholds in …

e67e91d

…vaults.test.ts

fix (build): try to fix syntax for new step

15422f7

fix (build): yet another try to fix syntact

36c705b

bvotteler added 10 commits May 31, 2022 13:51

refactor (vaultsAPI): change signature of reportVaultTheft to use Int…

8e73ac9

…erbtcPrimitivesVaultId instead of only AccountId

feat (transactionAPI): add method to wait for next finalized event

1a670a2

fix (test): reenable, implement gaps, and fix issues in redeem.test.t…

98e19e5

…s test covering vault theft and liquidation

chore: bump version

9bd980e

Minor version change due to breaking changes in `VaultsAPI.reportVaultTheft` signature.

fix (test): undo change in replace.test.ts

07f9aca

fix (test): change way to wait for vault theft event

c6c299a

chore: remove unused import

ed16752

fix: simplify vaultsAPI.isFlaggedForTheft, and simplify theft tests

cd97b16

chore: update polkadot js version

9063d9b

chore (test): avoid removing reserved/frozen balances in test prepara…

af50cb2

…tion (escrow tests)

bvotteler marked this pull request as ready for review June 1, 2022 16:17

bvotteler requested review from daniel-savu and gregdhill June 1, 2022 16:25

daniel-savu reviewed Jun 6, 2022

View reviewed changes

daniel-savu approved these changes Jun 6, 2022

View reviewed changes

gregdhill reviewed Jun 6, 2022

View reviewed changes

bvotteler merged commit 2935ca2 into interlay:master Jun 6, 2022

bvotteler deleted the brendon/test-add-multi-currency-vaults branch June 6, 2022 11:47

bvotteler mentioned this pull request Dec 21, 2022

Fix getGriefingCollateral tests #516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multi currency vaults #415

Add multi currency vaults #415

bvotteler commented May 26, 2022 •

edited

Loading

daniel-savu Jun 6, 2022

daniel-savu Jun 6, 2022

bvotteler Jun 6, 2022

daniel-savu Jun 6, 2022

bvotteler Jun 6, 2022 •

edited

Loading

daniel-savu Jun 6, 2022

daniel-savu Jun 6, 2022

bvotteler Jun 6, 2022

daniel-savu commented Jun 6, 2022

daniel-savu left a comment

bvotteler commented Jun 6, 2022

gregdhill left a comment

gregdhill Jun 6, 2022

bvotteler Jun 6, 2022

gregdhill Jun 6, 2022

bvotteler Jun 6, 2022

gregdhill Jun 6, 2022

bvotteler Jun 6, 2022

gregdhill Jun 6, 2022

bvotteler Jun 6, 2022

gregdhill Jun 6, 2022

bvotteler Jun 6, 2022

gregdhill Jun 6, 2022

bvotteler Jun 6, 2022

gregdhill Jun 6, 2022

bvotteler Jun 6, 2022

gregdhill Jun 6, 2022

bvotteler Jun 6, 2022

gregdhill Jun 6, 2022

bvotteler Jun 6, 2022

gregdhill Jun 6, 2022

bvotteler Jun 6, 2022

bvotteler commented Jun 6, 2022

Add multi currency vaults #415

Add multi currency vaults #415

Conversation

bvotteler commented May 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvotteler Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-savu commented Jun 6, 2022

daniel-savu left a comment

Choose a reason for hiding this comment

bvotteler commented Jun 6, 2022

gregdhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvotteler commented Jun 6, 2022

bvotteler commented May 26, 2022 •

edited

Loading

bvotteler Jun 6, 2022 •

edited

Loading