Jorge-Lopes commented 2 weeks ago

Description

Extend z:acceptance testing coverage of Vaults according to the functionalities identified on this document

Note: Some functionalities are already covered by existing test cases, so the ones listed below are the remaining gaps that need to be addressed.

### Tasks
- [x] Refactor existing tests with new helper functions
- [x] Test that user cannot open a vault above debt limit
- [x] Test that user can open a vault under debt limit
- [x] Test that user cannot increased vault debt above debt limit
- [x] Test that MintFee is applied to users debt when creating a vault and minting more IST
- [x] Test that Oracle prices are being received
- [x] Test that vaults that existed before the most recent upgrade continue to be useable
- [x] Test that user can pay off debt when totalDebt is above debtLimit
- [ ] Test that Stability fee is accrued to user’s debt over time
- [ ] Test that vault is liquidated when collateralization ratio =< liquidation ratio
- [ ] Test that liquidated assets are sent to auctioneer upon liquidation
- [x] Open PR to master
- [ ] Address PR requested changes
- [ ] Land PR

Jorge-Lopes commented 1 week ago

At commit 0df206e a file https://github.com/Agoric/agoric-sdk/issues/10259 a file ratio.js is created.

The purpose of this file is to export the method ceilMultiplyBy which is also exported by the @agoric/zoe package. The reason why this file was created was because when trying to install the @agoric/zoe package at the a3p-integration z:acceptance proposal, it fails due to the fact that @agoric/xsnap package couldn't be built successfully.

As described in the following issue: https://github.com/Agoric/agoric-sdk/issues/10259ƒ

When resolved, the ratio.js file should be removed and the ceilMultiplyBy method should be imported from the @agoric/zoe package.

Jorge-Lopes commented 1 week ago

At the Agoric/Inter Protocol App Acceptance Tests document, a list of test are identified as Vaults test cases.

From that list, the following tests are addressed on the auction tests:

The auctions are running as scheduled (and per governed auction parameters)
The auctioneer is able to sell liquidated collateral to bidders
Bidders are able to place a bid for liquidate collateral (by price and by discount)
Bids are awarded and bidder receives bought collateral (offer is filled)

For that reason are not included in the vaults-test.js.

Jorge-Lopes commented 1 week ago

Rel: https://github.com/Agoric/BytePitchPartnerEng/issues/15

Context:

In order to test that vaults created prior to the most recent system upgrade remain functional, I included an offer to open a vault is submitted during the use phase of the n:upgrade-next proposal. This implementation corresponds to commit 7ec2d31 and the test can be seen here

Problem:

While conducting this test, an unexpected error occurred while building the submissions:

Error#3: maxDebtFor called before a collateral quote was available for Object [Alleged: ATOM brand] {}

This error did not appear before commit fd91f78, which includes the f:replace-price-feeds proposal as part of the a3p-integration. The error seems related to the availability of a price feed for ATOM collateral.

I examined the commits and noticed that in vaults.test.js, a change was made at commit eb941d4 to prevent an error to be triggered when prices were not available.

I have then decided to incorporate this solution into the openVault.js, solving the issue and restoring the expected behavior.

Question:

Given that the use phase of n:upgrade-next always precedes the z:acceptance test phase, I concluded that repeating the instruction to push prices for ATOM in vaults.test.js would not be necessary.

Was my assumption correct?

Error logs:

23.50 2024-10-11T13:31:19.611Z SwingSet: vat: v43: walletFactory.fromBridge: { blockHeight: 1322, blockTime: 1728653478, owner: 'agoric1k7anzwjg7w57ltdg9tvypjgs48v6mdk7cqwt0k', spendAction: '{"body":"#{\\"method\\":\\"executeOffer\\",\\"offer\\":{\\"id\\":\\"openVault-1728653478639\\",\\"invitationSpec\\":{\\"callPipe\\":[[\\"getCollateralManager\\",[\\"$0.Alleged: BoardRemoteATOM brand\\"]],[\\"makeVaultInvitation\\"]],\\"instancePath\\":[\\"VaultFactory\\"],\\"source\\":\\"agoricContract\\"},\\"proposal\\":{\\"give\\":{\\"Collateral\\":{\\"brand\\":\\"$0\\",\\"value\\":\\"+10000000\\"}},\\"want\\":{\\"Minted\\":{\\"brand\\":\\"$1.Alleged: BoardRemoteIST brand\\",\\"value\\":\\"+5000000\\"}}}}}","slots":["board05557","board0257"]}', type: 'WALLET_SPEND_ACTION' }
23.50 2024-10-11T13:31:19.612Z SwingSet: vat: v43: walletFactory: { wallet: Object [Alleged: SmartWallet self] {}, actionCapData: { body: '#{"method":"executeOffer","offer":{"id":"openVault-1728653478639","invitationSpec":{"callPipe":[["getCollateralManager",["$0.Alleged: BoardRemoteATOM brand"]],["makeVaultInvitation"]],"instancePath":["VaultFactory"],"source":"agoricContract"},"proposal":{"give":{"Collateral":{"brand":"$0","value":"+10000000"}},"want":{"Minted":{"brand":"$1.Alleged: BoardRemoteIST brand","value":"+5000000"}}}}}', slots: [ 'board05557', 'board0257' ] } }
23.52 2024-10-11T13:31:19.634Z SwingSet: vat: v43: wallet agoric1k7anzwjg7w57ltdg9tvypjgs48v6mdk7cqwt0k starting executeOffer openVault-1728653478639
24.07 2024-10-11T13:31:20.176Z SwingSet: vat: v48: ----- VM.15  22 Object [Alleged: ATOM brand] {} makeVaultKit
24.07 2024-10-11T13:31:20.181Z SwingSet: vat: v43: wallet agoric1k7anzwjg7w57ltdg9tvypjgs48v6mdk7cqwt0k openVault-1728653478639 seated
24.15 2024-10-11T13:31:20.260Z SwingSet: vat: v48: ----- VM.15  23 Object [Alleged: ATOM brand] {} makeVaultKit made vault Object [Alleged: Vault self] {}
24.15 2024-10-11T13:31:20.264Z SwingSet: vat: v48: ----- Vault.10  2 initVaultKit start: collateral 10 { actualDebtPre: { brand: Object [Alleged: IST brand] {}, value: 0n }, collateralPre: { brand: Object [Alleged: ATOM brand] {}, value: 0n } }
24.16 2024-10-11T13:31:20.267Z SwingSet: vat: v48: ----- Vault.10  3 initVault 10 { wantedRun: { brand: Object [Alleged: IST brand] {}, value: 5_000_000n }, fee: { brand: Object [Alleged: IST brand] {}, value: 25_000n } } { brand: Object [Alleged: ATOM brand] {}, value: 0n }
24.16 2024-10-11T13:31:20.270Z SwingSet: vat: v48: ----- VM.15  24 helper.start() awaiting observe storedQuotesNotifier Object [Alleged: ATOM brand] {}
24.16 2024-10-11T13:31:20.272Z SwingSet: vat: v48: attempting recovery after initVaultKit failure (Error#3)
24.16 2024-10-11T13:31:20.272Z SwingSet: vat: v48: Error#3: maxDebtFor called before a collateral quote was available for Object [Alleged: ATOM brand] {}
24.16 2024-10-11T13:31:20.272Z SwingSet: vat: v48: Error: maxDebtFor called before a collateral quote was available for (an object)

Jorge-Lopes commented 1 week ago

At the Agoric/Inter Protocol App Acceptance Tests document, one of the identified Vaults test cases is to Test that Oracle prices are being received.

A comment made by Dan is if does it suffice to test that "inter protocol provides prices"?

I would like to confirm if that is the case, should the test simply verify if a value is being returned by Vstorage, exposed by the node published.vaultFactory.managers.managerX.quotes?

Jorge-Lopes commented 1 week ago

At the Agoric/Inter Protocol App Acceptance Tests document, a list of test are identified as Vaults test cases.

From that list, the following tests are temporary excluded due to its time constraint nature, making it easier to implement when the time advancing tool is available:

Stability fee is accrued to user’s debt over time
Vault is liquidated when collateralization ratio =< liquidation ratio
Liquidated assets are sent to auctioneer upon liquidation

Justification

Stability fee is accrued to user’s debt over time

The chargeAllVaults method of vaultManager is responsible to increment the debt of each vault accordingly to the InterestRate, which can be seen on the governance node of each vault manager. This process is executed repeatedly at times that are a multiple of the the ChargingPeriod, which can be seen on the governance node of vaultFactory.

Considering this constraint, we need to identify the vault debt at a certain time, advance a x number of cycles (ChargingPeriod), and then calculate if the vault debt InterestRate has increment of x * InterestRate.

Vault is liquidated

ToDo: add description of time constraint for vault liquidation

otoole-brendan commented 1 week ago

At the Agoric/Inter Protocol App Acceptance Tests document, one of the identified Vaults test cases is to Test that Oracle prices are being received.

A comment made by Dan is if does it suffice to test that "inter protocol provides prices"?

I would like to confirm if that is the case, should the test simply verify if a value is being returned by Vstorage, exposed by the node published.vaultFactory.managers.managerX.quotes?

@Jorge-Lopes that's right - I updated the doc. And yes - if vstorage shows the quote then that is a sufficient test

Chris-Hibbert commented 1 week ago

I would like to confirm if that is the case, should the test simply verify if a value is being returned by Vstorage, exposed by the node published.vaultFactory.managers.managerX.quotes?

Unfortunately, no. published.vaultFactory.managers.managerX.quotes will have a quote if a previous incarnation of the vault (before a recent upgrade) received an update from a priceFeed. When f:replace-price-feeds upgrades the auctions and vaults, the upgraded vaultFactory no longer holds a price quote, and it won't until the new priceFeeds start receiving quotes.

Question: Given that the use phase of n:upgrade-next always precedes the z:acceptance test phase, I concluded that repeating the instruction to push prices for ATOM in vaults.test.js would net be necessary.

A more robust solution would be to put it in the use phase of f:replace-price-feeds. I should have put it there myself rather than adding it to the test. If tests get rearranged and we change our minds about which proposals to include in upgrade 18, the robust solution is to start feeding prices in the use phase of f:replace-price-feeds, as that will move with the proposal that is causing the priceFeeds to be disrupted.

Jorge-Lopes commented 1 week ago

@Chris-Hibbert Thank you for your feedback.

I followed your suggestion and added a script to start feeding prices for Atom and stAtom during the use phase of f:replace-price-feeds. You can see this change in commit 4d4c517.

To accommodate this update, I also modified f:replace-price-feeds/priceFeedUpdate.test.js (see commit 3d1c147) and n:upgrade-next/openVault.js (see commit 2082c52).

The test verifying Oracle price reception was updated accordingly to push a new price and check if Vstorage reflects the updated value (see commit 38b10a3).

Please can you confirm if these improvements are aligned to what you expected?

Thank you in advance.

Chris-Hibbert commented 1 week ago

In 3d1c, I think we should re-set the roundId by reading from vstorage. Otherwise, this is brittle, and others will need to update it anytime they insert a price-updating proposal before f:.

Chris-Hibbert commented 1 week ago

[I can't review directly in those commits. Are they part of a draft PR I could comment on?]

In 38b1

  const vaultQuotePath = `published.vaultFactory.managers.${VAULT_MANAGER}.quotes`;
  const vaultQuoteBody = await getContractInfo(vaultQuotePath, {
    agoric,
    prefix: '',
  });
  const vaultQuote = vaultQuoteBody.quoteAmount.value[0].amountOut.value;

should use getVaultPrices from synthetic-chain. In general, looking up data in vstorage should use tools in synthetic-chain. If they don't exist, please add them there.

Chris-Hibbert commented 1 week ago

On 4d4c517, I'd prefer that pushPrice took a single brand and price as parameters. I'm not sure how to pass parameters from a shell script (use.sh) directly to javascript, so it may be necessary to write pushPrices.js, which would setup oracles etc. and call

  verifyPushedPrice(oraclesByBrand, 'ATOM', 12.01, ROUND_ID);
  verifyPushedPrice(oraclesByBrand, 'stATOM', 12.02, ROUND_ID);

When watching logs to see what is happening with price updates, it's extremely useful if every different price update of a separate currency updates to a different price.

Jorge-Lopes commented 1 week ago

Greetings @Chris-Hibbert

[I can't review directly in those commits. Are they part of a draft PR I could comment on?]

I have opened a PR to make it easier for review. Link: https://github.com/Agoric/agoric-sdk/pull/10280

On 4d4c517, I'd prefer that pushPrice took a single brand and price as parameters. I'm not sure how to pass parameters from a shell script (use.sh) directly to javascript, so it may be necessary to write pushPrices.js, which would setup oracles etc.

I have followed your suggestion and updated pushPrices.js to receive a single brand and price, passed via the use script. As well as fetching the priceFeed roundId from Vstorage.

  verifyPushedPrice(oraclesByBrand, 'ATOM', 12.01, ROUND_ID);
  verifyPushedPrice(oraclesByBrand, 'stATOM', 12.02, ROUND_ID);
When watching logs to see what is happening with price updates, it's extremely useful if every different price update of a separate currency updates to a different price.

Sorry but I’m a bit unclear about the purpose and implementation of the verifyPushedPrice function in this context. Currently, pushPrices.js uses retryUntilCondition to ensure price updates occur, and it logs the priceFeed lastRound in each cycle.

Could you please clarify your suggestion regarding verifyPushedPrice?

Chris-Hibbert commented 1 week ago

Could you please clarify your suggestion regarding verifyPushedPrice? It was intended as an improved name for an operation that calls pushPrice and then uses retryUntilCondition to verify that the price change took effect. pushPrices sounds like it just calls pushPrice in a loop, but the function and file do more than that.

Jorge-Lopes commented 1 week ago

It was intended as an improved name for an operation that calls pushPrice and then uses retryUntilCondition to verify that the price change took effect. pushPrices sounds like it just calls pushPrice in a loop, but the function and file do more than that.

Thank you for the clarification. I implemented your suggestion as you can see from the commits d1f2860 and e4ee364

Agoric / BytePitchPartnerEng

Extend `Vaults` testing coverage #22

Description

Context:

Problem:

Question:

Error logs:

Justification

Stability fee is accrued to user’s debt over time

Vault is liquidated