10130 excise SharedStateRecord

[turadg]

closes: #10130

Description

Provide examples of how a contract can have a shared local account, without the race condition of creating it conditionally in a flow.

In the course of this I corrected the LocalOrchestrationAccount method signatures (particularly deposit not returning the Amount).

Security Considerations

none, just example code

Scaling Considerations

none, example code

Documentation Considerations

Examples are implicit documentation. Can be mined for docs site tutorials if the need arises.

Testing Considerations

The upgrade test for send-anywhere is disabled, but since this just makes a vow there shouldn't be any upgrade interactions.

Upgrade Considerations

The examples aren't to be deployed.

The remove of sharedStateRecord won't affect anything on chain. Same for changing the return of deposit.

[cloudflare-workers-and-pages]

Deploying agoric-sdk with    Cloudflare Pages

Latest commit:	31e82eb
Status:	🚫  Build failed.

View logs

[mhofman]

I am surprised to not see a test failure with this change.

The misbehavior of the guest will cause the guest invocation to become suspended until an upgrade to a new version which repairs the behavior. That said I'd expect the test to fail if the invocation doesn't complete as expected.

[mhofman]

There's a little more to excise in the endowments logic, but removing the helper is a good start to prevent benign usage.

[mhofman]

Yeah this is not supported. First, a function is not passable. And even if we had a Far function, no guest created capability (aka remotable or promise, not copy data) can currently be passed back. The main problem is that when you pass a guest created remotable, there is no guarantee the host will only use that capability before the flow invocation completes. Ideally we'd need a way for the author to make explicit that the capability is no longer expected to be used, aka a revocation mechanism, and it would be an error of the flow to complete without explicitly revoking these ephemeral remotables first. The problem with promises is somewhat similar: we could allow them but enforce that the promise is settled by the flow by the time the flow completes. Without that it would be useless to the host.

[mhofman]

Would something like provideAccountForChain(orch.getChain('agoric')) work?

If you want to define this logic collocated, I'm wondering if we'd be able to use the orchestrateAll trick (likely with some changes in endowments needed), aka await atomicProvider.provideAsync('localAccount', flows.makeLocalAccount).

This all seems very clunky though, and I'm not sure how to make this cleaner. Is this a generic pattern that we need to support, or something we could have a helper built into orch (which would raise the question of how the orch state is shared between multiple orchestrated flows)?

[mhofman]

There seem to be some kind of design tension here in the orch API. From what I understand:

• Some contracts need a "local account" that is shared between multiple invocations of orchestrated flows (possibly all of them?)
• Creating that singleton local account is dependent on network and itself needs to be resumable

If this is something the orch API cannot facilitate better, then an orchestrated provider pattern is the correct idea, but the layering right now is not quite right. There is however simply no way around the flow having to await the provided orch account.

[mhofman]

If I understand what's going on here, this likely does not work on restart, and is upgrade hazardous for other reasons.

Afaik, provideAsync lazily invokes the maker if the value is not found in the store (nor pending).

orchestrate is an exo definition that needs to be rewired during start for every subsequent incarnation.

Given that the maker given to provide here contains the orchestrate call, and that it only happens once, the orchestrated flow will never be rewired in future incarnations, preventing the vat from upgrading.

Furthermore, the await here is for a non immediate promise. It is either dependent on network or on other vats, and would prevent the start result promise from immediately settling during the first crank. The only reason it works right now is because the first incarnation of the contract has this logic, and there is a limitation in zcf which cannot enforce single crank first start of contract (because the first zcf start is actually done in a second crank). If this was added as new logic in a second incarnation of the contract, the upgrade to that incarnation would fail.

I believe the following would work:

Suggested change

	const sharedLocalAccount = await atomicProvider.provideAsync(
	'localAccount',
	key => vowTools.when(orchestrate(key, {}, sharedFlows.makeLocalAccount)()),
	);
	const { makeLocalAccount } = orchestrateAll(sharedFlows, {});
	const sharedLocalAccount = zone.makeOnce(
	'localAccount',
	key => makeLocalAccount(),
	);

Then of course you still need to await the sharedLocalAccount inside flow, but there is just no way around that.

I imagine a slightly more reusable approach would be:

  const orchAccountsProvider = zone.subZone('orchAccounts');
  const { makeOrchAccount } = orchestrateAll(sharedFlows, {});
  const provideOrchAccount = chain => orchAccountProvider.makeOnce(chain, makeOrchAccount);

Then provideOrchAccount can be given as an endowment. It's return value of course must still be awaited in flows.

[turadg]

Great review! 🙏

[mhofman]

LGTM, but I'd like @0xpatrickdev's feedback since I roughly suggested the current approach.

[mhofman]

Blah I need to get back to my type mapping work.

[0xpatrickdev]

I don't have many insights on the correctness of the approach wrt to upgradability, but can clearly see this avoids the potential race condition.

The ergonomics look good and make sense to me. I left two suggestions where I think we can provide more context to the reader.

[0xpatrickdev]

Is this comment necessary? Maybe you are envisioning some shared helpers that will rely on add'l endowments in the future?

A @file comment might be helpful here. Perhaps something to the effect of:

Shared Flows are common async flow functions that can be reused across contracts. They are bound to a particular contract's context via orchestrateAll. See ./send-anywhere.contract.js for example usage.

[0xpatrickdev]

Since this is an example contract, consider over communicating what's going on here:

Suggested change

	/** @type {any} */
	/**
	* Setup a shared local account for use in async-flow functions.
	* Typically, exo initState functions need to resolve synchronously, but
	* `makeOnce` allows us to provide a Promise. When using this inside a flow,
	* we must await it to ensure the account is available for use.
	* @type {unknown}
	*/

[0xpatrickdev]

This doesn't seem to be the correct place for this - we should only care about the return signature of deposit in local-orchestration-account.js. We currently use the returnVoidWatcher (handler) there, so I believe this commit can be removed

[turadg]

Thanks for the catch. I revised to fix the type annotation instead