feat(internal): Add StorageNode delete method

[gibson042]

Fixes #7405

Description

Adds a StorageNode delete method and the backing implementation in vstorage.

Security Considerations

This extends the power of holding an append-style StorageNode to include clearing out yet-to-be-published chain storage data (i.e., from a setValue(…) earlier in construction of the block). But since StorageNodes are not widely shared and the same situation is already possible if a non-append StorageNode references the same path, this is considered acceptable.

Scaling Considerations

This represents a slight reduction in message size vs. using "set" to accomplish the same effect, because the new vstorage "delete" message expects an array of paths rather than an array of [path, value?] entries.

Documentation Considerations

No documentation is needed beyond the TypeScript information in JSDoc comments.

Testing Considerations

Extension of existing unit tests should suffice.

Upgrade Considerations

Vats (and more specifically, the bridge vat that holds all StorageNodes) must not be allowed to pull in the updated lib-chainStorage before all nodes have pulled in the vstorage.go changes—although it would be acceptable for both to coincide in a single release, and in practice vat changes will require an explicit upgrade that is managed separately from updating Go code.

[dckc]

this seems to verify that a message crosses a bridge.

How about a a test where we see something like 3 child nodes a, b, and c; then we delete b and we see only a and c?

[gibson042]

This runs against a fake backend, so such a test wouldn't be indicative of much (and also, the StorageNode interface does not provide a means of enumerating children). But verification of effects upon chain storage data when messages such as this are received is included in the changes to golang/cosmos/x/vstorage/vstorage_test.go.

[dckc]

This is clearly progress on #7405, but I'm not sure it fixes that issue until we have a tested path to upgrade vat-bridge - or a plan for one in another issue. For example, removing exited auction bids (#7618, #7906) is a recent motivation for this feature, but if we land this PR on master and use this feature in the auctioneer contract, all our tests will pass, but when we deploy the new auctioneer contract (#8049) without deploying a vat-bridge upgrade, any .delete() calls from the auctioneer contract to vat-bridge will fail.

I suppose that likewise upgrading the bridge vat without upgrading the golang side won't work, so the upgrade has to be a chain software upgrade. I see #7977, but it doesn't mention this feature explicitly. (It doesn't mention any features explicitly, though.)

[turadg]

Documenting @gibson042 's work-around until this available in production.

with sequence: false. The path is

1. setValue('') on such a node sends { method: 'set', args: [ [path] ] }:

   agoric-sdk/packages/internal/src/lib-chainStorage.js

   Lines 185 to 199 in 3f47cf0

   	async setValue(value) {
   	const { sequence, path, messenger } = this.state;
   	assert.typeof(value, 'string');
   	/** @type {StorageEntry} */
   	let entry;
   	if (!sequence && !value) {
   	entry = [path];
   	} else {
   	entry = [path, value];
   	}
   	await cb.callE(messenger, {
   	method: sequence ? 'append' : 'set',
   	args: [entry],
   	});
   	},

2. that message is handled in the vstorage module by calling keeper.SetStorageAndNotify(ctx, { key: path, value: nil }):

   agoric-sdk/golang/cosmos/x/vstorage/vstorage.go

   Lines 71 to 80 in 3f47cf0

   	case "set":
   	for _, arg := range msg.Args {
   	var entry agoric.KVEntry
   	err = json.Unmarshal(arg, &entry)
   	if err != nil {
   	return
   	}
   	keeper.SetStorageAndNotify(ctx, entry)
   	}
   	return "true", nil

3. ...which delegates to keeper.SetStorage(…):

   agoric-sdk/golang/cosmos/x/vstorage/keeper/keeper.go

   Lines 329 to 332 in 3f47cf0

   	func (k Keeper) SetStorageAndNotify(ctx sdk.Context, entry agoric.KVEntry) {
   	k.changeManager.Track(ctx, k, entry, false)
   	k.SetStorage(ctx, entry)
   	}

4. ...which interprets an entry argument for which HasValue() is false as a deletion: https://github.com/Agoric/agoric-sdk/blob/master/golang/cosmos/x/vstorage/keeper/keeper.go#L371-L383

[turadg]

I think @mhofman has some design feedback. I'm reviewing as Comment to get it off my review queue until we hear that.

[mhofman]

My thoughts were captured in #7405 (comment), and I haven't had time to get back to this since. We should probably have a design discussion on this topic.