feat: create chain account (ica) #9114

0xpatrickdev · 2024-03-20T01:30:10Z

closes: #9064
refs: #9042

Description

vat-orchestration - which just has a createAccount() method to start to request an ICA account on a remote chain
E(chainAccount).getAccountAddress() returns a chain account address (as well as .getRemoteAddress() and .getLocalAddress())
Example stakeAtom.contract.js, that creates an ICA on a remote chain.
- expect a 2nd PR for 9042 to send a delegate message
Working towards the spec here: https://github.com/agoric-labs/orchestration-api-spec/blob/main/src/index.d.ts

Outstanding Items

fix @agoric/orchestration -> @agoric/vats -> @agoric/orchestration dependency cycles
getRemoteAddress() returns a stale value - it should match what is logged in "IBC onConnect"
- see fix(network): Connection should include negotiated remoteAddress and localAddress #9186
testing - there's a bootstrap test that's not able to verify the full flow. Interested to get feedback on the approach. I think we may need to build out more network mocking for remote connections.
proto3json - note to self to test if any remote chains currently accept proto3json
- unfortunately, json | proto3-json are not valid for the osmo image i am using (attempt). Tangentially, Hermes does not propagate an error when the negotiating fails (it stops when simulate_tx fails), so the createAccount promise hangs. MF suggests filing a bug.
~~delegate() - protobuf encoding, so we can a message that doesn't throw an error~~
- tackling this in a follow up PR
~~e2e - check in a3p-style test with starship~~
- tackling this in a follow up PR

Security Considerations

The Port object is highly authoritative. The Port ID and Connection ID are used by host chains to determine the account address and recover closed channels.

There should be a single source allocating ports, and networkVat should ensure ports are not reused unless someone has a reference to the original port.

See #9165 where this is tracked.

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

0xpatrickdev · 2024-03-20T01:48:25Z

Performed manual E2E testing following QUICKSTART.md instructions here: 0xpatrickdev/starship-demo#2

(dated) screenshot

Dated because we are now returning the negotiated remote address string when calling getRemoteAddress()

Query ICA Params

# query cosmos
agd query interchain-accounts host params --node http://localhost:26654
allow_messages: []
host_enabled: true

# query osmosis
agd query interchain-accounts host params --node http://localhost:26655
allow_messages:
- '*'
host_enabled: true

Cosmos doesn't have ICA messages enabled in this testing image, so let's test with osmosis for now.

Create Hermes Clients

hermes create client --host-chain agoriclocal --reference-chain osmosis-test && hermes create client --host-chain osmosis-test --reference-chain agoriclocal
2024-03-27T20:27:28.606098Z  INFO ThreadId(01) running Hermes v1.8.0+39036d1
SUCCESS CreateClient(
    CreateClient(
        Attributes {
            client_id: ClientId(
                "07-tendermint-0",
            ),
            client_type: Tendermint,
            consensus_height: Height {
                revision: 0,
                height: 544,
            },
        },
    ),
)
2024-03-27T20:27:32.136044Z  INFO ThreadId(01) running Hermes v1.8.0+39036d1
SUCCESS CreateClient(
    CreateClient(
        Attributes {
            client_id: ClientId(
                "07-tendermint-1",
            ),
            client_type: Tendermint,
            consensus_height: Height {
                revision: 0,
                height: 26,
            },
        },
    ),
)

Create Connection

