Excise `reallocate()` #10871

Chris-Hibbert · 2025-01-22T01:01:25Z

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.

cloudflare-workers-and-pages · 2025-01-22T18:38:48Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`c522d55`
Status:	✅ Deploy successful!
Preview URL:	https://19e59eaf.agoric-sdk.pages.dev
Branch Preview URL:	https://6679-excisereallocate.agoric-sdk.pages.dev

View logs

dckc

looks ok from a quick skim.

I should think it thru some more before approving

dckc · 2025-01-23T00:00:53Z

packages/zoe/src/contractFacet/zcfSeat.js

+  // Removed.  See #6679
+  if (zcfBaggage.has('zcfSeatToStagedAllocations')) {
+    zcfBaggage.delete('zcfSeatToStagedAllocations');
+  }


So all of the relevant state is in a side table? There's no exo state schema evolution monkey business?

That's awfully convenient.

I think the deprecated form of reallocate() was the second design, and so ended up ini baggage rather than state.

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?

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 atomicRearrange rather than reallocate. 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.

mhofman

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.

mhofman · 2025-01-23T18:04:12Z

packages/zoe/docs/AttackGuide.md

@@ -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


Suggested change

THIS SECTION IS OBSOLETE. We've converted all code to use attomicRearrange

THIS SECTION IS OBSOLETE. We've converted all code to use atomicRearrange

Still applicable.

mhofman · 2025-01-23T18:05:09Z

packages/boot/test/bootstrapTests/zcf-upgrade.test.ts

I suppose we have coverage of basic upgrade functionality through some other test (preferably something lighter than an integration test)?

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 from the current version to itself, which is no change at all.

The test would be straightforward to add to a3p-integration. It 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.

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?

Bootstrap tests (in packages/boot/test/bootstrapTests) have the ability to test a contract running in a vat, and interacting with the standard set of vats started up using the whole bootstrap mechanism. It's hard to use these tests to do upgrade tests from one version of code to another because they get their code from agoric-sdk directly. So using this to compare what happens when upgrading from one version of ZCF to another would also be problematic.

This PR has now been rebased on top of the repaired Upgrade 19 (#10947). On top of that, it now installs a new version of ZCF early in the cycle. From the logs, I can see that the contracts that get installed or upgraded after that get the new version of ZCF.

Since the U19 upgrade repairs also ensured that the acceptance tests are now running on the upgraded versions of the contracts, we can also be sure that everything that is tested in the acceptance tests is running against upgraded contracts.

mhofman · 2025-01-23T18:12:27Z

packages/zoe/src/contractFacet/zcfSeat.js

+  // Removed.  See #6679
+  if (zcfBaggage.has('zcfSeatToStagedAllocations')) {
+    zcfBaggage.delete('zcfSeatToStagedAllocations');
+  }


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?

mhofman

LGTM but I'll defer approval to someone closer to this part of the system

mhofman · 2025-02-14T04:18:41Z

golang/cosmos/app/upgrade.go

+				// Register a new ZCF to be used for all future contract instances and upgrades
+				vm.CoreProposalStepForModules(
+					"@agoric/builders/scripts/vats/upgrade-zcf.js",
+				),


Why not apply this before the wallet factory upgrade so that vat can pick up the new zcf too?

mhofman · 2025-02-14T04:19:40Z

packages/zoe/docs/AttackGuide.md

@@ -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


Still applicable.

dckc

After some discussion of Upgrade Considerations with @turadg and @mhofman , it seems that this is ready to go.

In particular, upgrading zcf is sufficiently testedc.

mergify · 2025-02-16T18:52:57Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ integration-test-result
☑️ linear-history

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Chris-Hibbert added Zoe package: Zoe code-style defensive correctness patterns; readability thru consistency contract-upgrade labels Jan 22, 2025

Chris-Hibbert self-assigned this Jan 22, 2025

Chris-Hibbert requested a review from a team as a code owner January 22, 2025 01:01

Chris-Hibbert requested a review from AgoricTriage January 22, 2025 01:01

Chris-Hibbert mentioned this pull request Jan 22, 2025

docs: drop all references to reallocate() and its helpers Agoric/documentation#1266

Merged

Chris-Hibbert requested a review from dckc January 22, 2025 22:38

dckc reviewed Jan 23, 2025

View reviewed changes

mhofman reviewed Jan 23, 2025

View reviewed changes

Chris-Hibbert added the force:integration Force integration tests to run on PR label Jan 25, 2025

Chris-Hibbert force-pushed the 6679-exciseReallocate branch from b6231b6 to f36e8e7 Compare February 12, 2025 00:55

Chris-Hibbert requested review from dckc and mhofman February 12, 2025 17:06

mhofman reviewed Feb 14, 2025

View reviewed changes

dckc approved these changes Feb 14, 2025

View reviewed changes

Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Feb 16, 2025

Chris-Hibbert added 9 commits February 16, 2025 18:08

refactor!: remove reallocate and support for staged allocations

252e15b

refactor: satisfies() no longer needs zcf param

07c98cd

test: drop zcfProbe test

65a0794

docs: update references to zcf.reallocate() in docs

7c4cc53

docs: drop obsolete comment references to incrementBy, decrementBy

eba5c2a

test: update obsolete reallocate() call in test

1b18a97

chore: update ZCF in upgrade.go

b1785b0

refactor: update ZCF before upgrading walletFactory

533b61e

chore: typo in attacker's guide updatee

c522d55

Chris-Hibbert force-pushed the 6679-exciseReallocate branch from 10f3624 to c522d55 Compare February 16, 2025 18:08

mergify bot merged commit c9e286a into master Feb 16, 2025
83 checks passed

mergify bot deleted the 6679-exciseReallocate branch February 16, 2025 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Excise `reallocate()` #10871

Excise `reallocate()` #10871

Chris-Hibbert commented Jan 22, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 22, 2025 •

edited

Loading

dckc left a comment

dckc Jan 23, 2025

Chris-Hibbert Jan 23, 2025

mhofman Jan 23, 2025

Chris-Hibbert Jan 25, 2025

mhofman left a comment

mhofman Jan 23, 2025

mhofman Feb 14, 2025

mhofman Jan 23, 2025

Chris-Hibbert Jan 24, 2025 •

edited

Loading

mhofman Jan 25, 2025 •

edited

Loading

Chris-Hibbert Feb 12, 2025

mhofman Jan 23, 2025

mhofman left a comment

mhofman Feb 14, 2025

mhofman Feb 14, 2025

dckc left a comment

mergify bot commented Feb 16, 2025

		@@ -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

Excise reallocate() #10871

Excise reallocate() #10871

Conversation

Chris-Hibbert commented Jan 22, 2025 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Jan 22, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

mhofman Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 16, 2025

Excise `reallocate()` #10871

Excise `reallocate()` #10871

Chris-Hibbert commented Jan 22, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 22, 2025 •

edited

Loading

Chris-Hibbert Jan 24, 2025 •

edited

Loading

mhofman Jan 25, 2025 •

edited

Loading