Closed 0xpatrickdev closed 3 months ago
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
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?
For posterity, could you also describe/document how authority could be attenuated in each approach and why second bullet point is more clear?
Note to self, this is attenuating the power introduced via #9059
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);
},
/**
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.
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!
What is the Problem Being Solved?
With #8624, an
interceptTransfer
method is added toLocalChainAccount
. 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
invat-localchain
, and create a new type to capture an object with theinterceptTransfer
method. MaybeTransferAccount
,VTransferAccount
, orInterceptorAccount
?This method would also allow us to expose this functionality through a separate capability in BootstrapPowers
Alternatively, attenuate
LocalChainAccount
directly invat-orchestration
. Advanced users can request an LCA fromvat-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