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

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

Closed
warner opened this issue Jan 9, 2024 · 4 comments · Fixed by #9283
Closed

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

warner opened this issue Jan 9, 2024 · 4 comments · Fixed by #9283
Assignees
Labels
bug Something isn't working contract-upgrade Inter-protocol Overarching Inter Protocol Vaults VaultFactor (née Treasury)

Comments

@warner
Copy link
Member

warner commented Jan 9, 2024

As part of #8499 (comment) (look for v45-auctioneer-v48-vaultfactory), we observe that v48-vaultFactory follows (is a subscriber to) a governance-parameter notifier that lives in v45-auctioneer. The v45-auctioneer's @agoric/governance code is not durable, so when v45-auctioneer is upgraded, that notifier will be deleted/abandoned, and v48-vaultFactory may believe that governance has broken.

The subscriber code is probably set up here: https://github.com/Agoric/agoric-sdk/blob/release-mainnet1B/packages/inter-protocol/src/vaultFactory/liquidation.js#L211 . It uses observeIteration, so if v45-auctioneer's governance objects were durable, this would be safe against v45-auctioneer being upgraded. And it looks like this is called from a start method that will be called on each upgrade of v48-vaultFactory, so it should be safe against v48-vaultFactory upgrades.

To address this, I think the subscribeEach needs some sort of error-handling case, which can notice that the promise was disconnected (and perhaps check that the incarnation number is new), and react by requesting a new subscription from the (hopefully durable) auctioneerPublicFacet.

(#8079 might overlap with this, but is mainly about v48-vaultFactory not yet being upgradable, rather than what additional safeguards it needs against other vats being upgraded)

The tasks are:

  • change watchForGovernanceChange in v48-vaultFactory to tolerate upgrades of the upstream v45-auctioneer
  • demonstrate (via a3p?) upgrading v45-auctioneer, then making a governance change, then observing that v48-vaultFactory correctly notices the governance change

We must deploy the v48-vaultFactory fix before we can safely upgrade v45-auctioneer.

@warner warner added bug Something isn't working Inter-protocol Overarching Inter Protocol contract-upgrade labels Jan 9, 2024
@aj-agoric aj-agoric added the Vaults VaultFactor (née Treasury) label Jan 9, 2024
@dckc
Copy link
Member

dckc commented Jan 9, 2024

@Chris-Hibbert
Copy link
Contributor

The auctioneer isn't designed to be upgraded. The plan is to replace it with a new instance. This means the steps are

It might still be helpful to enhance watchForGovernanceChange to give the iterationObserver fail and finish methods, but I don't think they could do more than announce that the VaultManager has lost contact with the auction schedule.

@Chris-Hibbert
Copy link
Contributor

#9283 demonstrates that this is not an issue.

@dckc
Copy link
Member

dckc commented Apr 29, 2024

changing "completed" to "not planned"...

@dckc dckc reopened this Apr 29, 2024
@dckc dckc closed this as not planned Won't fix, can't repro, duplicate, stale Apr 29, 2024
@mergify mergify bot closed this as completed in #9283 May 6, 2024
mergify bot added a commit that referenced this issue May 6, 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!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working contract-upgrade Inter-protocol Overarching Inter Protocol Vaults VaultFactor (née Treasury)
Projects
None yet
4 participants