hermes create connection --a-chain agoriclocal --b-chain osmosis-test
2024-03-27T20:27:35.729425Z  INFO ThreadId(01) running Hermes v1.8.0+39036d1
2024-03-27T20:27:36.254436Z  INFO ThreadId(01) Creating new clients hosted on chains agoriclocal and osmosis-test
2024-03-27T20:27:37.096708Z  INFO ThreadId(01) foreign_client.create{client=osmosis-test->agoriclocal:07-tendermint-0}: 🍭 client was created successfully id=07-tendermint-1
2024-03-27T20:27:37.438693Z  INFO ThreadId(01) foreign_client.create{client=agoriclocal->osmosis-test:07-tendermint-0}: 🍭 client was created successfully id=07-tendermint-2
2024-03-27T20:27:41.980190Z  INFO ThreadId(01) 🥂 agoriclocal => OpenInitConnection(OpenInit { Attributes { connection_id: connection-0, client_id: 07-tendermint-1, counterparty_connection_id: None, counterparty_client_id: 07-tendermint-2 } }) at height 0-28
2024-03-27T20:27:52.934888Z  INFO ThreadId(01) 🥂 osmosis-test => OpenTryConnection(OpenTry { Attributes { connection_id: connection-1, client_id: 07-tendermint-2, counterparty_connection_id: connection-0, counterparty_client_id: 07-tendermint-1 } }) at height 0-573
2024-03-27T20:27:56.250458Z  WARN ThreadId(01) client consensus proof height too high, waiting for destination chain to advance beyond 0-30
2024-03-27T20:27:56.751490Z  WARN ThreadId(01) client consensus proof height too high, waiting for destination chain to advance beyond 0-30
2024-03-27T20:28:02.384917Z  INFO ThreadId(01) 🥂 agoriclocal => OpenAckConnection(OpenAck { Attributes { connection_id: connection-0, client_id: 07-tendermint-1, counterparty_connection_id: connection-1, counterparty_client_id: 07-tendermint-2 } }) at height 0-32
2024-03-27T20:28:07.805043Z  INFO ThreadId(01) 🥂 osmosis-test => OpenConfirmConnection(OpenConfirm { Attributes { connection_id: connection-1, client_id: 07-tendermint-2, counterparty_connection_id: connection-0, counterparty_client_id: 07-tendermint-1 } }) at height 0-591
2024-03-27T20:28:10.832171Z  INFO ThreadId(01) connection handshake already finished for Connection { delay_period: 0ns, a_side: ConnectionSide { chain: BaseChainHandle { chain_id: agoriclocal }, client_id: 07-tendermint-1, connection_id: connection-0 }, b_side: ConnectionSide { chain: BaseChainHandle { chain_id: osmosis-test }, client_id: 07-tendermint-2, connection_id: connection-1 } }
SUCCESS Connection {
    delay_period: 0ns,
    a_side: ConnectionSide {
        chain: BaseChainHandle {
            chain_id: ChainId {
                id: "agoriclocal",
                version: 0,
            },
            runtime_sender: Sender { .. },
        },
        client_id: ClientId(
            "07-tendermint-1",
        ),
        connection_id: Some(
            ConnectionId(
                "connection-0",
            ),
        ),
    },
    b_side: ConnectionSide {
        chain: BaseChainHandle {
            chain_id: ChainId {
                id: "osmosis-test",
                version: 0,
            },
            runtime_sender: Sender { .. },
        },
        client_id: ClientId(
            "07-tendermint-2",
        ),
        connection_id: Some(
            ConnectionId(
                "connection-1",
            ),
        ),
    },
}

Note that `connection-1` will be our `hostConnectionId` (what osmosis calls agoric) and `connection-0` will be our `controllerConnectionId` (what agoric calls osmosis). These will be used to create the ICA connection.

REPL Commands

command[0] E(home.agoricNames).lookup('instance', 'stakeAtom')
history[0] [Object Alleged: InstanceHandle]{}
command[1] E(home.zoe).getPublicFacet(history[0])
history[1] [Object Alleged: StakeAtom]{}
command[2] pf = history[1]
history[2] [Object Alleged: StakeAtom]{}
command[3] E(pf).provideAccount()
history[3] [Object Alleged: ChainAccount]{}
command[4] ica = history[3]
history[4] [Object Alleged: ChainAccount]{}
command[5] E(ica).getLocalAddress()
history[5] "/ibc-port/icacontroller-0"
command[6] E(ica).getAddress()
history[6] "/ibc-hop/connection-0/ibc-port/icahost/ordered/{\"version\":\"ics27-1\",\"controllerConnectionId\":\"connection-0\",\"hostConnectionId\":\"connection-1\",\"address\":\"\",\"encoding\":\"proto3\",\"txType\":\"sdk_multi_msg\"}"
command[7] E(ica).executeEncodedTx('test')
history[7] "{\"error\":\"ABCI code: 2: error handling packet: see events for details\"}"

Swingset Logs

