8981 upgrade vaults #9283

Chris-Hibbert · 2024-04-24T20:54:43Z

closes: #8049
closes: #8740
closes: #8868
closes: #8918
closes: #8981
closes: #8079
refs: #8400
closes: #8735
closes: #7873
closes: #8726
closes: #7954
closes: #8757
closes: #8728
closes: #8789

Description

Upgrade VaultFactory in A3P, relying on the new PriceFeeds, and auctions. The actual upgrade waits for the priceFeeds to start supplying before doing the upgrade, so there won't be any gap in priceUpdates.

When the upgrade is finished, we also update the auctioneerKit and Auction instance in the bootstrap environment.

This PR demonstrates that VaultFactory can be upgraded even though governance is not persistent (#8123).

Security Considerations

N/A

Scaling Considerations

This is largely in service of #8400, which reports that priceFeed vats are accumulating garbage. This PR switches to new priceFeeds, which won't have that problem, though cleaning up the existing vats is a task for the future.

Documentation Considerations

No changes to user-visible behavior.

Testing Considerations

A3P tests that verify that vaultFactory has been upgraded, that a new Auctioneer is running and is receiving prices. Verify that when prices drop, assets are sold via the auction, the bidder gets the proceeds, and the vaults are liquidated or reconstituted appropriately.

Upgrade Considerations

Upgrade all the vats!

cloudflare-workers-and-pages · 2024-04-24T21:19:12Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`e9affc8`
Status:	✅ Deploy successful!
Preview URL:	https://54cba879.agoric-sdk.pages.dev
Branch Preview URL:	https://8981-upgradevaults.agoric-sdk.pages.dev

View logs

turadg · 2024-04-25T14:16:26Z

a3p-integration/proposals/a:upgrade-next/agd-tools.js

+
+export const BID_OFFER_ID = 'bid-vaultUpgrade-test3';
+
+const agdQuery = path =>


easy to confuse with existing agd.query. it's not exported so low risk but consider renaming. e.g. queryVstorage

turadg · 2024-04-25T14:17:54Z

a3p-integration/proposals/a:upgrade-next/agd-tools.js

+  const body = JSON.parse(JSON.parse(queryout.value).values[0]);
+  return JSON.parse(body.body.substring(1));


don't we have real SmallCaps decoding now? consider importing from @endo/marshal

That didn't turn out to be straightforward. I left a XXX comment for later.

turadg · 2024-04-25T14:20:29Z

a3p-integration/proposals/a:upgrade-next/test.sh

 GLOBIGNORE=post.test.js
+
+yarn ava post.test.js


why move? I think this is less clear

I went through intermediate stages where there were other elements in GLOBIGNORE, and I won't be surprised if we get there again.

In those cases, it was clear that it should be defined at the top before all the yarn commands, rather than just before one of the lines that uses globs.

turadg · 2024-04-29T21:20:41Z

a3p-integration/proposals/a:upgrade-next/upgradeVaults.test.js

+} from './agd-tools.js';
+import { getDetailsMatchingVats } from './vatDetails.js';
+
+test.serial('check all priceFeed vats updated', async t => {


Ava serial happens to run sequentially but is isn't guaranteed too. I think we should try to avoid using it whenever possible.

Consider putting the sequence into one test with t.log to provide the progress messages. And consider helper like pushPrices(t) to abstract the logic so it's easy to follow what the test is doing and checking.

I switched to a single test driver that invokes several functions, separated by t.log().

And consider helper like pushPrices(t) to abstract the logic
I didn't understand this. There's already a pushPrices(), though it taked different params. Did you think there were other obvious helpers to extract?

turadg · 2024-04-29T21:24:24Z

a3p-integration/proposals/a:upgrade-next/vatDetails.js

+};
+
+/** @param {string} vatName */
+export const getDetailsMatchingVats = async vatName => {


please comment on what we need @agoric/synthetic-chain before this module can be replaced by one of its exports. consider linking to an issue or PR once can reference to see whether the work is done

Hmm. This file was added in #9158. I must need to rebase.

But the request is good. I'll write up an issue.

turadg · 2024-04-29T21:54:07Z

packages/inter-protocol/src/vaultFactory/params.js

+ * @param {Record<string, VaultManagerParams>} managerParamValues - sets of
+ *   parameters (plus brand:) keyed by Keyword. override stored initial values


Suggested change

* @param {Record<string, VaultManagerParams>} managerParamValues - sets of

* parameters (plus brand:) keyed by Keyword. override stored initial values

* @param {Record<string, VaultManagerParamValues & { collateralBrand: Brand }>} managerParamOverrides

Oh, yes! that's better. Thanks.

packages/inter-protocol/src/vaultFactory/params.js

turadg · 2024-04-29T22:03:46Z

packages/inter-protocol/src/vaultFactory/vaultDirector.js

@@ -437,6 +447,12 @@ const prepareVaultDirector = (
            allManagersDo(vm => vm.lockOraclePrices());
          });
        },
