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

Turn priceFeed proposal into a SoftwareUpgrade #10317

Merged
merged 11 commits into from
Oct 25, 2024
Merged

Conversation

Chris-Hibbert
Copy link
Contributor

closes: #9370

Description

The priceFeed proposal has been tested as an independent coreEval, but the current plan calls for it to be included in Upgrade 18. This adds it to upgrade.go and moves the tests to n:upgrade-next.

Security Considerations

No additional security implications.

Scaling Considerations

Addresses pre-existing scaling concerns (see #8400), and adds no new scaling issues.

Documentation Considerations

None

Testing Considerations

An existing test is moved to a new location.

The current version seems to tickle #10292, so tests may not pass at this point.

Upgrade Considerations

Targeted for upgrade 18.

@Chris-Hibbert Chris-Hibbert added contract-upgrade oracle Related to on-chain oracles. Vaults VaultFactor (née Treasury) auction next-release about next agoric-sdk or endo release labels Oct 22, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Oct 22, 2024
@Chris-Hibbert Chris-Hibbert requested a review from a team as a code owner October 22, 2024 23:41
@Chris-Hibbert Chris-Hibbert changed the base branch from master to fraz/replace-committee-a3p October 22, 2024 23:41
@Chris-Hibbert Chris-Hibbert added the force:integration Force integration tests to run on PR label Oct 22, 2024
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

priceFeedUpdate.test.js etc. seems like adequate test coverage. I did manage to summon attention to study the code in as much detail as I'd like, but I recognize a lot of the patterns, and if the tests pass, I'm reasonably confident that it's good.

I much prefer that #10317 is merged first. I expect @frazarshad can get that done in the next day.

Merging this into fraz/replace-committee-a3p will complicate review of #10317; I'd rather we don't do that, but I find it acceptable.

const config = configurations[opts.variant];
if (!config) {
console.error(Usage);
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

suggestion:
having process.exit() someplace other than the main function makes me uncomfortable. I'd rather see throw here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Copy link

cloudflare-workers-and-pages bot commented Oct 23, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: a109395
Status: ✅  Deploy successful!
Preview URL: https://1987fbab.agoric-sdk.pages.dev
Branch Preview URL: https://9370-pricefeed-swupgrade.agoric-sdk.pages.dev

View logs

@Chris-Hibbert
Copy link
Contributor Author

cc: @frazarshad
Since the auctioneer (instance and governor) are now being updated in the same software update, the EC update has to make use of those new values. Please take a look at how I connected them.

@frazarshad
Copy link
Contributor

Since the auctioneer (instance and governor) are now being updated in the same software update, the EC update has to make use of those new values. Please take a look at how I connected them.

LGTM 👍

@Chris-Hibbert Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Oct 25, 2024
extracted getVariantFromUpgradeName
The vaults upgrade consumed instance.auctioneer and compared it to the
new instance, passed from the new auctioneer proposal in a single-use
promise, and failed if they weren't different. In this software
upgrade, the instance.auctioneer value might be the new value, so this
test can fail. Since the single-use promise comes directly from the
new auction proposal, if it has a value, it's the new one.
Why didn't it complain earlier?
I added replaceElectorate.js, so it wouldn't be run a second time by
  yarn ava ./*.test.js
but that caused it to not in CI be found for
  yarn ava ./replaceElectorate.test.js
which seems bizarre to me
@Chris-Hibbert Chris-Hibbert force-pushed the 9370-priceFeed-swUpgrade branch from 33c5460 to a109395 Compare October 25, 2024 21:55
@mergify mergify bot merged commit 5c932ce into master Oct 25, 2024
80 checks passed
@mergify mergify bot deleted the 9370-priceFeed-swUpgrade branch October 25, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auction automerge:rebase Automatically rebase updates, then merge contract-upgrade force:integration Force integration tests to run on PR next-release about next agoric-sdk or endo release oracle Related to on-chain oracles. Vaults VaultFactor (née Treasury)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create proposal for upgrade16 to replace all priceFeed vats
3 participants