test(a3p): extend test coverage for Vaults on z:acceptance #10297

Jorge-Lopes · 2024-10-18T17:39:49Z

closes: https://github.com/Agoric/BytePitchPartnerEng/issues/22
closes: https://github.com/Agoric/BytePitchPartnerEng/issues/15

Description

The purpose of this PR is to extend the z:acceptance testing coverage of Vaults to include the following cases:

user cannot open a vault above debt limit
user can open a vault under debt limit
user cannot increase vault debt above debt limit
MintFee is applied to users debt when creating a vault and minting more IST
Oracle prices are being received
vaults that existed before the most recent upgrade continue to be useable
user can pay off debt when total debt is above debt limit

The existing tests were also refactored to take advantage of the new helper functions built and also included in this PR.

In addition to extending the test cases, other key update is the introduction of a new script, openVault.js, to open a new vault in the use phase of n:upgrade-next proposal.
This process allow us to test that vaults that existed before the most recent upgrade continue to be useable

A more detailed discussion of the decisions made for the implementations above can be found on the issue: https://github.com/Agoric/BytePitchPartnerEng/issues/22

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

This PR included a new file, /z:acceptance/test-lib/ratio.js, which exports 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 failed because @agoric/xsnap package couldn't be built successfully.
As described in the issue #10259

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

Upgrade Considerations

none

cloudflare-workers-and-pages · 2024-10-18T17:40:47Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`737765d`
Status:	✅ Deploy successful!
Preview URL:	https://c235cbb0.agoric-sdk.pages.dev
Branch Preview URL:	https://jorge-acceptance-vaults-test.agoric-sdk.pages.dev

View logs

dckc · 2024-10-29T16:58:43Z

@Chris-Hibbert it looks like I'm behind on about 3 weeks of context here. So I'll leave this to you. LMK if you want me more involved.

Chris-Hibbert · 2024-10-30T17:55:49Z

a3p-integration/proposals/z:acceptance/test-lib/ratio.js