2024-03-27T20:29:12.753Z SwingSet: vat: v50: ----- StakeAtom.2  2 provideAccount. Binding new port
2024-03-27T20:29:12.951Z SwingSet: ls: v15: Logging sent error stack (Error#1)
2024-03-27T20:29:12.952Z SwingSet: ls: v15: Error#1: No listeners for /ibc-hop/connection-0/ibc-port/icahost/ordered/{"version":"ics27-1","controllerConnectionId":"connection-0","hostConnectionId":"connection-1","address":"","encoding":"proto3","txType":"sdk_multi_msg"}
2024-03-27T20:29:12.952Z SwingSet: ls: v15: Error: No listeners for /ibc-hop/connection-0/ibc-port/icahost/ordered/{"version":"ics27-1","controllerConnectionId":"connection-0","hostConnectionId":"connection-1","address":"","encoding":"proto3","txType":"sdk_multi_msg"}
 at apply ()
 at Error (/bundled-source/.../node_modules/ses/src/error/tame-error-constructor.js:54)
 at inbound (.../network/src/network.js:753)
 at inbound (.../network/src/network.js:735)
 at apply ()
 at apply ()
 at ()

2024-03-27T20:29:12.952Z SwingSet: ls: v15: Error#1 ERROR_NOTE: Sent as error:liveSlots:v15#70001
2024-03-27T20:29:12.953Z SwingSet: xsnap: v15: Error#1 ERROR_NOTE: Sent as error:liveSlots:v15#70002
2024-03-27T20:29:12.953Z SwingSet: ls: v15: Logging sent error stack (Error#1)
2024-03-27T20:29:12.953Z SwingSet: xsnap: v15: Error#1 ERROR_NOTE: Sent as error:liveSlots:v15#70003
2024-03-27T20:29:12.953Z SwingSet: ls: v15: Logging sent error stack (Error#1)
2024-03-27T20:29:12.955Z SwingSet: xsnap: v15: Error#1 ERROR_NOTE: Sent as error:liveSlots:v15#70004
2024-03-27T20:29:12.956Z SwingSet: ls: v15: Logging sent error stack (Error#1)
2024-03-27T20:29:12.956Z SwingSet: xsnap: v15: Error#1 ERROR_NOTE: Sent as error:liveSlots:v15#70005
2024-03-27T20:29:12.956Z SwingSet: ls: v15: Logging sent error stack (Error#1)
2024-03-27T20:29:12.960Z SwingSet: xsnap: v15: Error#1 ERROR_NOTE: Sent as error:liveSlots:v15#70006
2024-03-27T20:29:12.960Z SwingSet: ls: v15: Logging sent error stack (Error#1)
2024-03-27T20:29:13.012Z block-manager: block 46 commit
2024-03-27T20:29:17.668Z block-manager: block 47 begin
2024-03-27T20:29:17.894Z block-manager: block 47 commit
Dragonberry Active
2024-03-27T20:29:22.696Z block-manager: block 48 begin
Dragonberry Active
2024-03-27T20:29:22.936Z SwingSet: vat: v16: IBC fromBridge { blockHeight: 48, blockTime: 1711571357, channelID: 'channel-0', connectionHops: [ 'connection-0' ], counterparty: { channel_id: 'channel-1', port_id: 'icahost' }, counterpartyVersion: '{"version":"ics27-1","controller_connection_id":"connection-0","host_connection_id":"connection-1","address":"osmo1d6em9ea5y3dye6em0awqyss7ssp0a7sgjk792x8cx647cfs7a4msk0fr45","encoding":"proto3","tx_type":"sdk_multi_msg"}', event: 'channelOpenAck', portID: 'icacontroller-0', type: 'IBC_EVENT' }
2024-03-27T20:29:22.974Z SwingSet: vat: v18: ----- Orchestration.2  2 ICA Channel Opened for /ibc-port/icacontroller-0 at /ibc-hop/connection-0/ibc-port/icahost/ordered/{"version":"ics27-1","controllerConnectionId":"connection-0","hostConnectionId":"connection-1","address":"","encoding":"proto3","txType":"sdk_multi_msg"}
2024-03-27T20:29:22.992Z block-manager: block 48 commit

The No listeners for... error is benign - it's a handled error in network.js that should appear in the happy path.

The IBC fromBridge log confirms osmosis has acknowledged our request for a remote account. The osmo1d6em9ea5y3dye6em0awqyss7ssp0a7sgjk792x8cx647cfs7a4msk0fr45 address is assigned.

# failed send packet, since we did not send packetBytes
2024-03-27T20:30:23.209Z block-manager: block 60 commit
Dragonberry Active
Dragonberry Active
2024/03/27 16:30:25 error while extracting context for action &{%!q(*vm.ActionHeader=&{IBC_EVENT 60 1711571418}) "acknowledgementPacket" "" {'\x01' "icacontroller-0" "channel-0" "icahost" "channel-1" "test" "0-0" '�'} "{\"error\":\"ABCI code: 2: error handling packet: see events for details\"}" 42EC87A3463FC18F46B75C419541B61A7880B42A}
2024-03-27T20:30:28.234Z block-manager: block 61 begin
Dragonberry Active
2024-03-27T20:30:28.241Z SwingSet: vat: v16: IBC fromBridge { acknowledgement: 'eyJlcnJvciI6IkFCQ0kgY29kZTogMjogZXJyb3IgaGFuZGxpbmcgcGFja2V0OiBzZWUgZXZlbnRzIGZvciBkZXRhaWxzIn0=', blockHeight: 61, blockTime: 1711571423, event: 'acknowledgementPacket', packet: { data: 'dGVzdA==', destination_channel: 'channel-1', destination_port: 'icahost', sequence: 1, source_channel: 'channel-0', source_port: 'icacontroller-0', timeout_height: {}, timeout_timestamp: 1711575013110980000 }, relayer: 'agoric1gtkg0g6x8lqc734ht3qe2sdkrfugpdp2h7fuu0', type: 'IBC_EVENT' }
2024-03-27T20:30:28.275Z block-manager: block 61 commit

Host Node Validation

```bash # query osmosis node, observe STATE_OPEN channel and account address osmo1d6em9ea5y3dye6em0awqyss7ssp0a7sgjk792x8cx647cfs7a4msk0fr45

agd query ibc channel channels --node http://localhost:26655
channels:

channel_id: channel-1
connection_hops:
- connection-1
  counterparty:
  channel_id: channel-0
  port_id: icacontroller-0
  ordering: ORDER_ORDERED
  port_id: icahost
  state: STATE_OPEN
  version: '{"version":"ics27-1","controller_connection_id":"connection-0","host_connection_id":"connection-1","address":"osmo1d6em9ea5y3dye6em0awqyss7ssp0a7sgjk792x8cx647cfs7a4msk0fr45","encoding":"proto3","tx_type":"sdk_multi_msg"}'
channel_id: channel-0
connection_hops:
- connection-0
  counterparty:
  channel_id: channel-0
  port_id: transfer
  ordering: ORDER_UNORDERED
  port_id: transfer
  state: STATE_OPEN
  version: ics20-1
  height:
  revision_height: "831"
  revision_number: "0"
  pagination:
  next_key: null
  total: "0"

0xpatrickdev · 2024-03-27T21:09:10Z

packages/vats/src/orchestration.js

+              .bind(`/ibc-port/icacontroller-${this.state.icaControllerNonce}`)
+              .catch(e => Fail`Failed to bind port: ${bare(e)}`);
+
+            this.state.icaControllerNonce += 1;


We probably want something more robust than this. If the same port identifier (and connection id) is reused, it will result in the same address on the host chain. Maybe this should be networkVat's responsibility, unless vat-orchestration is the only one handing out icacontroller-* ports?

The network vat used to have some makeNonceMaker() code that seems useful for this sort of thing. I/we ripped it out because keeping it would involve yet another exo class. But maybe this justifies having it after all. Should be called something more like registerPrefix or reservePrefix though.

Opened a ticket to track work: #9165

packages/vats/src/orchestration.js

dckc · 2024-03-28T03:06:54Z

packages/boot/test/bootstrapTests/test-vat-orchestration.ts

+    const bundle = await bundleCache.load(path, name);
+    const installationRef = await EV(zoe).install(bundle);


we try to avoid E(zoe).install(bundle) in anything but toys. This seems to be a pretty high-fidelity test, so I suggest using controller.validateAndInstallBundle(bundle) and E(zoe).installBundleID(bundleID, label).

oops... I see we didn't do this consistently in test-net-ibc-upgrade.ts. oh well.

Thank you, TIL

cf

drop zoe.install(bundle) #8083

Remove Zoe support for full-source bundles in favor of hash bundles #4565

dckc · 2024-03-28T03:08:48Z

packages/boot/test/bootstrapTests/test-vat-orchestration.ts

+  const orchestration = await EV.vat('bootstrap').consumeItem('orchestration');
+  // XXX this should not throw
+  await t.notThrowsAsync(async () => {
+    EV(orchestration).provideAccount(serverLocalAddr, clientLocalAddr);


since it's called provideX, let's test that has find-or-create-X behavior; i.e. that calling it again with the same args gives the same result rather than making a fresh one.

Updated to createAccount, no longer using provide

dckc · 2024-03-28T03:14:36Z

packages/orchestration/src/proposals/start-stakeAtom.js

+          board: true,
+          chainStorage: true,


board and chainStorage don't seem to be used

dckc · 2024-03-28T03:14:48Z

packages/orchestration/src/proposals/start-stakeAtom.js

+        produce: {
+          stakeAtom: true,
+        },


nor produce.stakeAtom

dckc · 2024-03-28T03:16:58Z

packages/vats/src/orchestration.js

+ * @param {PowerStore} powers
+ * @param {K} name
+ */
+const getPower = (powers, name) => {


why getPower? why not getNetwork()?

dckc · 2024-03-28T03:18:22Z

packages/vats/src/orchestration.js

+        Fail`getPurse not implemented yet`;
+      },
+      async close() {
+        // XXX retrieve all assets first?


perhaps assert that they're all gone? or take some dust threshold parameter?

dckc · 2024-03-28T03:19:10Z

packages/vats/src/orchestration.js

+      },
+      async close() {
+        // XXX retrieve all assets first?
+        // XXX do we also revoke the port?


no; the caller supplied it; presumably they own it

dckc · 2024-03-28T03:21:15Z

packages/vats/src/orchestration.js

+     * ) => PromiseVow<string>} [onReceive]
+     *   optional cb handler for connection recieving packet
+     */
+    (onOpen, onClose, onReceive) => ({ onOpen, onClose, onReceive }),


storing functions in durable state? does that work? I'd be surprised.

ty, addressed

dckc · 2024-03-28T03:23:52Z

packages/vats/src/orchestration.js

+    /** @param {Partial<OrchestrationPowers>} [initialPowers] */
+    initialPowers => {
+      /** @type {PowerStore} */
+      const powers = zone.detached().mapStore('PowerStore');


why treat this as a collection? the keys are statically known. seems like using them as individual properties is more straightforward.

oh... maybe because state properties can't be added on upgrade?
please add a comment to that effect, if that's the reason.

There is now a comment with more details and an issue ref

packages/vats/src/orchestration.js

dckc · 2024-03-28T03:38:30Z

packages/vats/src/orchestration.js

+         * @param {Port} [currentPort] if a port is provided, it will be reused
+         *   to create the account
+         */
+        async provideAccount(


The provide prefix means find-or-create; i.e. if it's already been created, return the existing one. This code makes a fresh account on every call.

Suggested change

async provideAccount(

async makeAccount(

dckc · 2024-03-28T03:45:56Z

packages/vats/src/proposals/orchestration-proposal.js

+
+    [addOrchestrationToClient.name]: {
+      consume: {
+        client: 'provisioning',


client is an ag-solo thing; is this code intended for production?

Thanks for calling this out and making me think about it more. I suppose we should have two proposals - one for ag-solo / devnet and another for mainnet. Removed ag-solo for now, but may add a commit so it's available on home .

dckc · 2024-04-02T17:36:46Z

packages/orchestration/src/types.d.ts

+export type ConnectionId = `connection-${number}`;
+
+export type AttenuatedNetwork = {
+  bind: import('@agoric/network/src/router').RouterProtocol['bind'];


Suggested change

export type ConnectionId = `connection-${number}`;

export type AttenuatedNetwork = {

bind: import('@agoric/network/src/router').RouterProtocol['bind'];

import type { RouterProtocol } from '@agoric/network/src/router';

export type ConnectionId = `connection-${number}`;

export type AttenuatedNetwork = {

bind: RouterProtocol['bind'];

dckc · 2024-04-02T17:38:27Z

packages/orchestration/test/utils/address.test.js

+  t.is(
+    makeICAConnectionAddress('connection-0', 'connection-1'),
+    '/ibc-hop/connection-0/ibc-port/icahost/ordered/{"version":"ics27-1","controllerConnectionId":"connection-0","hostConnectionId":"connection-1","address":"","encoding":"proto3","txType":"sdk_multi_msg"}',
+    'returns connection string when controllerConnectionId and hostConnectionId are provided',


👏 yay for explicit test assertions

0xpatrickdev · 2024-04-02T21:28:35Z

packages/boot/tools/ibc/mocks.js

+  },
+  sendPacket: {
+    // ICA Send Packet (Transaction)
+    acknowledgementPacket: _obj => ({


This step is not reached in tests yet, only channelOpenAck. channelOpenAck

thanks for the note. when do you plan to cover the case? that would be good to document in the code.

Added a comment: not yet tested. will be used in https://github.com/Agoric/agoric-sdk/issues/8881

Original motivation for my comment - realized this wasn't used when I posted the PR. Think it's OK to leave as it corresponds with the diagram being checked in in the README

turadg · 2024-04-02T23:09:51Z

packages/boot/test/bootstrapTests/test-orchestration.ts

@@ -86,3 +88,37 @@ test('stakeBld', async t => {
    },
  });
 });
+
+test('stakeAtom', async t => {


this is a good start. can you get it to interact with the smart wallet using mocked bridge responses?

happy to pair on that

turadg · 2024-04-02T23:14:37Z

packages/boot/test/bootstrapTests/test-vat-orchestration.ts

+  t.truthy(remoteAddress.includes('icahost'), 'remoteAddress is returned');
+  t.truthy(localAddress.includes('icacontroller'), 'localAddress is returned');
+  t.truthy(accountAddress.includes('osmo1'), 'accountAddress is returned');


Suggested change

t.truthy(remoteAddress.includes('icahost'), 'remoteAddress is returned');

t.truthy(localAddress.includes('icacontroller'), 'localAddress is returned');

t.truthy(accountAddress.includes('osmo1'), 'accountAddress is returned');

t.regex(remoteAddress, /icahost/);

t.regex(localAddress, /icacontroller/);

t.regex(accountAddress, /osmo1/);

('m indifferent to the explicit messages)

turadg · 2024-04-02T23:15:15Z

packages/boot/tools/ibc/ics27-1.md

+
+# ICS-27 Interchain Accounts
+
+Sequence diagrams for Interchain Accounts based on [ics-027-interchain-accounts/README.md#9ffb1d2](https://github.com/cosmos/ibc/blob/9ffb1d26d3018b6efda546189ec7e43d56d23da3/spec/app/ics-027-interchain-accounts/README.md).


👏

consider making the file README.md so it will appear when someone navigates to the directory path

turadg · 2024-04-02T23:17:30Z

packages/boot/tools/ibc/mocks.js

+  },
+  sendPacket: {
+    // ICA Send Packet (Transaction)
+    acknowledgementPacket: _obj => ({


thanks for the note. when do you plan to cover the case? that would be good to document in the code.

packages/builders/scripts/vats/init-orchestration.js

turadg · 2024-04-02T23:44:09Z

packages/vats/src/orchestration.js

+ */
+
+/**
+ * PowerStore is used so additional powers can be added on upgrade.


please reference the ticket about Exo state migrations because the lack of that is what's motivating this.

the best issue I can find is #7337

turadg · 2024-04-02T23:47:07Z

packages/vats/src/orchestration.js

+        ) {
+          await null;
+
+          let port;


port never changes and you're figuring out what to initialize it with. let's do the work to make it a const.

turadg · 2024-04-02T23:47:38Z

packages/vats/src/orchestration.js

+
+          let port;
+          if (!matches(currentPort, M.remotable('Port'))) {
+            if (currentPort) trace('Invalid Port provided, binding a new one.');


if they passed a non-port for currentPort, shouldn't that be an error?

I think you want the check to be whether currentPort === undefined, which indicates whether the caller passed an argument. then validate it's actually a port and error if it isn't.

Correct, agree this should be an error.

Aside - it seems I was mistaken in some of my understanding here. This test is failing on the second createAccount call:

const orchestration = await EV.vat('bootstrap').consumeItem('orchestration'); const account1 = await EV(orchestration).createAccount( 'connection-0', 'connection-0', ); t.truthy(account1, 'createAccount returns an account'); const port = await EV(account1).getPort(); t.truthy(port, 'getPort returns a port'); const port1Addr = await EV(port).getLocalAddress(); t.truthy(port1Addr); const account2 = await EV(orchestration).createAccount( 'connection-1', 'connection-1', port, ); // fails - `key channel-0:icacontroller-0 already registered in collection channelKeyToSeqAck`: t.truthy(account2, 'createAccount returns an account with port provided'); const port2 = await EV(account2).getPort(); const port2Addr = await EV(port2).getLocalAddress(); t.is(port1Addr, port2Addr, 'port address is the same');

Removing this logic for now. Port reuse is in scope for #9068

turadg · 2024-04-02T23:49:51Z

packages/vats/src/orchestration.js

+          return chainAccount.account;
+        },
+        /** @param {string} _chainName e.g. cosmos */
+        getChain(_chainName) {


I suspect this is at the wrong level of abstraction. I expect we'll have a utility in front of this vat which manages chain config.

Acknowledged. I think the idea is to be able to do:

const cosmos = await E(orchestration).getChain('cosmos'); const account = await E(cosmos).createAccount();

And getChain might look something like:

getChain(chainName) { return E(agoricNames || someOtherNamehub).lookup('chain', chainName); }

Nonetheless, out of scope for this PR (in scope for #9063) and was intended just to be a placeholder based on this proposed spec. Happy to remove from this PR if you'd prefer

yes, please remove the placeholders. ideally everything included has tests

turadg · 2024-04-02T23:51:46Z

packages/vats/src/orchestration.js

+/** @typedef {OrchestrationKit['admin']} OrchestrationAdmin */
+/** @typedef {OrchestrationKit['public']} Orchestration */


this name too general to bind to the public facet, imo. sites that need it could easily pull it off OrchestrationKit. I suppose the same for OrchestrationAdmin

I removed admin and OrchestrationAdmin in 2a39e6d.

I am wary of only exported OrchestrationKit - consumers of the API will likely only use the public facet, so the goal was to export a type that can be used to represent the orchestration available in the bootstrap space for a core proposal (e.g. stakeAtom). Would you prefer to see it called OrchestrationPF?

dckc · 2024-04-03T22:12:49Z

packages/boot/test/bootstrapTests/test-vat-orchestration.ts

+  t.truthy(account, 'createAccount returns an account');
+
+  const res = await EV(account).close();
+  t.is(res, 'Connection closed');


how about trying to use it after it's closed and checking that it throws?

dckc · 2024-04-03T22:14:53Z

packages/boot/test/bootstrapTests/test-vat-orchestration.ts

@@ -70,3 +70,20 @@ test('createAccount returns an ICA connection', async t => {
    accountAddress,
  });
 });
+
+test('ICA connection can be closed', async t => {


Is it important that closing a connection frees up resources? If so, is it feasible to check that?

I can imagine it might not be feasible... that any relevant collections are weak / non-enumerable.

These are good questions. I created #9192 to track

turadg

Solid progress! Let's land this

turadg · 2024-04-03T22:13:34Z

packages/orchestration/src/orchestration.js

@@ -156,9 +137,10 @@ const prepareChainAccount = zone =>
          // XXX parseAddress currently throws, should it return '' instead?
          this.state.accountAddress = parseAddress(remoteAddr);
        },
-        async onClose(_connection, reason) {
+        async onClose(_connection, reason, _connectionHandler) {


why add the unused param? I went the opposite direction in 44107b9

I did this in the course of fixing a broken interface guard. The unused param is not needed to satisfy the interface guard, but it felt weird making the change without including this.

#9191 looks awesome btw. There, or a separate PR would be great to export ConnectionHandler types and an interface guard.

turadg · 2024-04-03T22:22:12Z

packages/orchestration/src/utils/address.js

+    encoding, //
+    txType: 'sdk_multi_msg',
+  });
+  return `/ibc-hop/${controllerConnectionId}/ibc-port/icahost/${ordering}/${connString}`;


this suggests the https://github.com/Agoric/agoric-sdk/pull/9191/files#diff-20e9eca9ab68ad47bb9d4caa091036228bdd2ac215652dffebbdc1d27fbc80abR66 should be more flexible. I think this PR should land first tho

@iomekam

refs: #9042 ## Description In the course of reviewing #9114 it wasn't clear what the `/ibc-hop` encoding spec was. This PR separates an `ibc-utils.js` to try to encapsulate that. @iomekam @michaelfig can you provide some additional documentation on its design? This also adds types I used to understand the code, dropping `any` bindings from 338 to 86. One type change was in `vows` to add the context param. ### Security Considerations  ### Scaling Considerations  ### Documentation Considerations  ### Testing Considerations  ### Upgrade Considerations

refs: #9114 closes: #9317 ## Description - Miscellaneous cleanup related to `CosmosInterchainService` (part of `vat-orchestration`), closing out todos for 1) unnecessary PowerStore abstraction and 2) vow compat for early synchronous returns - Optimizes ICQ Port Allocation by reusing a single port for all ICQ connections ### Security Considerations n/a ### Scaling Considerations n/a ### Documentation Considerations js doc added where necessary ### Testing Considerations included tests for new behavior ### Upgrade Considerations these changes are intended to better prepare us for future upgrades

0xpatrickdev force-pushed the 9042-stake-atom branch from b1184b5 to 9aaad9e Compare March 27, 2024 19:56

0xpatrickdev commented Mar 27, 2024

View reviewed changes

packages/vats/src/orchestration.js Outdated Show resolved Hide resolved

0xpatrickdev commented Mar 27, 2024

View reviewed changes

packages/vats/src/orchestration.js Outdated Show resolved Hide resolved

dckc reviewed Mar 28, 2024

View reviewed changes

packages/vats/src/orchestration.js Outdated Show resolved Hide resolved

dckc reviewed Mar 28, 2024

View reviewed changes

dckc reviewed Apr 2, 2024

View reviewed changes

0xpatrickdev force-pushed the 9042-stake-atom branch from 51bbee4 to 1b6090b Compare April 2, 2024 21:07

0xpatrickdev marked this pull request as ready for review April 2, 2024 21:15

0xpatrickdev requested a review from turadg April 2, 2024 21:15

0xpatrickdev changed the title ~~feat: stake atom~~ feat: create chain account (ica) Apr 2, 2024

0xpatrickdev commented Apr 2, 2024

View reviewed changes

turadg reviewed Apr 2, 2024

View reviewed changes

turadg mentioned this pull request Apr 3, 2024

IBC types and refactoring #9191

Merged

dckc reviewed Apr 3, 2024

View reviewed changes

turadg approved these changes Apr 3, 2024

View reviewed changes

test(ibc): add bootstrap fixtures for ics27-1 mocking

477cbaf

0xpatrickdev added 2 commits April 4, 2024 10:06

feat(orchestration): add makeICAConnectionAddress and parseAddress utils

39b73d5

feat(orchestration): create ChainAccount

ba75ed6

0xpatrickdev force-pushed the 9042-stake-atom branch from 5427003 to c6de9a0 Compare April 4, 2024 14:12

feat(orchestration): add stakeAtom example contract

82f1901

0xpatrickdev force-pushed the 9042-stake-atom branch from c6de9a0 to 82f1901 Compare April 4, 2024 14:56

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Apr 4, 2024

0xpatrickdev mentioned this pull request Apr 4, 2024

orchestration - ICA Controller - Account - Close Connection Lifecycle #9192

Closed

mergify bot merged commit 21fe8d9 into master Apr 4, 2024
74 checks passed

mergify bot deleted the 9042-stake-atom branch April 4, 2024 15:59

0xpatrickdev mentioned this pull request Jul 30, 2024

chore: cosmos interchain service cleanup #9810

Merged

		const bundle = await bundleCache.load(path, name);
		const installationRef = await EV(zoe).install(bundle);


		# ICS-27 Interchain Accounts

		Sequence diagrams for Interchain Accounts based on [ics-027-interchain-accounts/README.md#9ffb1d2](https://github.com/cosmos/ibc/blob/9ffb1d26d3018b6efda546189ec7e43d56d23da3/spec/app/ics-027-interchain-accounts/README.md).

		/** @typedef {OrchestrationKit['admin']} OrchestrationAdmin */
		/** @typedef {OrchestrationKit['public']} Orchestration */

feat: create chain account (ica) #9114

feat: create chain account (ica) #9114

Conversation

0xpatrickdev commented Mar 20, 2024 • edited Loading

Description

Outstanding Items

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

0xpatrickdev commented Mar 20, 2024 • edited Loading

0xpatrickdev Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

turadg Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev commented Mar 20, 2024 •

edited

Loading

0xpatrickdev commented Mar 20, 2024 •

edited

Loading

0xpatrickdev Mar 27, 2024 •

edited

Loading

0xpatrickdev Mar 28, 2024 •

edited

Loading

0xpatrickdev Apr 3, 2024 •

edited

Loading

0xpatrickdev Apr 3, 2024 •

edited

Loading

turadg Apr 3, 2024 •

edited

Loading