Excise reallocate()
#10871
Excise reallocate()
#10871
Conversation
Chris-Hibbert
added
Zoe
Deploying agoric-sdk with
|
| Latest commit: |
b6231b6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://40b94d30.agoric-sdk.pages.dev |
| Branch Preview URL: | https://6679-excisereallocate.agoric-sdk.pages.dev |
| // Removed. See #6679 | ||
| if (zcfBaggage.has('zcfSeatToStagedAllocations')) { | ||
| zcfBaggage.delete('zcfSeatToStagedAllocations'); | ||
| } |
There was a problem hiding this comment.
So all of the relevant state is in a side table? There's no exo state schema evolution monkey business?
That's awfully convenient.
There was a problem hiding this comment.
I think the deprecated form of reallocate() was the second design, and so ended up ini baggage rather than state.
There was a problem hiding this comment.
Since this is a regular map, should we assert that the map is empty before throwing away state?
And to ensure we don't run into this case, have we checked that all collections are empty on known chains?
There was a problem hiding this comment.
Since this is a regular map, should we assert that the map is empty before throwing away state?
The state is never valuable.
- it contains proposed allocations, so the contents are amounts and not assets
- Best practices is (and has been for a year) to use
atomicRearrangerather thanreallocate. If a contract we don't know about still has staged allocations, this update would make them unusable.
And to ensure we don't run into this case, have we checked that all collections are empty on known chains?
I've made sure that all contracts that we know about don't use reallocate. Contracts that don't use reallocate, can't make productive use of the collection.
There aren't any contracts that we don't know about running on durable chains. If third parties have code in draft that still follows the old approach, this will help them modernize before they get to a chain.
There was a problem hiding this comment.
Regarding upgrade considerations, removing a deprecated API is I think safe if we've checked that no currently deployed contract uses it.
Technically we should be able to remove a ZCF API like this even if a contract were using it since a null upgrade of a vat doesn't currently bump the zcf bundle, and picking up the change would be an explicit upgrade / restart of the contract.
| @@ -18,6 +18,8 @@ The main focus of most threats would be a breach of one of Zoe's core invariants | |||
|
|
|||
| ## Reallocation | |||
|
|
|||
| THIS SECTION IS OBSOLETE. We've converted all code to use attomicRearrange | |||
There was a problem hiding this comment.
| THIS SECTION IS OBSOLETE. We've converted all code to use attomicRearrange | |
| THIS SECTION IS OBSOLETE. We've converted all code to use atomicRearrange |
There was a problem hiding this comment.
I suppose we have coverage of basic upgrade functionality through some other test (preferably something lighter than an integration test)?
There was a problem hiding this comment.
No, we don't.
In order to test upgrading a contract from one version of ZCF to another, we'd need to be able to do what I call time-travel testing, or maintain two different versions of ZCF to install within a single test. We can do that in agoric-3-proposals to represent the change from a past version to the current version, or in a3p-integration to represent the change from the current-on-chain version to this new one. But otherwise, any unit test or bootstrap test would be testing a move the the current version to itself, which is no change at all.
The test it would be straightforward to add to a3p-integration would call E(zoeConfigFacet).updateZcfBundleId(newBundleId), early in the upgrade process, and then any vat that gets upgraded later will use the new ZCF. The tests in p::upgrade-19 exercise ZCF enough to show that the upgrade will be successful.
I'll add that to the tests, and also add the update to the staged changes in upgrade.go.
There was a problem hiding this comment.
I thought we had the ability to do a fake livelsots test of a single contract? I suppose any interaction with Zoe is a problem for such a test?
| // Removed. See #6679 | ||
| if (zcfBaggage.has('zcfSeatToStagedAllocations')) { | ||
| zcfBaggage.delete('zcfSeatToStagedAllocations'); | ||
| } |
There was a problem hiding this comment.
Since this is a regular map, should we assert that the map is empty before throwing away state?
And to ensure we don't run into this case, have we checked that all collections are empty on known chains?
closes: #6679
Description
Removes the last vestiges of
zcf.reallocate()and staged allocations.Security Considerations
The deprecated code turned out to be a bug farm. We've had no problems with the replacement.
Scaling Considerations
None.
Documentation Considerations
A separate PR (Docs:#1266) will remove the documentation. It has been marked deprecated for quite some time.
Testing Considerations
Removed tests. All the new code (using
atomicRearrange) has had tests all along.Upgrade Considerations
No known code uses the deprecated functionality. These changes are confined to ZCF. It won't matter when or if Zoe gets upgraded. The next time we bump the ZCF version, new and upgraded contracts will start using the new ZCF, but there's no hurry to make that happen.