+ * @param {*} divideOp
+ * @returns {import('@agoric/ertp/src/types.js').Amount<'nat'>}
+ */
+const multiplyHelper = (amount, ratio, divideOp) => {


Why is this duplicated from ratio.js? Why is the rest of this file necessary?

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 failed because @agoric/xsnap package couldn't be built successfully.

This issue describes the current behaviour in more detail (#10259)

I have included this as a Testing Considerations.

a3p-integration/proposals/z:acceptance/test-lib/vaults.js

Chris-Hibbert · 2024-10-30T18:50:23Z

a3p-integration/proposals/z:acceptance/test-lib/vaults.js

+        source: 'continuing',
+      },
+      offerArgs: {
+        deadline,


shouldn't deadline be with respect to the chainTimer, and not Date.now()? It looks like this is pervasive throughout these tests. Do we need a separate issue to refactor these tests to use timeMath and times from the chainTimer?

Is the chainTimer in a3p on wall-clock time?

This function was indeed extracted from an existing test helper file vaults.mts, and I agree with your observation that this Date.now() behavior appears consistently throughout the a3p-integration tests.
It would make sense to open an issue to refactor these tests, provided there is agreement to move toward using chainTimer and timeMath for consistency and accuracy in deadline calculations.

As for whether the chainTimer itself is on wall-clock time, I’m not sure, but I will dig deeper on the subject

cc: @anilhelvaci

Chris-Hibbert · 2024-10-30T18:54:26Z

a3p-integration/proposals/z:acceptance/test-lib/vaults.js

+  const GOV4ADDR = 'agoric1c9gyu460lu70rtcdp95vummd6032psmpdx7wdy';
+  const govAccounts = [GOV1ADDR, GOV2ADDR, GOV4ADDR];


Please import these from some common location.

The Gov4 address is currently declared in multiple locations across a3p-integration. Given its upcoming use in governance, we intend to open a ticket for the next sprint to enable importing Gov4 from synthetic-chain. Once implemented, we’ll refactor the following files to reference the new common location:

a3p-integration/proposals/z:acceptance/governance.test.js

a3p-integration/proposals/z:acceptance/lib/vaults.mts

a3p-integration/proposals/z:acceptance/vaults.test.js

a3p-integration/proposals/n:upgrade-next/agoric-tools.js

In the meantime, would it be possible to proceed with landing this PR as is?

a3p-integration/proposals/z:acceptance/vaults.test.js

Chris-Hibbert · 2024-10-30T22:46:59Z

a3p-integration/proposals/z:acceptance/vaults.test.js

+    /*
+     * The long-living-vault user is being created and used to open a vault in the n:upgrade-next proposal USE phase.
+     * The offer to open a vault is implemented in a3p-integration/proposals/n:upgrade-next/openVault.js
+     */


This is acceptable on an interim basis, but it would better and cleaner to add a vault in the use phase of something in agoric-3-proposals so we don't have to rely on retaining this open vault in n:upgrade-next, which will continuously evolve as we push out releases.

After this PR, would you make a PR that opens a vault there that we can use the next time we need one?

I agree that opening a vault in one of the agoric-3-proposals would be a more robust and sustainable solution.
I'll take on this task and open a ticket to add it to our sprint backlog, ensuring we have a dedicated PR for that work.

Thank you for highlighting this improvement.

rel: Agoric/BytePitchPartnerEng#22 rel: Agoric/BytePitchPartnerEng#15

rel: Agoric/BytePitchPartnerEng#22

rel: Agoric/BytePitchPartnerEng#22 rel: #10259

Jorge-Lopes · 2024-11-05T13:38:18Z

The CI action responsible for running proposal tests failed because the Gov2 wallet didn’t have sufficient funds to cover transaction fees, which prevented it from voting on the proposed change.

While reviewing the governance tests, I noticed that since these tests validate the proper functioning of the economic committee, they should be executed before the vaults tests. Specifically, the "User can pay off debt when totalDebt is above debtLimit" test in vaults.test.js depends on the economic committee's vote to modify a parameter (the debt limit).

To address these issues, the commit ee650c5:

Funds the Gov2 wallet to ensure it can cover transaction fees.
Reorders the execution of governance.test.js to run before vaults.test.js.

Jorge-Lopes requested a review from a team as a code owner October 18, 2024 17:39

Jorge-Lopes requested a review from AgoricTriage October 18, 2024 17:39

Jorge-Lopes force-pushed the jorge/acceptance-vaults-test branch 2 times, most recently from a7c84c7 to 5b7b4d8 Compare October 29, 2024 13:12

Jorge-Lopes requested review from turadg, dckc and Chris-Hibbert October 29, 2024 16:48

dckc removed their request for review October 29, 2024 16:58

Chris-Hibbert requested changes Oct 30, 2024

View reviewed changes

Jorge-Lopes force-pushed the jorge/acceptance-vaults-test branch from e9c8907 to c9a7008 Compare October 31, 2024 10:22

Jorge-Lopes requested a review from Chris-Hibbert October 31, 2024 16:19

Chris-Hibbert approved these changes Oct 31, 2024

View reviewed changes

Jorge-Lopes force-pushed the jorge/acceptance-vaults-test branch from b51397a to 0ff52c0 Compare November 1, 2024 10:46

Jorge-Lopes added the automerge:rebase Automatically rebase updates, then merge label Nov 1, 2024

Jorge-Lopes added 14 commits November 5, 2024 13:06

chore(a3p): open a vault during n:upgrade "use" phase

b037227

rel: Agoric/BytePitchPartnerEng#22 rel: Agoric/BytePitchPartnerEng#15

chore(a3p): add @agoric/ertp to use AmountMath

afdaf25

rel: Agoric/BytePitchPartnerEng#22

chore(a3p): create helper functions for Vaults tests

3cefe08

rel: Agoric/BytePitchPartnerEng#22

test(a3p): extend Vaults test coverage

34785c9

rel: Agoric/BytePitchPartnerEng#22

chore(a3p): open vault at use phase of upgrade

482e2aa

rel: Agoric/BytePitchPartnerEng#22

chore(a3p): include gov4 to update parameters

295ab4d

rel: Agoric/BytePitchPartnerEng#22

test(a3p): update oracle baseId

ed7f5e0

rel: Agoric/BytePitchPartnerEng#22

chore(a3p): refresh yarn.lock after installing dependencies

f297cb6

rel: Agoric/BytePitchPartnerEng#22

fix(a3p): replace @endo/init import

46bb1b6

rel: Agoric/BytePitchPartnerEng#22

chore(a3p): remove unnecessary logs and blank lines

3f64b9f

rel: Agoric/BytePitchPartnerEng#22

fix(a3p): use makeRatio for mintFeeRatio

763280c

rel: Agoric/BytePitchPartnerEng#22

fix(a3p): optimize error messages and reduce logs

177b716

rel: Agoric/BytePitchPartnerEng#22

lint(a3p): fix linting issue at ratio.js

60fb999

fix(a3p): handle Execution Fee while checking user's balance

6c15b9c

rel: Agoric/BytePitchPartnerEng#22

lint(a3p): lint fix vaults.test.js

ef7dfe8

Jorge-Lopes force-pushed the jorge/acceptance-vaults-test branch from 16a1d81 to ef7dfe8 Compare November 5, 2024 13:06

Jorge-Lopes added 2 commits November 5, 2024 13:07

fix(a3p): fund Gov2 wallet and reorder tests

ee650c5

rel: Agoric/BytePitchPartnerEng#22

fix(a3p): implement work around for xsnap package issue #10259

737765d

rel: Agoric/BytePitchPartnerEng#22 rel: #10259

mergify bot merged commit d603b6c into master Nov 5, 2024
81 checks passed

mergify bot deleted the jorge/acceptance-vaults-test branch November 5, 2024 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(a3p): extend test coverage for Vaults on z:acceptance #10297

test(a3p): extend test coverage for Vaults on z:acceptance #10297

Jorge-Lopes commented Oct 18, 2024

cloudflare-workers-and-pages bot commented Oct 18, 2024 •

edited

Loading

dckc commented Oct 29, 2024

Chris-Hibbert Oct 30, 2024

Jorge-Lopes Oct 31, 2024 •

edited

Loading

Chris-Hibbert Oct 30, 2024

Jorge-Lopes Oct 31, 2024

Jorge-Lopes Oct 31, 2024

Chris-Hibbert Oct 30, 2024

Jorge-Lopes Oct 31, 2024 •

edited

Loading

Chris-Hibbert Oct 30, 2024

Jorge-Lopes Oct 31, 2024

Jorge-Lopes commented Nov 5, 2024

		const GOV4ADDR = 'agoric1c9gyu460lu70rtcdp95vummd6032psmpdx7wdy';
		const govAccounts = [GOV1ADDR, GOV2ADDR, GOV4ADDR];

test(a3p): extend test coverage for Vaults on z:acceptance #10297

test(a3p): extend test coverage for Vaults on z:acceptance #10297

Conversation

Jorge-Lopes commented Oct 18, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Oct 18, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

dckc commented Oct 29, 2024

Chris-Hibbert Oct 30, 2024

Choose a reason for hiding this comment

Jorge-Lopes Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Chris-Hibbert Oct 30, 2024

Choose a reason for hiding this comment

Jorge-Lopes Oct 31, 2024

Choose a reason for hiding this comment

Jorge-Lopes Oct 31, 2024

Choose a reason for hiding this comment

Chris-Hibbert Oct 30, 2024

Choose a reason for hiding this comment

Jorge-Lopes Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Chris-Hibbert Oct 30, 2024

Choose a reason for hiding this comment

Jorge-Lopes Oct 31, 2024

Choose a reason for hiding this comment

Jorge-Lopes commented Nov 5, 2024

cloudflare-workers-and-pages bot commented Oct 18, 2024 •

edited

Loading

Jorge-Lopes Oct 31, 2024 •

edited

Loading

Jorge-Lopes Oct 31, 2024 •

edited

Loading