Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(orchestration): deposit ERTP payment to ICA #9342

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented May 8, 2024

closes: #9193
closes: #9042

Description

Security Considerations

  • Offer Safety: withdrawFromSeat is needed to retrieve a live payment from a seat. Offer safety is upheld because the user is not allowed to request a Want, but we need to proceed carefully. There is a chance the deposit fails along the way, and we need to ensure the payment can be recovered and deposited back to a users seat.

  • chainTimerService is needed to create a timeout parameter for MsgTransfer

Scaling Considerations

  • In order for a ChainAccount to have a deposit() functionality, a LocalChainAccount needs to be created. This could lead to a lot of LCA's

    • given the LCA is currently closed over in state, and might wind up with funds the holder needs to access, we might want to consider exposing interfaces for the LCA's (.withdraw, .executeTx, .query)
  • TimerService is needed to create a MsgTransfer timeout parameter. See Orchestration: optimize timeoutHeight parameter for transactions, queries, and transfers #9324 for more details

Documentation Considerations

Testing Considerations

  • I wanted to add more explicit tests for the functionality added to LocalChainAccount, but struggled to find a place to add them (and get access to a mint in bootstrap tests). Both deposit() and withdraw() are tested indirectly by tests in boot/../test-orchestration.ts

  • I would like feedback on the imperative .deposit() flow - what happens if this throws? Can we assume the callers supplied payment is still live if it wasn't deposited to the LCA?

Upgrade Considerations

Copy link

cloudflare-pages bot commented May 8, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 37083e6
Status: ✅  Deploy successful!
Preview URL: https://37ad759a.agoric-sdk.pages.dev
Branch Preview URL: https://9193-deposit-to-ica.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev requested review from turadg and dckc May 8, 2024 18:10
trace('Transferring funds to ICA');
/**
* // TODO can we infer `/ibc.applications.transfer.v1.MsgTransferResponse`?
* @type {unknown[]}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My current (unsuccessful) attempt at this:

packages/cosmic-proto/src/helpers.ts
git diff packages/cosmic-proto/src/helpers.ts
diff --git a/packages/cosmic-proto/src/helpers.ts b/packages/cosmic-proto/src/helpers.ts
index ca192be60..00ebfea30 100644
--- a/packages/cosmic-proto/src/helpers.ts
+++ b/packages/cosmic-proto/src/helpers.ts
@@ -1,6 +1,15 @@
-import type { QueryAllBalancesRequest } from './codegen/cosmos/bank/v1beta1/query.js';
-import type { MsgSend } from './codegen/cosmos/bank/v1beta1/tx.js';
-import type { MsgDelegate } from './codegen/cosmos/staking/v1beta1/tx.js';
+import type {
+  QueryAllBalancesRequest,
+  QueryAllBalancesResponse,
+} from './codegen/cosmos/bank/v1beta1/query.js';
+import type {
+  MsgSend,
+  MsgSendResponse,
+} from './codegen/cosmos/bank/v1beta1/tx.js';
+import type {
+  MsgDelegate,
+  MsgDelegateResponse,
+} from './codegen/cosmos/staking/v1beta1/tx.js';
 import { RequestQuery } from './codegen/tendermint/abci/types.js';
 import type { Any } from './codegen/google/protobuf/any.js';
 import {
@@ -13,17 +22,32 @@ import {
  * `unknown` but it's actually this. The `value` string is a base64 encoding of
  * the bytes array.
  */
-export type AnyJson = { typeUrl: string; value: string };
-
-// TODO codegen this by modifying Telescope
+export type AnyJson = {
+  typeUrl: string;
+  value: string;
+};
 export type Proto3Shape = {
   '/cosmos.bank.v1beta1.MsgSend': MsgSend;
+  '/cosmos.bank.v1beta1.MsgSendResponse': MsgSendResponse;
   '/cosmos.bank.v1beta1.QueryAllBalancesRequest': QueryAllBalancesRequest;
+  '/cosmos.bank.v1beta1.QueryAllBalancesResponse': QueryAllBalancesResponse;
   '/cosmos.staking.v1beta1.MsgDelegate': MsgDelegate;
+  '/cosmos.staking.v1beta1.MsgDelegateResponse': MsgDelegateResponse;
   '/ibc.applications.transfer.v1.MsgTransfer': MsgTransfer;
   '/ibc.applications.transfer.v1.MsgTransferResponse': MsgTransferResponse;
 };

+export type ResponseType<T> =
+  T extends '/ibc.applications.transfer.v1.MsgTransfer'
+    ? '/ibc.applications.transfer.v1.MsgTransferResponse'
+    : T extends '/cosmos.bank.v1beta1.MsgSend'
+      ? '/cosmos.bank.v1beta1.MsgSendResponse'
+      : T extends '/cosmos.staking.v1beta1.MsgDelegate'
+        ? '/cosmos.staking.v1beta1.MsgDelegateResponse'
+        : T extends '/cosmos.bank.v1beta1.QueryAllBalancesRequest'
+          ? '/cosmos.bank.v1beta1.QueryAllBalancesResponse'
+          : never;
+
 /**
  * The encoding introduced in Protobuf 3 for Any that can be serialized to JSON.
  *
packages/vats/src/localchain.js
git diff packages/vats/src/localchain.js
diff --git a/packages/vats/src/localchain.js b/packages/vats/src/localchain.js
index 5a07e8ad3..6ab60c379 100644
--- a/packages/vats/src/localchain.js
+++ b/packages/vats/src/localchain.js
@@ -5,6 +5,8 @@ import { AmountShape } from '@agoric/ertp';

 const { Fail, bare } = assert;

+/** @import {TypedJson, ResponseType, Proto3Shape} from '@agoric/cosmic-proto'; */
+
 /**
  * @typedef {{
  *   system: import('./types.js').ScopedBridgeManager;
@@ -82,8 +84,9 @@ const prepareLocalChainAccount = zone =>
         return E(purse).withdraw(amount);
       },
       /**
-       * @param {import('@agoric/cosmic-proto').TypedJson<unknown>[]} messages
-       * @returns {Promise<import('@agoric/cosmic-proto').TypedJson[]>}
+       * @template {keyof Proto3Shape} T
+       * @param {TypedJson<T>[]} messages
+       * @returns {Promise<TypedJson<ResponseType<T>>[]>}
        */
       async executeTx(messages) {
         const { address, powers } = this.state;
@@ -178,8 +181,8 @@ const prepareLocalChain = (zone, makeAccount) =>
          * the query fails. Otherwise, return the response as a JSON-compatible
          * object.
          *
-         * @param {import('@agoric/cosmic-proto').TypedJson} request
-         * @returns {Promise<import('@agoric/cosmic-proto').TypedJson>}
+         * @param {TypedJson} request
+         * @returns {Promise<TypedJson>}
          */
         async query(request) {
           const requests = harden([request]);
@@ -197,11 +200,11 @@ const prepareLocalChain = (zone, makeAccount) =>
          * system error, will return all results to indicate their success or
          * failure.
          *
-         * @param {import('@agoric/cosmic-proto').TypedJson[]} requests
+         * @param {TypedJson[]} requests
          * @returns {Promise<
          *   {
          *     error?: string;
-         *     reply: import('@agoric/cosmic-proto').TypedJson;
+         *     reply: TypedJson;
          *   }[]
          * >}
          */

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

]);
trace('MsgTransfer result', result);

return /** @type {{ sequence: number }} */ (result);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was expecting to see a bigint or string here. Surprised to see a number, but this is what I observed with local chains.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that observed data captured in a unit test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's currently captured in packages/boot/tools/supports.ts: https://github.com/Agoric/agoric-sdk/pull/9342/files#diff-5f4f231637e90b9cdfecc29b28c80beb222607ad154bbc266cbf52c6a7c246e9R438-R446

I considered adding a new type helper to @agoric/cosmic-proto that's like JsonSafe, but wanted to be sure of this behavior before proceeding. @michaelfig - do you have any insights here? Are bigints cast to numbers for the LocalChainAccount implementation?

/**
* see stakingAccountKit.js for an example until #9212
* @param {Payment} payment
*/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given Scaling Considerations, we may want to reconsider including deposit() on ChainAccount.

@@ -5,6 +5,8 @@ import { AmountShape } from '@agoric/ertp';

const { Fail, bare } = assert;

/** @import {TypedJson, ResponseType, Proto3Shape} from '@agoric/cosmic-proto'; */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @import {TypedJson, ResponseType, Proto3Shape} from '@agoric/cosmic-proto'; */
/** @import {TypedJson, Proto3Shape} from '@agoric/cosmic-proto'; */

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wouldn't worry about extra types for now. especially because ambients can make it look like imports that aren't used that actually are during build time

"sourceChannelId": "channel-1",
"sourcePortId": "transfer"
},
"icqEnabled": false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set true for tests, where query responses are mocked, but won't work on a local simulated chain

@@ -295,6 +295,7 @@ export const makeSwingsetTestKit = async (

let inbound;
let ibcSequenceNonce = 0;
let icaExecuteTxSequence = 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let icaExecuteTxSequence = 0;
let lcaExecuteTxSequence = 0;

@@ -215,3 +339,5 @@ test.serial('stakeAtom - smart wallet', async t => {
'delegate fails with invalid validator',
);
});

test.todo('deposit to LCA fails, payment should be returned');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently only simulating failed LCA -> ICA transfer. This captures the last nested block of the Deposit() try / catch, but we have nothing testing the first depositToSeat, which depends on await Object.values(payments)[0]. Suggestions on how to simulate this welcome.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time for a meeting; better share my notes so far

@@ -77,6 +77,11 @@ export const AmountShape = harden({
value: AmountValueShape,
});

export const NatAmountShape = harden({
brand: BrandShape,
value: NatValueShape,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a NatValueShape? even though it's the same as M.nat()? odd.

@@ -1,3 +1,4 @@
/* eslint-disable camelcase */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not obvious why camelcase should be disabled. is it easy to add a few words? no big deal for test code, I guess

@@ -150,6 +151,7 @@ test.serial('stakeAtom - smart wallet', async t => {
'agoric1testStakAtom',
);

// 1. Make Account
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider turning such comments into t.log() calls.

odd... makeAcountInvitationMaker? with 1 c? oops!

},
proposal: {
give: {
// @ts-expect-error BoardRemote is not assignable to Brand<any>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I/we really should fix that. the getBoardId() kludge is a bad idea. the mapping from value to slot (board id) belongs in a table external to the object; we learned this in makeClientMarshaller but we haven't back-ported it all the way to here.

IOU an issue, I guess.

});
t.like(wd.getLatestUpdateRecord(), {
status: { id: 'request-deposit-success', numWantsSatisfied: 1 },
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I'd like to see a check that the money got there. Can we capture calls across the bridge and check for a suitable one here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best we can do in this environment is mock, which doesn't give the highest confidence or seem worth the cost. I am going to pursue better unit testing at the exo level per your suggestion to get more confidence around these failure cases

},
}),
{
message: 'Deposit failed, payment returned.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, it would be good to have evidence that the right vbank messages went over the bridge

ah... but we do have payouts below

bondDenom = 'uatom',
bondDenomLocal = 'ibc/C4CFF46FD6DE35CA4CF4CE031E643C8FDC9BA4B99AE598E9B0ED98FE3A2319F9',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be nice to get this from hashing... or to test that this matches what we get from hashing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Something i think we'll tackle as part of #9211

@@ -50,25 +73,40 @@ export const start = async (zcf, privateArgs, baggage) => {
zcf,
);

/** @type {BrandToIssuer} */
const brandToIssuer = zone.mapStore('brandToIssuer');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a durable store is over-kill for this. it costs a syscall for each access.

Comment on lines +89 to +91
const icqConnection = icqEnabled
? await E(orchestration).provideICQConnection(controllerConnectionId)
: undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's ok by the new await safety style, but I've picked up an aversion to code that sometimes represents a turn boundary and sometimes doesn't.

Suggested change
const icqConnection = icqEnabled
? await E(orchestration).provideICQConnection(controllerConnectionId)
: undefined;
const icqConnection = await (icqEnabled
? E(orchestration).provideICQConnection(controllerConnectionId)
: undefined);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also reminded to check for a clear commit point, as in the prepare / commit pattern.

If makeAccount() succeeds but provideICQConnection() fails, should we release the account?

@@ -77,6 +77,11 @@ export const AmountShape = harden({
value: AmountValueShape,
});

export const NatAmountShape = harden({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const NatAmountShape = harden({
/** @see {makeNatAmountShape} to match a specific brand */
export const NatAmountShape = harden({

Comment on lines 86 to 87
numerator: AmountShape,
denominator: AmountShape,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
numerator: AmountShape,
denominator: AmountShape,
numerator: NatAmountShape,
denominator: NatAmountShape,


// 2. Deposit to Account
await wd.executeOffer({
id: 'request-deposit-success',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

success is independent of the offer identity

Suggested change
id: 'request-deposit-success',
id: 'request-deposit',

reading further I see you have other variants. Still I think success or failure are independent of identity. E.g.

request-deposit
request-deposit-bad-want
request-deposit-two-gives

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tripped over that name/id on 1st reading too.

status: { id: 'request-deposit-success', numWantsSatisfied: 1 },
});

await t.throwsAsync(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to have these validation tests, but they don't need to integrate with smart-wallet. Spinning up a contract in isolation is a bit of work, I don't know that they even need to integrate with stakeAtom. Could these just be tests of the StakingAccountKit exo? Those would run very fast

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged and agree: #9342 (comment)

@@ -60,7 +61,8 @@
},
"files": [
"test/**/*.test.js",
"test/**/*.test.ts"
"test/**/*.test.ts",
"test/**/test-*.js"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminds me to revive #8653

// messages from the LCA (e.g., withdraw funds)
await E(localAccount).deposit(payment);

const timeoutTimestamp =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a conditional await. I think it's safe because of the await above it, but please add a // @jessie-check at the top to be sure. all modules that run in a vat should pass jessie-check

trace('Transferring funds to ICA');
/**
* // TODO can we infer `/ibc.applications.transfer.v1.MsgTransferResponse`?
* @type {unknown[]}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +67 to +68
const chainTimerService = await chainTimerServiceP;
const chainTimerBrand = await E(chainTimerService).getTimerBrand();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not pipeline?

Suggested change
const chainTimerService = await chainTimerServiceP;
const chainTimerBrand = await E(chainTimerService).getTimerBrand();
const chainTimerBrand = await E(chainTimerServiceP).getTimerBrand();

@@ -266,7 +267,7 @@ export interface ChainAccount {
opts?: Partial<Omit<TxBody, 'messages'>>,
) => Promise<string>;
/** deposit payment from zoe to the account*/
deposit: (payment: Payment) => Promise<void>;
deposit: (payment: Payment) => Promise<{ sequence: number }>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtribble wants to review changes to this API

@@ -5,6 +5,8 @@ import { AmountShape } from '@agoric/ertp';

const { Fail, bare } = assert;

/** @import {TypedJson, ResponseType, Proto3Shape} from '@agoric/cosmic-proto'; */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wouldn't worry about extra types for now. especially because ambients can make it look like imports that aren't used that actually are during build time

@turadg turadg mentioned this pull request May 8, 2024
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm running out of steam for today.

But at least the bankManager change is critical.

Comment on lines +252 to +253
this.state.brandToIssuer.has(giveAmount.brand) ||
Fail`${giveAmount.brand} not registered`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contracts can rely on Zoe to ensure that brands in proposals are registered.

await depositToSeat(zcf, seat, give, payments);
throw Fail`Deposit failed, payment returned.`;
} catch (error) {
if (error.message.includes('not a live paymen')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our style for errors and control flow is to not peek at errors.

Why do we care what sort of error it is?

await depositToSeat(zcf, seat, give, payments);
throw Fail`Deposit failed, payment returned.`;
} catch (error) {
if (error.message.includes('not a live paymen')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, are we testing all these failure modes?

I wonder if it would help to hoist much of this logic to a function that we can unit test.

Comment on lines +23 to +28
/**
* - `give` allows any `Nat` `issuerKeyword` record. Must be exactly one entry.
* - `exit` must be `{ waived: null }`
* - `want` must be empty
*/
export const DepositProposalShape = M.splitRecord(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waived: null merits some explanation: once somebody makes such an offer, undoing it may be infeasible, so the client can't honor exit requests; clients must waive their right to exit.

docs should bear a WARNING notice about lack of offer safety

maybe the name should be scarier too... GiveAndHope :)

maybe these docs belong on the deposit() method and/or the Deposit invitationMaker.

const { address, powers } = this.state;
const bankManager = getPower(powers, 'bankManager');

const acctsBank = E(bankManager).getBankForAddress(address);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This getBankForAddress() call should happen before this account object is created. No account should have access to everybody else's purses. critical

(Didn't I give this feedback a while ago? hm.)

@erights erights self-requested a review May 8, 2024 23:59
mergify bot added a commit that referenced this pull request May 9, 2024
Refs: #9342

## Description

Make it so executeTx gives the proper response message for each request
message.

### Security Considerations

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations

<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

### Upgrade Considerations

<!-- What aspects of this PR are relevant to upgrading live production
systems, and how should they be addressed? -->
@0xpatrickdev 0xpatrickdev marked this pull request as draft May 9, 2024 17:35
mergify bot added a commit that referenced this pull request May 10, 2024
refs: #9342, #9193
stacked on

 - #9353

## Description

Provide localchain accounts only with the `bank` for the relevant
address, rather than giving them access to all accounts in the form of
the `bankManager`.

earlier discussion:
https://github.com/Agoric/agoric-sdk/pull/9342/files#r1594791187

### Security Considerations

bankManager was excess authority

### Scaling / Documentation / Testing Considerations

Nothing significant: no scaling changes; existing docs/tests suffice.

### Upgrade Considerations

code is not released / deployed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

orchestration - ICA Controller - deposit payment to ICA user story: StakeAtom
3 participants