Add a coreEval for vaults auctions and test in a3p #9911
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
de3b258
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://72d80c9a.agoric-sdk.pages.dev |
| Branch Preview URL: | https://9887-vaults-auctions-coreeva.agoric-sdk.pages.dev |
Chris-Hibbert
force-pushed
the
9887-vaults-auctions-coreEval
branch
from
e410284 to
7fe9b42
Compare
There was a problem hiding this comment.
I was surprised to see a:upgrade-next delayed, but it makes sense. Master needs to stage several CoreEvals that must be tested on top of u16, not the next upgrade.
So we don't have to move upgrade-next around each time we enter and leave such a situation, let's give it a durable id. e allows for four coreEvals before it, but we have a lot of letters in the alphabet. I suggest n:upgrade-next because it's half-way through the alphabet and n is a mnemonic for "next"
There was a problem hiding this comment.
i'm a little concerned at how much we're copying this around, but I think it can be solved better once it's in the agoric-3-proposals repo. That makes it easier to refactor between proposals and the @agoric/synthetic-chain lib, also in the repo
There was a problem hiding this comment.
Yeah, I'll migrate the stable parts to synthetic-chain.
There was a problem hiding this comment.
why is this waiting for the next upgrade instead of happening in a CoreEval?
There was a problem hiding this comment.
It should be part of whatever upgrade/CoreEval updates the priceFeeds. Should I create a dedicated directory for that?
Moving it to upgrade-next with the rest of what was there seemed like the default choice, saying the least about where it should actually end up. In order to move it to another directory, I'd feel obligated to fill that directory out more, which seems more unrelated to this PR's goals.
| @@ -29,132 +25,6 @@ func isFirstTimeUpgradeOfThisVersion(app *GaiaApp, ctx sdk.Context) bool { | |||
| return true | |||
| } | |||
|
|
|||
| // upgradePriceFeedCoreProposalSteps returns the core proposal steps for the | |||
There was a problem hiding this comment.
why are all these Golang changes necessary?
There was a problem hiding this comment.
I wouldn't say "necessary". How about "beneficial"?
They were in upgrade.go because we thought upgrading vaults/auctions/priceFeeds was a package deal. Now that we've split them apart, it's not clear when we'll get to priceFeeds, or whether it'll be a coreEval or SoftwareUpgrade.
I think removing this code makes it easier for whoever works on the next Upgrade to put together a clean set of changes.
I can move the cleanup to a separate PR if you would prefer that.
| @@ -143,6 +152,8 @@ export const addAuction = async ({ | |||
| ); | |||
| // don't overwrite auctioneerKit or auction instance yet. Wait until | |||
| // upgrade-vault.js | |||
|
|
|||
| auctionsDone.resolve(true); | |||
There was a problem hiding this comment.
is there no other way to indicate this than this boolean in promise space?
if it must be in this somewhat global space please give it a more specific name. Maybe by a general pattern we can use to know a value is one-off. E.g. didAuctionsOverUpgrade16
There was a problem hiding this comment.
A resolved value in the promise space seems like the ideal solution. I didn't look for other possibilities. What would you like to improve about it?
Would removing it once it has served its purpose resolve your concerns?
There was a problem hiding this comment.
Would removing it once it has served its purpose resolve your concerns?
Yes!
The new name is sufficient until we have another auctions upgrade
Chris-Hibbert
added
contract-upgrade
Vaults
Chris-Hibbert
force-pushed
the
9887-vaults-auctions-coreEval
branch
2 times, most recently
from
8f15cb0 to
028f710
Compare
Chris-Hibbert
force-pushed
the
9887-vaults-auctions-coreEval
branch
from
028f710 to
9ce0c22
Compare
closes: #9887
Description
Create a coreEval that replaces the Auctions contract, and upgrades VaultFactory.
Security Considerations
No security-relevant changes.
Scaling Considerations
Doesn't make things better or worse.
Documentation Considerations
N/A
Testing Considerations
Includes a test in a3p-integration that shows that there's a new auction, that vaultFactory upgrades, that new bids submitted go to the new auction, and that the upgraded vaults respond to price updates.
Upgrade Considerations
Upgrade is tested.