Orchestration: Attenuate interceptTransfer (IBC Hooks) method on LocalChainAccount

[0xpatrickdev]

What is the Problem Being Solved?

With #8624, an interceptTransfer method is added to LocalChainAccount. This method is powerful, and might impede* the flow of transfer packets if used incorrectly.

* Impede transfer packets sent only to the LocalChainAccount address, not chain wide.

Description of the Design

LocalChainAccount is a useful object without this method, so we might consider:

• Attenuate LocalChainAccount in vat-localchain, and create a new type to capture an object with the interceptTransfer method. Maybe TransferAccount, VTransferAccount, or InterceptorAccount?

  • This method would also allow us to expose this functionality through a separate capability in BootstrapPowers

• Alternatively, attenuate LocalChainAccount directly in vat-orchestration. Advanced users can request an LCA from vat-localchain directly, and the authority / intended use will be more clear when reviewing code.

Security Considerations

This suggested change is motivated by potential security concerns. I've added the "needs-design" label to give folks an opportunity to weigh in.

Scaling Considerations

Test Plan

Upgrade Considerations

[0xpatrickdev]

And in the course of completing this task, or once we align on design, we should update the type spec:

diff --git a/packages/orchestration/src/types.d.ts b/packages/orchestration/src/types.d.ts
index e41541f39..64ca1aed6 100644
--- a/packages/orchestration/src/types.d.ts
+++ b/packages/orchestration/src/types.d.ts
@@ -12,6 +12,7 @@ import type {
   Redelegation,
   UnbondingDelegation,
 } from '@agoric/cosmic-proto/cosmos/staking/v1beta1/staking.js';
+import type { LocalChainAccount } from '@agoric/vats/src/localchain';

 export type AttenuatedNetwork = Pick<RouterProtocol, 'bindPort'>;

@@ -108,8 +109,25 @@ export type AmountArg = ChainAmount | Amount;
 export interface Orchestrator {
   getChain: <C extends keyof KnownChains>(chainName: C) => Promise<Chain<C>>;

-  makeLocalAccount: () => Promise<LocalChainAccount>;
-
+  /**
+   * creates a module account on the local chain (agoric)
+   * can be used to send and receive tokens over ibc, or any
+   * `Proto3JSONMsg` msg supported by the local chain
+   *
+   * @internal
+   * alternatively, can get one from:
+   * E(E(orchestration).getChain('agoric')).createAccount() => Promise<LocalChainAccount>;
+   */
+  createLocalAccount: () => Promise<
+    Omit<LocalChainAccount, 'interceptTransfer'>
+  >;
+  /**
+   * @throws not yet implemented
+   * @internal
+   * placeholder, for when `interceptTransfer` is safe for all `OrchestrationService` consumers
+   */
+  // TODO is `createInterceptorAccount`, `createIBCHooksAccount` a better name?
+  createTransferAccount: () => Promise<LocalChainAccount>;
   /**
    * For a denom, return information about a denom including the equivalent
    * local Brand, the Chain on which the denom is held, and the Chain that

[LuqiPan]

And in the course of completing this task, or once we align on design, we should update the type spec:

diff --git a/packages/orchestration/src/types.d.ts b/packages/orchestration/src/types.d.ts
index e41541f39..64ca1aed6 100644
--- a/packages/orchestration/src/types.d.ts
+++ b/packages/orchestration/src/types.d.ts
@@ -12,6 +12,7 @@ import type {
   Redelegation,
   UnbondingDelegation,
 } from '@agoric/cosmic-proto/cosmos/staking/v1beta1/staking.js';
+import type { LocalChainAccount } from '@agoric/vats/src/localchain';

 export type AttenuatedNetwork = Pick<RouterProtocol, 'bindPort'>;

@@ -108,8 +109,25 @@ export type AmountArg = ChainAmount | Amount;
 export interface Orchestrator {
   getChain: <C extends keyof KnownChains>(chainName: C) => Promise<Chain<C>>;

-  makeLocalAccount: () => Promise<LocalChainAccount>;
-
+  /**
+   * creates a module account on the local chain (agoric)
+   * can be used to send and receive tokens over ibc, or any
+   * `Proto3JSONMsg` msg supported by the local chain
+   *
+   * @internal
+   * alternatively, can get one from:
+   * E(E(orchestration).getChain('agoric')).createAccount() => Promise<LocalChainAccount>;
+   */
+  createLocalAccount: () => Promise<
+    Omit<LocalChainAccount, 'interceptTransfer'>
+  >;
+  /**
+   * @throws not yet implemented
+   * @internal
+   * placeholder, for when `interceptTransfer` is safe for all `OrchestrationService` consumers
+   */
+  // TODO is `createInterceptorAccount`, `createIBCHooksAccount` a better name?
+  createTransferAccount: () => Promise<LocalChainAccount>;
   /**
    * For a denom, return information about a denom including the equivalent
    * local Brand, the Chain on which the denom is held, and the Chain that

Trying to understand this approach, is this bullet point 1?

[LuqiPan]

For posterity, could you also describe/document how authority could be attenuated in each approach and why second bullet point is more clear?

[LuqiPan]

Note to self, this is attenuating the power introduced via #9059

[michaelfig]

Alternatively, attenuate LocalChainAccount directly in vat-orchestration. Advanced users can request an LCA from vat-localchain directly, and the authority / intended use will be more clear when reviewing code.

I have a different suggestion. Since interceptTransfer was deemed too powerful (because it could affect the acknowledgement packet), let's just completely replace it in vat-localchain with a passive-only listener (monitorTransfer), and advanced users can use the much more powerful vat-transfer directly to intercept and rewrite acknowledgements.

diff --git a/packages/vats/src/localchain.js b/packages/vats/src/localchain.js
index 06e2dfaa5..53567db1c 100644
--- a/packages/vats/src/localchain.js
+++ b/packages/vats/src/localchain.js
@@ -34,9 +34,9 @@ export const LocalChainAccountI = M.interface('LocalChainAccount', {
     .optional(M.pattern())
     .returns(AmountShape),
   executeTx: M.callWhen(M.arrayOf(M.record())).returns(M.arrayOf(M.record())),
-  interceptTransfers: M.callWhen()
-    .optional(M.remotable('TransferTap'))
-    .returns(M.opt(M.remotable('Unregistrar'))),
+  monitorTransfers: M.callWhen(M.remotable('TransferTap')).returns(
+    M.remotable('Unregistrar'),
+  ),
 });
 
 /** @param {import('@agoric/base-zone').Zone} zone */
@@ -90,13 +90,10 @@ const prepareLocalChainAccount = zone =>
         const system = getPower(powers, 'system');
         return E(system).toBridge(obj);
       },
