fix(orchestration): subscribeToTransfers() atomically

[dckc]

refs: #10391

Description

There was a race in subscribeToTransfers. Use a Vow to avoid the race.

Security / Scaling / Upgrade / Documentation Considerations

can't think of any

Testing Considerations

I'm not sure how to make a test for this. The purpose of this fix is mostly to stop other tests from failing.

[0xpatrickdev]

These are two tests we have that cover this path (via auto-stake-it.contract.js / auto-stake-it.flows.js) :

• orchestration/test/examples/auto-stake-it.contract.test.ts
• multichain-testing/test/auto-stake-it.test.ts

I assume they don't have the same symptoms we are currently facing due to these async calls in between .makeAccount() and .monitorTransfer():

agoric-sdk/packages/orchestration/src/examples/auto-stake-it.flows.js

Lines 45 to 78 in 0105e1a

	const [localAccount, stakingAccount] = await Promise.all([
	agoric.makeAccount(),
	/** @type {Promise<OrchestrationAccount<any> & StakingAccountActions>} */ (
	remoteChain.makeAccount()
	),
	]);
	const [localChainAddress, remoteChainAddress] = await Promise.all([
	localAccount.getAddress(),
	stakingAccount.getAddress(),
	]);
	const agoricChainId = (await agoric.getChainInfo()).chainId;
	const { transferChannel } = await chainHub.getConnectionInfo(
	agoricChainId,
	chainId,
	);
	assert(transferChannel.counterPartyChannelId, 'unable to find sourceChannel');
	const localDenom = `ibc/${denomHash({ denom: remoteDenom, channelId: transferChannel.channelId })}`;
	// Every time the `localAccount` receives `remoteDenom` over IBC, delegate it.
	const tap = makeStakingTap({
	localAccount,
	stakingAccount,
	validator,
	localChainAddress,
	remoteChainAddress,
	sourceChannel: transferChannel.counterPartyChannelId,
	remoteDenom,
	localDenom,
	});
	// XXX consider storing appRegistration, so we can .revoke() or .updateTargetApp()
	// @ts-expect-error tap.receiveUpcall: 'Vow<void> | undefined' not assignable to 'Promise<any>'
	await localAccount.monitorTransfers(tap);

But their passing should help towards giving us confidence this change does not introduce a regression.

[cloudflare-workers-and-pages]

Deploying agoric-sdk with    Cloudflare Pages

Latest commit:	7b77993
Status:	✅  Deploy successful!
Preview URL:	https://da154411.agoric-sdk.pages.dev
Branch Preview URL:	https://dc-monitor-race.agoric-sdk.pages.dev

View logs

[turadg]

I don't know all the possible ramifications of this change so I don't feel qualified to Approve. I think we need @iomekam or @michaelfig who I think made packet-tools.

[michaelfig]

I think we need @iomekam or @michaelfig who I think made packet-tools.

Guilty as charged! (Thanks for the ping, I'm taking a look now.)

[michaelfig]

I recommend a different tactic that can robustly avoid the race. Let me know what you think.

[michaelfig]

Here's how I would solve the race: check for the cached registration, and if it's not present, atomically replace it with a vow.

        subscribeToTransfers() {
          // Subscribe to the transfers for this account.
          const { lca, reg: cachedReg } = this.state;
          if (cachedReg) {
            return when(cachedReg);
          }
          // Atomically update the registration.
          const { tap } = this.facets;
          const reg = watch(E(lca).monitorTransfers(tap));
          this.state.reg = reg;
          return when(reg);
        },

[michaelfig]

Oh, and the FIXME comment on line 330-331 is no longer needed:

          // FIXME when it returns undefined this causes an error:
          // In "unsubscribeFromTransfers" method of (PacketToolsKit utils): result: undefined "[undefined]" - Must be a promise

It was solved 3 months ago by @0xpatrickdev changing decrPendingPatterns's method guard to use .returns(M.undefined()).

[michaelfig]

Suggested change

	reg: /** @type {Remote<TargetRegistration> | 'RACING' | null} */ (null),
	reg: /** @type {Vow<Remote<TargetRegistration>> | null} */ (null),