Save the contractKit for the auctioneer before overwriting it #10694
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Chris-Hibbert
added
contract-upgrade
force:integration
warner
approved these changes
Dec 14, 2024
| @@ -206,6 +206,9 @@ export const addAuction = async ( | |||
| ); | |||
|
|
|||
| const governedContractKits = await governedContractKitsP; | |||
| // @ts-expect-error The original auctioneerKit had everything it needs | |||
| await governedContractKits.init(legacyKit.instance, legacyKit); | |||
There was a problem hiding this comment.
I don't think this should have an await.. init() is sync.
I imagine this clause could move earlier, so we're stashing the old value before we reset/resolve the new value. That would make more temporal sense, but 1: since our promise-space fulfills legacyKit to the old value even though/if we've reset/resolved the matching produceAuctioneerKit, it doesn't matter which order we do it in, and 2: doing it "backwards" makes it more obvious that we know the promise-space works that way; it doesn't look like we're accidentally relying on it not giving us back the new value. maybe.
| @@ -26,8 +26,7 @@ export const testRecordedRetiredInstances = async ({ | |||
| assert(auctionIDs.length === 1); | |||
| const auctionInstance = retiredContractInstances.get(auctionIDs[0]); | |||
| trace({ auctionInstance }); | |||
| // I don't know why it's neither in governedContractKits nor contractKits | |||
| // assert(await E(governedContractKits).get(auctionInstance)); | |||
| assert(await E(governedContractKits).get(auctionInstance)); | |||
There was a problem hiding this comment.
glad we had this test ready to go, let's figure out later why it got commented out
There was a problem hiding this comment.
I commented it out, because I thought the important thing was saving the instance.
turadg
approved these changes
Dec 14, 2024
Deploying agoric-sdk with
|
| Latest commit: |
185e078
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1c6020bf.agoric-sdk.pages.dev |
| Branch Preview URL: | https://cth-saveauctioneerkit.agoric-sdk.pages.dev |
mujahidkay
pushed a commit
that referenced
this pull request
Dec 17, 2024
refs: #10680 ## Description We misplaced the adminFacets that would allow us to manage Auctions after they've been replaced and were about to do it again. The auctions were [last upgraded](https://github.com/Agoric/agoric-sdk/blob/43345a561fbdf7621c369abb15e6839f7c696565/packages/inter-protocol/src/proposals/add-auction.js#L157) in `agoric-upgrade-16av`. That code fails to save the instance's adminFacet, and only stores the contractKit in bootstrap promise space under the name `auctioneerKit`, where it will be overwritten on upgrade. Our other contracts now save a copy of the `contractKit` in either `contractKits` or `governedContractKits`, indexed by the instance, so the facets will hang around. This saves the old auctioneer during upgrade so we can manage it later (upgrade, terminate, change parameters). ### Security Considerations Losing our last handle for vats is a problem. ### Scaling Considerations We're upgrading vats to deal with scaling. ### Documentation Considerations None. ### Testing Considerations there was a test in #10680 which looked for this kit in `governedContractKits`, but I commented it out when it didn't succeed. It succeeds now. ### Upgrade Considerations Yes.
mujahidkay
added a commit
that referenced
this pull request
Dec 17, 2024
### Description Cherry-picks the following commits from master: - #10696 (3e27c74) - #10694 (140c1be) - #10704 (ab3fcb5, c025cb7) No new upgrade name has been added. Using following rebase todo: ``` # PR #10704 Branch build-deps-use-backport-of-cosmos-sdk-v0-46-15-10704- label onto pick 140c1be Save the contractKit for the auctioneer before overwriting it (#10694) pick 3e27c74 test: add proposal w/300 E(chainTimerService).getTimerBrand() calls (#10696) label base-build-deps-use-backport-of-cosmos-sdk-v0-46-15-10704- pick ab3fcb5 build(deps): use backport of cosmos-sdk v0.46.15 pick c025cb7 fix(cosmic-swingset): expect chain --halt-height exit status > 1 label pr-10704--build-deps-use-backport-of-cosmos-sdk-v0-46-15-10704- reset base-build-deps-use-backport-of-cosmos-sdk-v0-46-15-10704- # test: add proposal w/300 E(chainTimerService).getTimerBrand() calls (#10696) merge -C 5939b2a pr-10704--build-deps-use-backport-of-cosmos-sdk-v0-46-15-10704- # build(deps): use backport of cosmos-sdk v0.46.15 (#10704) ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs: #10680
Description
We misplaced the adminFacets that would allow us to manage Auctions after they've been replaced and were about to do it again. The auctions were last upgraded in
agoric-upgrade-16av. That code fails to save the instance's adminFacet, and only stores the contractKit in bootstrap promise space under the nameauctioneerKit, where it will be overwritten on upgrade. Our other contracts now save a copy of thecontractKitin eithercontractKitsorgovernedContractKits, indexed by the instance, so the facets will hang around. This saves the old auctioneer during upgrade so we can manage it later (upgrade, terminate, change parameters).Security Considerations
Losing our last handle for vats is a problem.
Scaling Considerations
We're upgrading vats to deal with scaling.
Documentation Considerations
None.
Testing Considerations
there was a test in #10680 which looked for this kit in
governedContractKits, but I commented it out when it didn't succeed. It succeeds now.Upgrade Considerations
Yes.