-      async interceptTransfers(tap) {
+      async monitorTransfers(tap) {
         const { address, powers } = this.state;
         const transfer = getPower(powers, 'transfer');
-        if (tap) {
-          return E(transfer).registerTap(address, tap);
-        }
-        await E(transfer).unregisterTap(address);
+        return E(transfer).registerTap(address, tap);
       },
     },
   );
diff --git a/packages/vats/src/transfer.js b/packages/vats/src/transfer.js
index f9185ee97..03b608a4d 100644
--- a/packages/vats/src/transfer.js
+++ b/packages/vats/src/transfer.js
@@ -12,43 +12,49 @@ export const prepareTransferInterceptor = zone =>
     'TransferInterceptor',
     AppI,
     /**
+     * @param {boolean} isActiveTap Whether the tap is active (can modify
+     *   acknowledgments), or passive (can't modify the middleware results).
      * @param {ERef<import('./bridge-target.js').System>} system
      * @param {ERef<import('./bridge-target.js').App>} tap
      */
-    (system, tap) => ({ system, tap }),
+    (isActiveTap, system, tap) => ({ isActiveTap, system, tap }),
     {
       async upcall(obj) {
-        const { system, tap } = this.state;
+        const { isActiveTap, system, tap } = this.state;
 
         obj.type === 'VTRANSFER_IBC_EVENT' ||
           Fail`Invalid upcall argument type ${obj.type}; expected VTRANSFER_IBC_EVENT`;
 
         // First, call our target contract listener.
-        // A VTransfer interceptor can return a write acknowledgement
+        // A VTransfer active interceptor can return a write acknowledgement
         let retP = E(tap).upcall(obj);
 
         // See if the upcall result needs special handling.
         if (obj.event === 'writeAcknowledgement') {
-          // Allow the upcall result to trigger an ack.
           const ackMethod = {
             type: 'IBC_METHOD',
             method: 'receiveExecuted',
             packet: obj.packet,
           };
-          // FIXME: use the Vow `watch` operator.
-          retP = retP
-            .then(rawAck => {
-              // Write out the encoded ack.
+          if (isActiveTap) {
+            // FIXME: use the Vow `watch` operator.
+            retP = retP.then(rawAck => {
+              // Encode the tap's ack and write it out.
               const ack = dataToBase64(coerceToData(rawAck));
               ackMethod.ack = ack;
               return E(system).downcall(ackMethod);
-            })
-            .catch(error => {
-              console.error(`Error sending ack:`, error);
-              const rawAck = JSON.stringify({ error: error.message });
-              ackMethod.ack = dataToBase64(rawAck);
-              return E(system).downcall(ackMethod);
             });
+          } else {
+            // This is a passive tap, so forward the ack without intervention.
+            ackMethod.ack = obj.acknowledgement;
+            retP = E(system).downcall(ackMethod);
+          }
+          retP.catch(error => {
+            console.error(`Error sending ack:`, error);
+            const rawAck = JSON.stringify({ error: error.message });
+            ackMethod.ack = dataToBase64(rawAck);
+            return E(system).downcall(ackMethod);
+          });
         }
 
         // Log errors in the upcall handling.
@@ -65,6 +71,10 @@ const TransferMiddlewareI = M.interface('TransferMiddleware', {
     M.string(),
     M.remotable('TransferInterceptor'),
   ).returns(M.remotable('TargetUnregister')),
+  registerActiveTap: M.callWhen(
+    M.string(),
+    M.remotable('TransferInterceptor'),
+  ).returns(M.remotable('TargetUnregister')),
   unregisterTap: M.callWhen(M.string()).returns(),
 });
 
@@ -92,7 +102,15 @@ const prepareTransferMiddleware = (zone, makeTransferInterceptor) =>
 
         // Wrap the tap so that its upcall results determine how to contact the
         // system.  Never allow the tap to send to the system directly.
-        const interceptor = makeTransferInterceptor(system, tap);
+        const interceptor = makeTransferInterceptor(false, system, tap);
+        return E(targetRegistry).register(target, interceptor);
+      },
+      async registerActiveTap(target, tap) {
+        const { system, targetRegistry } = this.state;
+
+        // Wrap the tap and allow it to modify acknowledgements.
+        // system.  Never allow the tap to send to the system directly.
+        const interceptor = makeTransferInterceptor(true, system, tap);
         return E(targetRegistry).register(target, interceptor);
       },
       /**

[0xpatrickdev]

Trying to understand this approach, is this bullet point 1?

It is maybe a distraction to the issue at hand, was trying to also get feedback on how we want to incorporate this in Orchestration. Probably can ignore the createTransferAccount bit for now, which also seems irrelevant in light of @michaelfig's suggestion, and can expect to see just createLocalAccount in the first iteration.

For posterity, could you also describe/document how authority could be attenuated in each approach and why second bullet point is more clear?

When #8624 lands, it would be adding additional authority (interceptTransfer) to LocalchainAccount. In this first second bullet, our BootstrapPowers would not distinguish between the two - localchain power gives you LocalchainAccount with query + executeTx functionality but also interceptTransfer.

In the second first bullet, I am asking for a separate object in the bootstrap space to capture an LCA with interceptTransfer.

The impact is more from a documentation / readability standpoint - it's much easier to quickly scan a proposal and get a sense of the authority it requires when BootstrapPowers are more granular.

I have a different suggestion. Since interceptTransfer was deemed too powerful (because it could affect the acknowledgement packet), let's just completely replace it in vat-localchain with a passive-only listener (monitorTransfer), and advanced users can use the much more powerful vat-transfer directly to intercept and rewrite acknowledgements.

Awesome, thanks @michaelfig. Mainly just wanted to get a separate bootstrap power out of this, which seems like it'll be transferMiddleware with your suggestions.

[LuqiPan]

This issue slipped through my radar until now.

I have a different suggestion. Since interceptTransfer was deemed too powerful (because it could affect the acknowledgement packet), let's just completely replace it in vat-localchain with a passive-only listener (monitorTransfer), and advanced users can use the much more powerful vat-transfer directly to intercept and rewrite acknowledgements.

Just wanna say I like this suggestion!