zoe adminFacet can terminate contract #10267
Conversation
Chris-Hibbert
force-pushed
the
9716-zoe-kill
branch
from
3c2c80c to
b377536
Compare
Deploying agoric-sdk with
|
| Latest commit: |
507c979
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4b226e84.agoric-sdk.pages.dev |
| Branch Preview URL: | https://9716-zoe-kill.agoric-sdk.pages.dev |
Chris-Hibbert
force-pushed
the
9716-zoe-kill
branch
from
8bd303e to
85cdd1f
Compare
There was a problem hiding this comment.
The plan to get this code to mainnet isn't completely explicit, but I think it's clear enough.
The upgrade requires only ... upgrading Zoe.
We have rough plans to upgrade all vats to match master in due course:
- build SDK releases from master #9252
- add code to upgrade all upgradable vats during agoric-upgrade-12 #8219
In particular, we have experience with replace-zoe.js in #8291
| const liquidityIssuer = await E(publicFacet).getLiquidityIssuer(); | ||
| const liquidityBrand = await E(liquidityIssuer).getBrand(); | ||
| const liquidity = value => AmountMath.make(liquidityBrand, value); | ||
|
|
||
| // Alice adds liquidity | ||
| // 10 moola = 5 simoleans at the time of the liquidity adding | ||
| // aka 2 moola = 1 simolean | ||
| const addLiquidityProposal = harden({ | ||
| give: { Central: moola(10n), Secondary: simoleans(5n) }, | ||
| want: { Liquidity: liquidity(10n) }, | ||
| }); | ||
| const paymentKeywordRecord = harden({ | ||
| Central: moolaPayment, | ||
| Secondary: simoleanPayment, | ||
| }); | ||
| const addLiquidityInvitation = E(publicFacet).makeAddLiquidityInvitation(); | ||
| const addLiqSeatP = await E(zoe).offer( | ||
| addLiquidityInvitation, | ||
| addLiquidityProposal, | ||
| paymentKeywordRecord, | ||
| ); | ||
|
|
||
| console.log(await E(addLiqSeatP).getOfferResult()); | ||
|
|
||
| const liquidityPayout = await E(addLiqSeatP).getPayout('Liquidity'); | ||
|
|
||
| const liquidityTokenPurseP = E(liquidityIssuer).makeEmptyPurse(); | ||
| await E(liquidityTokenPurseP).deposit(liquidityPayout); |
There was a problem hiding this comment.
What's the purpose of the stuff between starting the contract and shutting it down?
There was a problem hiding this comment.
copied from a previous test. Not adding anything here.
| [3, 7, 0], | ||
| ]; | ||
| const dump = await main(t, ['shutdownAutoswap', startingValues]); | ||
| t.deepEqual(dump.log, expectedShutdownAutoswapOkLog); |
There was a problem hiding this comment.
Have you considered using t.snapshot()?
| t.deepEqual(dump.log, expectedShutdownAutoswapOkLog); | |
| t.snapshot(dump.log); |
There was a problem hiding this comment.
Nope, hadn't thought of that.
Perhaps moderately simpler to maintain.
I was at first intending to not request review until this had been tested with those changes. Then I realized that this code only gets invoked by a coreEval, and it's benign until invoked. We can make this change now, and wait for kernel changes to try to run it. If you think we should wait for that testing before merging and installing on-chain, then please say so and revert your approval. Or just wait to approve the upcoming PR that will add this change to |
Chris-Hibbert
force-pushed
the
9716-zoe-kill
branch
from
1c578a7 to
672f9f5
Compare
I'm content with that plan. |
Chris-Hibbert
added
the
automerge:rebase
Chris-Hibbert
force-pushed
the
9716-zoe-kill
branch
from
672f9f5 to
81aaeac
Compare
Chris-Hibbert
removed
the
automerge:rebase
Chris-Hibbert
force-pushed
the
9716-zoe-kill
branch
from
81aaeac to
507c979
Compare
Chris-Hibbert
added
the
automerge:rebase
refs: #10267 ## Description Upgrade Zoe as part of the next software upgrade. ### Security Considerations The new Zoe will add the ability to terminate vats from bootstrap space. ### Scaling Considerations Terminating vats will allow us to address some space leakage issues. ### Documentation Considerations The new behavior is accessible on a facet that is only exposed to the bootstrap space. Doesn't need to impact developer docs. ### Testing Considerations Tested in a bootstrap test. ### Upgrade Considerations We expect to include this change with the next software upgrade, so we can start making use of the ability to remove unneeded vats.
refs: #10267 ## Description We'll be upgrading Zoe in the next SW upgrade. We need to reset the KREAd subscribers when we do that. ### Security Considerations No broader security implications ### Scaling Considerations Not a scaling concern ### Documentation Considerations I should have remembered this when adding the Zoe upgrade. ### Testing Considerations None ### Upgrade Considerations There's a constraint that we need to enforce when we upgrade Zoe. So far, we've been remembering it, but it would be better to automate.
closes: #9716
Description
Add the ability to terminate a vat if you have the adminFacet.
Security Considerations
Doesn't add authority, because anyone with the adminFacet could also upgrade the contract to something that calls
zcf.shutdown(). In practice, this will be controlled by governance or the bootstrap space.Scaling Considerations
The scaling issue is about cleaning up abandoned vats that are holding memory we'd like to release.
Documentation Considerations
We should (not urgently) update the zoe docs to mention this ability.
Testing Considerations
Adding a unit test wouldn't show anything, since it relies on the adminNode.
A SwingSet test shows that the vat is dead.
I will pass this to @warner as a draft, so he can do extensive testing of the cleanup process.
Upgrade Considerations
The upgrade requires only merging the code and upgrading Zoe. This will redefine all the existing
adminFacets. The holders of those facets (in the bootstrap space) will then be able to invoke the method and observe the results.