+        async updateShortfallReporter(newInvitation) {


updateShortfallReporter has another meaning in this same module. please rename. consider setShortfallReporter.

good catch! done.

packages/inter-protocol/src/vaultFactory/vaultManager.js

turadg · 2024-05-01T22:58:43Z

a3p-integration/proposals/a:upgrade-next/upgradeVaults.test.js

+// test.serial() isn't guaranteed to run tests in order, so we cobble together a driver
+test('driver', async t => {


this module has helpers functions, but not a "driver" object.

it's also not testing the driver.

Suggested change

// test.serial() isn't guaranteed to run tests in order, so we cobble together a driver

test('driver', async t => {

test('auction after upgrade', async t => {

turadg · 2024-05-01T23:04:29Z

packages/builders/scripts/vats/upgradeVaults.js

+/** @type {import('@agoric/deploy-script-support/src/externalTypes.js').ProposalBuilder} */
+export const defaultProposalBuilder = async ({ publishRef, install }) =>
+  harden({
+    sourceSpec: '@agoric/inter-protocol/src/proposals/upgrade-vaults.js',


It's annoying to have multiple files with the same name.

Like package.json, types.js and typeGuards.js? :-P

I think it's more confusing that it's not clear which proposal maps to which script. Maybe we should move to having a single module that provides both aspects; they're rarely separate.

Regardless, we should avoid camelCase in filenames because of the problem for case insensitive file systems. But I won't block on it.

turadg · 2024-05-01T23:05:46Z

packages/inter-protocol/src/vaultFactory/params.js

@@ -1,5 +1,3 @@
-// @jessie-check


My understanding from @erights is that for all code that runs in vats we should try to constrain to Jessie-safe

Chris-Hibbert · 2024-05-02T16:17:31Z

I'm still getting this in three tests.

 error TS9006: Declaration emit for this file requires using private name 'StartFunction' from module '"/home/runner/work/agoric-sdk/agoric-sdk/packages/zoe/src/zoeService/utils"'. An explicit type annotation may unblock declaration emit.

I'd appreciate help figuring out how to address this.

turadg · 2024-05-02T16:27:29Z

packages/inter-protocol/src/vaultFactory/params.js

-    );
+    const newInitial = managerParamOverrides
+      ? Object.values(managerParamOverrides).find(e => e.brand === brand)
+      : undefined;


isn't this what find() returns when nothing matches the predicate?

Object.values() is unhappy when managerParamOverrides is undefined.

ah. I'm surprised it's ever undefined because the function signature says it's Record<string, VaultManagerParamOverrides>. If optional please give it a default of {} instead.

refs: #9283 ## Description #9283 was [failing integration](https://github.com/Agoric/agoric-sdk/actions/runs/8927087379/job/24519667094?pr=9283) with, ``` Error: src/proposals/econ-behaviors.js(4,1): error TS9006: Declaration emit for this file requires using private name 'StartFunction' from module '"/home/runner/work/agoric-sdk/agoric-sdk/packages/zoe/src/zoeService/utils"'. An explicit type annotation may unblock declaration emit. ``` I've been meaning to adopt the `Tagged` helper from types-fest so I took this opportunity to try it and it solved this problem. So this PR adds it to `@agoric/internal` and uses it for the `Installation` and `Instance` types that need a tag. It doesn't use if for the three other `declare const` we have in the repo. While debugging I also noticed we could cut a few ambient runtime imports so I did that. One required moving ManualTimer type into its impl module. ### Security Considerations I vendored the file instead of importing from NPM to not take the external dep  ### Scaling Considerations n/a, types  ### Documentation Considerations should not be developer-facing  ### Testing Considerations CI  ### Upgrade Considerations n/a, types

…ead of terms

migrate from taking Auction's publicFacet in terms to taking the instance in privateArgs Allow Parameter values to be overridden on restart repair some erroneous guards Deal with reconstituted invitations Governance: Allow the electorate invitation to change on upgrade simplify and speed up invitation validation

attempt to address error Declaration emit for this file requires using private name 'StartFunction' from module '"/Users/chris/agoric-sdk/packages/zoe/src/zoeService/utils"'. An explicit type annotation may unblock declaration emit

improve naming and type decl of VaultManagerParamOverrides replace an ugly for loop with a nice Object.values(...).find(...). remove name re-use (updateShortfallReporter) renamings for clarity remove outdated comments don't rely on test.serial() to run tests in order

refs: #8400 refs: #8498 closes: #9371 ## Description #8400 reported that the priceFeed vats hold onto old quotes in their recoverySet, preventing them from being collected. #9283 applied the fixes to master. These fixes address the growth in priceFeed vats and Zoe, but scaledPriceAuthorities were still growing. This resolves that problem by upgrading scaledPriceAuthority contracts to not use their recoverySets. <details> <summary><b>Expand</b> for performance graphs</summary> <img width="809" alt="image" src="https://github.com/Agoric/agoric-sdk/assets/13372636/889bb283-89c8-434f-8a67-efa56d0382ad"> Kernel allocation is in blue, and the scale is on the left. It varies from 48862 to 49743, with a small amount of long-term growth.The other active vats (v9=Zoe, v29=ATOM-USD_priceFeed, v43=wallet, 72=ATOM-USD_priceFeed, 74=auctioneer) use the scale on the right, with Zoe varying tightly around 3600, and the others low and stable. scaledPriceAuthority-ATOM doesn't have enough variation to be worth graphing. </details> ### Security Considerations Upgrade existing contracts. No new vulnerabilities. ### Scaling Considerations This addresses the largest known category of growth on the chain. ### Documentation Considerations Add some documentation on creating proposals. ### Testing Considerations Tested in A3P. We should exercise all the clients of priceFeeds in our test environments. ### Upgrade Considerations This PR includes a proposal that will upgrade all vats with `scaledPriceAuthority` in their label. That should work on or test chains as well as MainNet. These changes should be included in the next release.

refs: #9382 refs: #9584 ## Description Add a test that was supposed to be in #9283, where it says > A3P tests that verify that vaultFactory has been upgraded, that a new Auctioneer is running and is receiving prices. Verify that when prices drop, assets are sold via the auction, the bidder gets the proceeds, and the vaults are liquidated or reconstituted appropriately. It was too hard to verify the results of the auction, because of the timing of vault liquidations and auction runs, so the actual check was dropped. The subsequent PR (#9371) that upgraded scaledPriceAuthorities seems to have broken the upgrade, and the missing test failed to warn us. Here we test that vaultFactory was actually upgraded by verifying that it's getting prices from the new price feeds, and drop the upgrade of scaledPriceAuthority until we can figure out how to make that upgrade compatible. ### Security Considerations Not relevant ### Scaling Considerations Drops the upgrade of scaledPriceAuthority, which fixed part of the memory growth. This was the smaller portion of the growth, so it's more important to get the rest of the fixes in than to also include this. ### Documentation Considerations None. ### Testing Considerations Replaces a missing test. ### Upgrade Considerations Repairs upgrade.

Chris-Hibbert added test contract-upgrade Vaults VaultFactor (née Treasury) auction labels Apr 24, 2024

Chris-Hibbert self-assigned this Apr 24, 2024

Chris-Hibbert changed the base branch from master to 8740-newAuction April 24, 2024 21:05

Chris-Hibbert added the force:integration Force integration tests to run on PR label Apr 24, 2024

Chris-Hibbert force-pushed the 8981-upgradeVaults branch from d932480 to 37c6b27 Compare April 24, 2024 21:19

Chris-Hibbert force-pushed the 8981-upgradeVaults branch 2 times, most recently from 30cde92 to b863e16 Compare April 24, 2024 21:43

Chris-Hibbert requested a review from turadg April 25, 2024 00:13

Chris-Hibbert mentioned this pull request Apr 25, 2024

Start a new auction in a3p-integration #9158

Merged

Chris-Hibbert force-pushed the 8740-newAuction branch 2 times, most recently from 3774694 to 2d5a66a Compare April 29, 2024 18:02

Base automatically changed from 8740-newAuction to master April 29, 2024 18:36

This was referenced Apr 29, 2024

vaultFactory contract not upgradable #8079

Closed

vat-vaultFactory may believe governance is broken when vat-auctioneer is upgraded #8726

Closed

turadg requested changes Apr 29, 2024

View reviewed changes

Chris-Hibbert force-pushed the 8981-upgradeVaults branch from 5dd44ff to 31fb602 Compare April 30, 2024 20:23

turadg approved these changes May 1, 2024

View reviewed changes

turadg reviewed May 2, 2024

View reviewed changes

turadg mentioned this pull request May 2, 2024

Tagged helper for StartFunction #9315

Merged

Chris-Hibbert force-pushed the 8981-upgradeVaults branch from 64ef197 to 2859394 Compare May 3, 2024 00:46

Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label May 6, 2024

iomekam and others added 3 commits May 6, 2024 17:15

fix! vault factory takes auctioneerPublicFacet from private args inst…

d1b1b44

…ead of terms

test: verify that vaults and auctions upgrade correctly

d2b84ee

Chris-Hibbert added 5 commits May 6, 2024 17:15

test: drop obsolete deployment upgrade-test

f2d2378

chore: typescript magic

5dcd362

attempt to address error Declaration emit for this file requires using private name 'StartFunction' from module '"/Users/chris/agoric-sdk/packages/zoe/src/zoeService/utils"'. An explicit type annotation may unblock declaration emit

test: repair some tests

cadd1b2

chore: update yarn.lock and comments

ee016c0

Chris-Hibbert force-pushed the 8981-upgradeVaults branch from 636e81a to ee016c0 Compare May 6, 2024 17:15

chore: re-fix reference path

e9affc8

mergify bot merged commit 24f7f32 into master May 6, 2024
63 checks passed

mergify bot deleted the 8981-upgradeVaults branch May 6, 2024 18:46

Chris-Hibbert mentioned this pull request Jun 26, 2024

Remove scaled price authority upgrade #9585

Merged

LuqiPan mentioned this pull request Aug 13, 2024

WIP: in VaultFactory, move auctioneer public facet from terms to private args #8049

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8981 upgrade vaults #9283

8981 upgrade vaults #9283

Chris-Hibbert commented Apr 24, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Apr 24, 2024 •

edited

Loading

turadg Apr 25, 2024

Chris-Hibbert Apr 30, 2024

turadg Apr 25, 2024

Chris-Hibbert Apr 30, 2024

turadg Apr 25, 2024

Chris-Hibbert Apr 30, 2024

turadg Apr 29, 2024

Chris-Hibbert May 1, 2024

turadg Apr 29, 2024

Chris-Hibbert Apr 30, 2024

turadg Apr 29, 2024

Chris-Hibbert May 1, 2024

turadg Apr 29, 2024

Chris-Hibbert May 1, 2024

turadg May 1, 2024

Chris-Hibbert May 2, 2024

turadg May 1, 2024

turadg May 1, 2024

Chris-Hibbert commented May 2, 2024

turadg May 2, 2024

Chris-Hibbert May 2, 2024

turadg May 2, 2024


		export const BID_OFFER_ID = 'bid-vaultUpgrade-test3';

		const agdQuery = path =>

		const body = JSON.parse(JSON.parse(queryout.value).values[0]);
		return JSON.parse(body.body.substring(1));

		* @param {Record<string, VaultManagerParams>} managerParamValues - sets of
		* parameters (plus brand:) keyed by Keyword. override stored initial values

		// test.serial() isn't guaranteed to run tests in order, so we cobble together a driver
		test('driver', async t => {

	// test.serial() isn't guaranteed to run tests in order, so we cobble together a driver
	test('driver', async t => {
	test('auction after upgrade', async t => {

8981 upgrade vaults #9283

8981 upgrade vaults #9283

Conversation

Chris-Hibbert commented Apr 24, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Apr 24, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

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

Chris-Hibbert commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert commented Apr 24, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Apr 24, 2024 •

edited

Loading