-
Notifications
You must be signed in to change notification settings - Fork 208
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
chore: parameterize price feed replacement (check liquidation) #9827
Conversation
runs like this so far:
|
87bd81d
to
97fa744
Compare
Deploying agoric-sdk with Cloudflare Pages
|
e69fe85
to
659c016
Compare
18f788e
to
aafae91
Compare
@Chris-Hibbert help me understand why liquidation is not triggering? p.s. because the updated price is coming from a feed that vaults/auctions have yet to be introduced to. ✘ [fail]: 3. verify liquidation$ yarn test test/bootstrapTests/price-feed-replace.test.ts
...
✘ [fail]: 3. verify liquidation
─
3. verify liquidation
test/bootstrapTests/price-feed-replace.test.ts:125
124: // vaultFactory sent collateral for liquidation
125: t.like(readLatest(metricsPath), {
126: numActiveVaults: 0,
Difference (- actual, + expected):
{
liquidatingCollateral: {
- value: 0n,
+ value: 15000000n,
},
liquidatingDebt: {
- value: 0n,
+ value: 100500000n,
},
lockedQuote: null,
- numActiveVaults: 1,
+ numActiveVaults: 0,
- numLiquidatingVaults: 0,
+ numLiquidatingVaults: 1,
}
› Object.value [as like] (file:///home/connolly/projects/agoric-sdk/node_modules/@endo/ses-ava/src/ses-ava-test.js:164:35)
› <anonymous> (test/bootstrapTests/price-feed-replace.test.ts:125:5)
─
1 test failed
error Command failed with exit code 1. I rebased it on #9999. There's more to do, but I'd like to get this part working. |
076c604
to
f83e8c2
Compare
Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label. |
When a new price aggregator is launched, oracle operators need to accept new invitations.
0af884f
to
3d6c4e7
Compare
@Chris-Hibbert over to you as of 3d6c4e7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two questions.
I saw that you addressed the missing consume
power.
await priceFeedDrivers[collateralBrandKey].refreshInvitations(); | ||
}); | ||
|
||
test.serial('1. place bid', async t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@turadg has pointed out that test.serial()
doesn't mean "must run in this order". It apparently means "must run one at a time in some order".
Should we do something else for tests that must run in a particular order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when tests must run in a particular order, they're better thought of as one test. if you want more structure around the steps, a little helper can be made to log the progress.
oracleBrandProduce[name].reset(); | ||
oracleBrandProduce[name].resolve(brand); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it okay for this to overwrite existing brands? Do we expect to use that ability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not.
provideInertBrand(name)
gives the same result for the same name, so it shouldn't make any difference.
But the .reset()
is probably a distraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll drop the reset(). Thanks.
[deployPriceFeeds.name]: { | ||
consume: { | ||
agoricNamesAdmin: t, | ||
board: t, | ||
chainStorage: t, | ||
chainTimerService: t, | ||
econCharterKit: t, | ||
highPrioritySendersManager: t, | ||
namesByAddressAdmin: t, | ||
priceAuthority: t, | ||
priceAuthorityAdmin: t, | ||
startGovernedUpgradable: t, | ||
zoe: t, | ||
}, | ||
instance: { | ||
produce: t, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
installPriceAggregator needs
installation: {
produce: { priceAggregator },
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that a later commit handled this.
3d6c4e7
to
d0c83f8
Compare
d0c83f8
to
3d6c4e7
Compare
This has been incorporated into #10074. |
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
DRAFT until:
setup
,outcome
constantsstacked on
was:
Description
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations