Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8981 upgrade vaults #9283

Merged
merged 9 commits into from
May 6, 2024
Merged

8981 upgrade vaults #9283

merged 9 commits into from
May 6, 2024

Conversation

Chris-Hibbert
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert commented Apr 24, 2024

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!

@Chris-Hibbert Chris-Hibbert self-assigned this Apr 24, 2024
@Chris-Hibbert Chris-Hibbert changed the base branch from master to 8740-newAuction April 24, 2024 21:05
@Chris-Hibbert Chris-Hibbert added the force:integration Force integration tests to run on PR label Apr 24, 2024
Copy link

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

Deploying agoric-sdk with  Cloudflare Pages  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

@Chris-Hibbert Chris-Hibbert force-pushed the 8981-upgradeVaults branch 2 times, most recently from 30cde92 to b863e16 Compare April 24, 2024 21:43
@Chris-Hibbert Chris-Hibbert requested a review from turadg April 25, 2024 00:13
@Chris-Hibbert 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

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

const agdQuery = path =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 25 to 27
const body = JSON.parse(JSON.parse(queryout.value).values[0]);
return JSON.parse(body.body.substring(1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

GLOBIGNORE=post.test.js

yarn ava post.test.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why move? I think this is less clear

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} from './agd-tools.js';
import { getDetailsMatchingVats } from './vatDetails.js';

test.serial('check all priceFeed vats updated', async t => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

};

/** @param {string} vatName */
export const getDetailsMatchingVats = async vatName => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Comment on lines 188 to 189
* @param {Record<string, VaultManagerParams>} managerParamValues - sets of
* parameters (plus brand:) keyed by Keyword. override stored initial values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes! that's better. Thanks.

packages/inter-protocol/src/vaultFactory/params.js Outdated Show resolved Hide resolved
packages/inter-protocol/src/vaultFactory/params.js Outdated Show resolved Hide resolved
@@ -437,6 +447,12 @@ const prepareVaultDirector = (
allManagersDo(vm => vm.lockOraclePrices());
});
},
async updateShortfallReporter(newInvitation) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! done.

packages/inter-protocol/src/vaultFactory/vaultManager.js Outdated Show resolved Hide resolved
Comment on lines 110 to 111
// test.serial() isn't guaranteed to run tests in order, so we cobble together a driver
test('driver', async t => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed

/** @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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -1,5 +1,3 @@
// @jessie-check
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Chris-Hibbert
Copy link
Contributor Author

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.

);
const newInitial = managerParamOverrides
? Object.values(managerParamOverrides).find(e => e.brand === brand)
: undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mergify bot added a commit that referenced this pull request May 2, 2024
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

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations
n/a, types
<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations
should not be developer-facing
<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations
CI

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

### Upgrade Considerations
n/a, types
<!-- What aspects of this PR are relevant to upgrading live production
systems, and how should they be addressed? -->
@Chris-Hibbert 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
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
@mergify mergify bot merged commit 24f7f32 into master May 6, 2024
63 checks passed
@mergify mergify bot deleted the 8981-upgradeVaults branch May 6, 2024 18:46
mergify bot added a commit that referenced this pull request May 24, 2024
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.
mergify bot added a commit that referenced this pull request Jun 26, 2024
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.
mhofman pushed a commit that referenced this pull request Jun 26, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment