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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/ERTP/src/typeGuards.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({

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.

});

export const RatioShape = harden({
numerator: AmountShape,
denominator: AmountShape,
Comment on lines 86 to 87
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,

Expand Down
132 changes: 129 additions & 3 deletions packages/boot/test/bootstrapTests/test-orchestration.ts
Original file line number Diff line number Diff line change
@@ -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

import { test as anyTest } from '@agoric/zoe/tools/prepare-test-env-ava.js';

import type { TestFn } from 'ava';
Expand Down Expand Up @@ -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!

await wd.executeOffer({
id: 'request-account',
invitationSpec: {
Expand All @@ -167,9 +169,131 @@ test.serial('stakeAtom - smart wallet', async t => {
t.like(wd.getLatestUpdateRecord(), {
status: { id: 'request-account', numWantsSatisfied: 1 },
});

const { ATOM } = agoricNamesRemotes.brand;
const { ATOM, DAI_axl, BLD } = agoricNamesRemotes.brand;
ATOM || Fail`ATOM missing from agoricNames`;
DAI_axl || Fail`DAI_axl missing from agoricNames`;
BLD || Fail`BLD missing from agoricNames`;

// 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.

invitationSpec: {
source: 'continuing',
previousOffer: 'request-account',
invitationMakerName: 'Deposit',
},
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.

ATOM: { brand: ATOM, value: 100n },
},
exit: { waived: null },
},
});
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


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)

wd.executeOffer({
id: 'request-deposit-failure-no-want-allowed',
invitationSpec: {
source: 'continuing',
previousOffer: 'request-account',
invitationMakerName: 'Deposit',
},
proposal: {
give: {
// @ts-expect-error BoardRemote is not assignable to Brand<any>
ATOM: { brand: ATOM, value: 100n },
},
want: {
// @ts-expect-error BoardRemote is not assignable to Brand<any>
BLD: { brand: BLD, value: 100n },
},
exit: { waived: null },
},
}),
{
message: /proposal: want(.*?)Must be: {}/,
},
);

await t.throwsAsync(
wd.executeOffer({
id: 'request-deposit-failure-two-give-amounts',
invitationSpec: {
source: 'continuing',
previousOffer: 'request-account',
invitationMakerName: 'Deposit',
},
proposal: {
give: {
// @ts-expect-error BoardRemote is not assignable to Brand<any>
ATOM: { brand: ATOM, value: 100n },
// @ts-expect-error BoardRemote is not assignable to Brand<any>
BLD: { brand: BLD, value: 100n },
},
exit: { waived: null },
},
}),
{
message: /proposal: give: Must not have more than 1 properties/,
},
);

await t.throwsAsync(
wd.executeOffer({
id: 'request-deposit-failure-unknown-issuer',
invitationSpec: {
source: 'continuing',
previousOffer: 'request-account',
invitationMakerName: 'Deposit',
},
proposal: {
give: {
// @ts-expect-error BoardRemote is not assignable to Brand<any>
DAI_axl: { brand: DAI_axl, value: 100n },
},
exit: { waived: null },
},
}),
{
message: /brand(.*?)not registered/,
},
);

await t.throwsAsync(
wd.executeOffer({
id: 'request-deposit-failure-transfer-packet-timeout',
invitationSpec: {
source: 'continuing',
previousOffer: 'request-account',
invitationMakerName: 'Deposit',
},
proposal: {
give: {
// @ts-expect-error BoardRemote is not assignable to Brand<any>
ATOM: { brand: ATOM, value: 504n },
},
exit: { waived: null },
},
}),
{
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

},
);
t.like(wd.getLatestUpdateRecord(), {
status: {
id: 'request-deposit-failure-transfer-packet-timeout',
numWantsSatisfied: 1,
payouts: {
ATOM: { value: 504n },
},
},
});

// 3. Delegate from Account to Validator
const validatorAddress: CosmosValidatorAddress = {
address: 'cosmosvaloper1test',
chainId: 'gaiatest',
Expand All @@ -192,12 +316,12 @@ test.serial('stakeAtom - smart wallet', async t => {
status: { id: 'request-delegate-success', numWantsSatisfied: 1 },
});

// 4. Delegate Failure (invalid validator address or amount)
const validatorAddressFail: CosmosValidatorAddress = {
address: 'cosmosvaloper1fail',
chainId: 'gaiatest',
addressEncoding: 'bech32',
};

await t.throwsAsync(
wd.executeOffer({
id: 'request-delegate-fail',
Expand All @@ -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.

23 changes: 20 additions & 3 deletions packages/boot/tools/supports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;


const makeAckEvent = (obj: IBCMethod<'sendPacket'>, ack: string) => {
ibcSequenceNonce += 1;
Expand Down Expand Up @@ -429,9 +430,25 @@ export const makeSwingsetTestKit = async (
switch (obj.type) {
case 'VLOCALCHAIN_ALLOCATE_ADDRESS':
return 'agoric1mockVlocalchainAddress';
case 'VLOCALCHAIN_EXECUTE_TX':
// returns one empty object per message
return obj.messages.map(() => ({}));
case 'VLOCALCHAIN_EXECUTE_TX': {
icaExecuteTxSequence += 1;
// returns one empty object per message unless specified
return obj.messages.map(message => {
switch (message['@type']) {
case '/ibc.applications.transfer.v1.MsgTransfer': {
if (message.token.amount === '504') {
throw Error('simulated MsgTransfer packet timeout');
}
// like `JsonSafe<MsgTransferResponse>`, but bigints are converted to numbers
return {
sequence: icaExecuteTxSequence,
};
}
default:
return {};
}
});
}
default:
throw Error(`VLOCALCHAIN message of unknown type ${obj.type}`);
}
Expand Down
13 changes: 12 additions & 1 deletion packages/builders/scripts/orchestration/init-stakeAtom.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,16 @@ export const defaultProposalBuilder = async (
) => {
const {
hostConnectionId = 'connection-1',
controllerConnectionId = 'connection-0',
controllerConnectionId = 'connection-1',
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

transferChannel = {
counterpartyChannelId: 'channel-1',
counterpartyPortId: 'transfer',
sourceChannelId: 'channel-1',
sourcePortId: 'transfer',
},
icqEnabled = true,
} = options;
return harden({
sourceSpec: '@agoric/orchestration/src/proposals/start-stakeAtom.js',
Expand All @@ -23,6 +31,9 @@ export const defaultProposalBuilder = async (
hostConnectionId,
controllerConnectionId,
bondDenom,
bondDenomLocal,
transferChannel,
icqEnabled,
},
],
});
Expand Down
2 changes: 2 additions & 0 deletions packages/cosmic-proto/src/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { MsgSend } from './codegen/cosmos/bank/v1beta1/tx.js';
import type { MsgDelegate } 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 { MsgTransfer } from './codegen/ibc/applications/transfer/v1/tx.js';

/**
* The result of Any.toJSON(). The type in cosms-types says it returns
Expand All @@ -16,6 +17,7 @@ export type Proto3Shape = {
'/cosmos.bank.v1beta1.MsgSend': MsgSend;
'/cosmos.bank.v1beta1.QueryAllBalancesRequest': QueryAllBalancesRequest;
'/cosmos.staking.v1beta1.MsgDelegate': MsgDelegate;
'/ibc.applications.transfer.v1.MsgTransfer': MsgTransfer;
};

/**
Expand Down
4 changes: 3 additions & 1 deletion packages/orchestration/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"@endo/patterns": "^1.3.1"
},
"devDependencies": {
"@agoric/swingset-vat": "^0.32.2",
"@cosmjs/amino": "^0.32.3",
"@cosmjs/proto-signing": "^0.32.3",
"@endo/ses-ava": "^1.2.1",
Expand All @@ -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

],
"nodeArguments": [
"--loader=tsx",
Expand Down
70 changes: 54 additions & 16 deletions packages/orchestration/src/examples/stakeAtom.contract.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,34 +11,57 @@ import { prepareStakingAccountKit } from '../exos/stakingAccountKit.js';

const trace = makeTracer('StakeAtom');
/**
* @import { Baggage } from '@agoric/vat-data';
* @import { IBCConnectionID } from '@agoric/vats';
* @import { ICQConnection, OrchestrationService } from '../types.js';
* @import {Baggage} from '@agoric/vat-data';
* @import {IBCConnectionID} from '@agoric/vats';
* @import {LocalChain} from '@agoric/vats/src/localchain.js';
* @import {IBCChannelInfo, OrchestrationService, BrandToIssuer} from '@agoric/orchestration';
* @import {TimerBrand, TimerService} from '@agoric/time'
*/

/**
* @typedef {{
* hostConnectionId: IBCConnectionID;
* controllerConnectionId: IBCConnectionID;
* bondDenom: string;
* bondDenomLocal: string;
* transferChannel: IBCChannelInfo;
* icqEnabled: boolean;
* chainTimerBrand: TimerBrand;
* }} StakeAtomTerms
*/

/**
*
* @param {ZCF<StakeAtomTerms>} zcf
* @param {{
* localchain: LocalChain;
* orchestration: OrchestrationService;
* storageNode: StorageNode;
* marshaller: Marshaller;
* chainTimerService: TimerService;
* }} privateArgs
* @param {Baggage} baggage
*/
export const start = async (zcf, privateArgs, baggage) => {
// TODO #9063 this roughly matches what we'll get from Chain<C>.getChainInfo()
const { hostConnectionId, controllerConnectionId, bondDenom } =
zcf.getTerms();
const { orchestration, marshaller, storageNode } = privateArgs;
const {
hostConnectionId,
controllerConnectionId,
bondDenom,
bondDenomLocal,
transferChannel,
issuers,
brands,
icqEnabled,
chainTimerBrand,
} = zcf.getTerms();
const {
localchain,
orchestration,
marshaller,
storageNode,
chainTimerService,
} = privateArgs;

const zone = makeDurableZone(baggage);

Expand All @@ -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.

for (const [keyword, brand] of Object.entries(brands)) {
brandToIssuer.init(brand, issuers[keyword]);
}

async function makeAccount() {
const account = await E(orchestration).makeAccount(
hostConnectionId,
controllerConnectionId,
);
// #9212 TODO do not fail if host does not have `async-icq` module;
// communicate to OrchestrationAccount that it can't send queries
const icqConnection = await E(orchestration).provideICQConnection(
controllerConnectionId,
);
const accountAddress = await E(account).getAddress();
trace('account address', accountAddress);
const { holder, invitationMakers } = makeStakingAccountKit(

// TODO #9063, #9212 this should come from Chain object
const icqConnection = icqEnabled
? await E(orchestration).provideICQConnection(controllerConnectionId)
: undefined;
Comment on lines +89 to +91
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?


const localAccount = await E(localchain).makeAccount();
const localAccountAddress = await E(localAccount).getAddress();
const chainAddress = await E(account).getAddress();
const { holder, invitationMakers } = makeStakingAccountKit({
account,
localAccount,
storageNode,
accountAddress,
chainAddress,
localAccountAddress,
icqConnection,
bondDenom,
);
bondDenomLocal,
transferChannel,
brandToIssuer,
chainTimerService,
chainTimerBrand,
});
return {
publicSubscribers: holder.getPublicTopics(),
invitationMakers,
Expand Down
Loading
Loading