9584 upgrade price feeds #10074
9584 upgrade price feeds #10074
Conversation
Chris-Hibbert
added
contract-upgrade
oracle
Deploying agoric-sdk with
|
| Latest commit: |
eb941d4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://20487841.agoric-sdk.pages.dev |
| Branch Preview URL: | https://9584-upgradepricefeeds.agoric-sdk.pages.dev |
Chris-Hibbert
force-pushed
the
9584-upgradePriceFeeds
branch
4 times, most recently
from
3b7ea4a
to
60f025d
Compare
| # CoreEvalProposal to replace existing price_feed and scaledPriceAuthority vats | ||
| # with new contracts. Auctions will need to be replaced, and Vaults will need to | ||
| # get at least a null upgrade in order to make use of the new prices. Oracle | ||
| # operators will need to accept new invitations, and sync to the roundId (0) of | ||
| # the new contracts in order to feed the new pipelines. |
There was a problem hiding this comment.
not sure you meant this all to be a heading
| export const queryVstorage = path => | ||
| agd.query('vstorage', 'data', '--output', 'json', path); | ||
|
|
||
| export const getOracleInstance = async price => { |
There was a problem hiding this comment.
can you add a comment along the lines of
// XXX kludge of some of marshal. see https://github.com/Agoric/agoric-3-proposals/issues/176
| const deadline = toSec(Date.now()) + voteDurSec; | ||
|
|
||
| const zip = (xs, ys) => xs.map((x, i) => [x, ys[i]]); | ||
| const fromSmallCapsEntries = txt => { |
There was a problem hiding this comment.
please add a "kludged part of marshal" note on this one too.
| t.truthy(instance); | ||
| }; | ||
|
|
||
| const checkPriceFeedVatsUpdated = async t => { |
There was a problem hiding this comment.
Should this check that cardinality is 2? we talked about that.
There was a problem hiding this comment.
Turns out that is happening in countPriceFeedVats()
| const details = await getDetailsMatchingVats('auctioneer'); | ||
| // This query matches both the auction and its governor, so double the count | ||
| t.true(Object.keys(details).length > 2); | ||
| }; |
There was a problem hiding this comment.
consider using t.like to check details
There was a problem hiding this comment.
details only has vatID, incarnation, vatName, and bundleID, so there's not much value. counting instances is all I can do.
But the number does need to be updated, This is the third instance.
| @@ -1,8 +1,32 @@ | |||
| /* global process */ | |||
|
|
|||
| import { makeHelpers } from '@agoric/deploy-script-support'; | |||
There was a problem hiding this comment.
is this whole file subsumed by updatePriceFeeds.js now?
There was a problem hiding this comment.
It's still referenced by updateAtomPriceFeed, and four others, but nothing in this PR depends on it. I'll drop these changes.
|
|
||
| /** @import {EconomyBootstrapPowers} from './econ-behaviors.js'; */ | ||
| /** @import {ChainlinkConfig} from '@agoric/inter-protocol/src/price/fluxAggregatorKit.js'; */ | ||
| /** @typedef {typeof import('@agoric/inter-protocol/src/price/fluxAggregatorContract.js').start} FluxStartFn */ |
There was a problem hiding this comment.
move this typedef to fluxAggregatorContract.js?
| }; | ||
|
|
||
| /** | ||
| * Create inert brands (no mint or issuer) referred to by price oracles. |
There was a problem hiding this comment.
| * Create inert brands (no mint or issuer) referred to by price oracles. | |
| * Provide (find or create) inert brands (no mint or issuer) referred to by price oracles. |
| description: AGORIC_INSTANCE_NAME, | ||
| brandIn, | ||
| brandOut, | ||
| timer: await chainTimerService, |
There was a problem hiding this comment.
pls add
// XXX powerful timer service in terms. see #NNN
|
|
||
| trace('distributing invitations', oracleAddresses); | ||
| // This doesn't resolve until oracle operators create their smart wallets. | ||
| // Don't block bootstrap on it. |
There was a problem hiding this comment.
| // Don't block bootstrap on it. | |
| // Don't block completion on it. |
| // publish into agoricNames so that others can await its presence. | ||
| // This must stay after registerPriceAuthority above so it's evidence of registration. |
There was a problem hiding this comment.
I don't see a "registerPriceAuthority above". I think this is copied from elsewhere. Does it still apply somehow?
There was a problem hiding this comment.
Maybe the registerPriceAuthority is inside startScaledPriceAuthority.
There was a problem hiding this comment.
I simplified the comment. The original form applies when first creating a priceFeed, but on upgrade, those waiting would get the previous version if they don't have an interlock like our priceAuthority8400.
| async function selectKitsWithInstallation(kits) { | ||
| /** @type {StartedInstanceKit<any>[]} */ | ||
| const scaledPAKitMapP = Array.from(kits.values()).map(kit => [ | ||
| kit, | ||
| E(zoe).getInstallationForInstance(kit.instance), | ||
| ]); | ||
| const scaledPAKitMap = await deeplyFulfilled(harden(scaledPAKitMapP)); | ||
| const scaledPAKitEntries = []; | ||
| for (const [instance, installation] of scaledPAKitMap) { | ||
| if (spaInstallation === installation) { | ||
| scaledPAKitEntries.push(instance); | ||
| } | ||
| } | ||
| return scaledPAKitEntries; | ||
| } | ||
| const scaledPAKitEntries = await selectKitsWithInstallation(contractKits); |
There was a problem hiding this comment.
"entries" strongly suggests an array of [X, Y] tuples. scaledPAKitEntries seems to be an array of Xs
Why all the maps and such? Isn't this code just filtering the kits on installation?
An async filter adds a little bit of a wrinkle, but still...
| async function selectKitsWithInstallation(kits) { | |
| /** @type {StartedInstanceKit<any>[]} */ | |
| const scaledPAKitMapP = Array.from(kits.values()).map(kit => [ | |
| kit, | |
| E(zoe).getInstallationForInstance(kit.instance), | |
| ]); | |
| const scaledPAKitMap = await deeplyFulfilled(harden(scaledPAKitMapP)); | |
| const scaledPAKitEntries = []; | |
| for (const [instance, installation] of scaledPAKitMap) { | |
| if (spaInstallation === installation) { | |
| scaledPAKitEntries.push(instance); | |
| } | |
| } | |
| return scaledPAKitEntries; | |
| } | |
| const scaledPAKitEntries = await selectKitsWithInstallation(contractKits); | |
| async function maybeSPAKit(kit) { | |
| const installation = await E(zoe).getInstallationForInstance(kit.instance); | |
| return spaInstallation === installation ? [kit] : []; | |
| } | |
| const scaledPAKits = (await Promise.all([...contractKits.values()].map(maybeSPAKit))).flat(); |
There was a problem hiding this comment.
Thanks. That's simpler.
| for (const kitEntry of scaledPAKitEntries) { | ||
| trace({ kitEntry }); | ||
|
|
||
| const keyword = kitEntry.label.match(/scaledPriceAuthority-(.*)/)[1]; |
There was a problem hiding this comment.
matching by label seems fragile. the relevant brand is in the terms of the contract. Not the standard terms, but inside ratios:
agoric-sdk/packages/zoe/src/contracts/scaledPriceAuthority.js
Lines 41 to 51 in 18a1b58
Then we can get a string name for the brand by reverse lookup in agoricNames.
There was a problem hiding this comment.
keyword is a misnomer that seemed harmless until #8238 . AFAICT, replaceScaledPriceAuthority uses it only as a compatibility default for issuerName.
Please use issuerName.
| [replaceScaledPriceAuthorities.name]: { | ||
| consume: { | ||
| agoricNamesAdmin: t, | ||
| contractKits: t, |
There was a problem hiding this comment.
contractKits has lots of power - admin and creator facets for lots of contracts.
We only want the kits with a certain installation here.
That would be an interesting pattern to support explicitly.
See also:
| ) => ({ | ||
| manifest: { | ||
| [replaceScaledPriceAuthorities.name]: { | ||
| consume: { |
There was a problem hiding this comment.
In these enumerations of powers, what do you think about a comment separating the widely shared ones like agoricNames with the closely held ones like contractKits?
There was a problem hiding this comment.
Not sure how much it helps at this point, but doesn't hurt anything.
| startUpgradable: t, | ||
| }, | ||
| instance: { | ||
| produce: t, |
There was a problem hiding this comment.
| produce: t, | |
| // This is a right to add/replace any instance. That we update only the relevant ones must be verified by inspection. | |
| produce: t, |
The primary goal is to stop using the original priceFeeds and scaledPriceAuthorities that are retaining old quotePayments. (see In order to do so, we also replace the auctioneer, and upgrade the vaultManager, since they have no mechanism for dynamically finding out about new priceAuthorities. There are bootstrap tests and a3p-integration/ tests here to demonstrate that after the upgrades and replacements, all these well-known vats are in touch with one another and liquidation still works.
Chris-Hibbert
force-pushed
the
9584-upgradePriceFeeds
branch
from
bf1bd0c
to
eb941d4
Compare
…10163) closes: #9982 closes: #10172 ## Description When a governed contract is upgraded, its paramManager (which was ephemeral) is replaced, and the contractGovernor needs to get a fresh copy. ### Security Considerations This bug caused us to need to cut RC2 when preparing the vaultFactory release. That time, the fix was to upgrade the contractGovernor when upgrading a governed contract. ### Scaling Considerations No scaling impacts. ### Testing Considerations added a new test, which fails without the fix. ### Upgrade Considerations This is staged on top of #10074, and the goal is to include it with the priceFeed coreEval. Recent commits have updated the coreEval to include upgrading and installing the contractGovernor code.
closes: #10076 refs: #10074 ## Description Build platform-dependent proposals for the PriceFeed coreEval. For each platform (not counting A3P) defined in `updatePriceFeeds.js`, a command is added, which invokes `scripts/build-submission.sh` three times to build each of the required proposals (updatePriceFeeds.js, add-auction.js, and upgradeVaults.js). The invocation for updatePriceFeeds takes a parameter specifying the platform. ### Security Considerations N/A ### Scaling Considerations N/A ### Documentation Considerations Not user-visible ### Testing Considerations test on DevNet and MainFork ### Upgrade Considerations It's all about upgrade. This coreEval has code that varies depending on the collaterals and oracles present, so we have to build separate proposals for each platform.
closes: #9584
closes: #9928
refs: #9827
refs: #9748
refs: #9382
closes: #10031
Description
We added upgrading the scaledPriceAuthority to the steps in upgrading vaults, auctions, and priceFeeds, and didn't notice that it broke things. The problem turned out to be that the "priceAuthority" object registered with the priceFeedRegistry was an ephemeral object that was not upgraded. This fixes that by re-registering the new priceAuthority.
Then, to simplify the process of cleaning up the uncollected cycles reported in #9483, we switched to replacing the scaledPriceAuthorities rather than upgrading them.
We also realized that we would need different coreEvals in different environments, since the Oracle addresses and particular assets vary for each (test and mainNet) chain environment.
#9748 addressed some of the issues in the original coreEval. #9999 showed what was needed for upgrading priceFeeds, which was completed by #9827. #10021 added some details on replacing scaledPriceAuthorities.
Security Considerations
N/A
Scaling Considerations
Addresses one of our biggest scaling issues.
Documentation Considerations
N/A
Testing Considerations
Thorough testing in a3p, and our testnets. #9886 discusses some testing to ensure Oracles will work with the upgrade.
Upgrade Considerations
See above