feat(orchestration): send message #9195

0xpatrickdev · 2024-04-04T21:41:16Z

closes: #8881
refs: #9042

Description

Implement .executeEncodedTx on ChainAccount
- accepts array of {typeUrl: string, value: Base64Bytes}. The Base64Bytes value keys are converted to Uint8Array, and the array of messages are then encoded inside TxBody.
- the .send() is a JSON string in the event of success or failure. If a result key is present it is returned. If an error key is present, or neither key is found, an error is thrown.
Implement delegate for StakeAtom
- in addition to creating an accounts, users can now delegate. They can use invitations or call .delegate() on their Account Holder
- depositing a payment (and sending to the new ICA via IBC) will be tackled as part of orchestration - ICA Controller - deposit payment to ICA #9193
Improved ibc mocks and bridge i/o in bootstrap style testing. Currently, delegating 10uatom from cosmos1test to cosmosvaloper1test should result in a successful TX. Others, like delegating to cosmosvaloper1fail will fail.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

packages/boot/tools/supports.ts

packages/orchestration/src/contracts/stakingAccountHolder.js

packages/cosmic-proto/package.json

michaelfig · 2024-04-05T18:59:37Z

packages/boot/tools/ibc/mocks.js

+     * @param {ChannelOpenInitPacket} obj
+     */
+    channelOpenAck: obj => {
+      const icaControllerNonce = obj?.packet?.source_port?.split('-')?.[1];


This seems suspicious, since channelId's are frequently different between the subject chain and the counterparty, and also have no relation to the port number. Could you maybe separate and document what these numbers actually are:

Suggested change

const icaControllerNonce = obj?.packet?.source_port?.split('-')?.[1];

const sourcePortSuffix = obj?.packet?.source_port?.split('-')?.at(-1);

const destPortSuffix = obj?.packet?.destination_port?.split('-')?.at(-1);

Agree, I will add your suggestions so this mocking is documented clearly.

michaelfig

I'm not sure I have enough context to be able to officially approve, but AFAIK you can merge directly into 9111-proto-messages without my approval.

michaelfig · 2024-04-05T19:01:19Z

packages/boot/tools/ibc/mocks.js

+        channelID: `channel-${icaControllerNonce}`,
+        counterparty: {
+          port_id: obj.packet.destination_port,
+          channel_id: `channel-${icaControllerNonce}`,


Suggested change

channelID: `channel-${icaControllerNonce}`,

counterparty: {

port_id: obj.packet.destination_port,

channel_id: `channel-${icaControllerNonce}`,

// Fake a channel ID from its port suffix.

channelID: `channel-${sourcePortSuffix}`,

counterparty: {

port_id: obj.packet.destination_port,

// Fake a channelID from its port suffix.

channel_id: `channel-${destPortSuffix}`,

michaelfig · 2024-04-05T19:21:02Z

packages/orchestration/src/utils/tx.js

+/** @import { Proto3Msg } from '../types'; */
+
+/** @typedef {string} Base64Bytes - Uint8Array, base64 encoded */
+/** @typedef {{ typeUrl: string; value: Base64Bytes; }} Proto3Msg */


Why the two different definitions of Proto3Msg?

Not intentional, thank you

turadg · 2024-04-05T20:39:37Z

I'm not sure I have enough context to be able to officially approve, but AFAIK you can merge directly into 9111-proto-messages without my approval.

@0xpatrickdev please don't. I'd like to land that branch, #9112, today. It's green and just needs approval.

0xpatrickdev · 2024-04-05T20:56:59Z

I'm not sure I have enough context to be able to officially approve, but AFAIK you can merge directly into 9111-proto-messages without my approval.

@0xpatrickdev please don't. I'd like to land that branch, #9112, today. It's green and just needs approval.

Planning to go to master. Based against 9111-proto-messages as this PR is dependent on it.

turadg · 2024-04-05T21:42:25Z

packages/boot/tools/ibc/mocks.js

+
+/**
+ * @param {string} versionString version string
+ * @param {{ address: string; }} [params]


params is optional but what's the point of calling this without it?

turadg · 2024-04-05T21:42:46Z

packages/boot/tools/ibc/mocks.js

+ */
+
+/**
+ * @param {string} versionString version string


please convey that this string itself must be in JSON

It's an arbitrary string. ICA happens to use a JSON format, but IBC transfer uses a non-JSON string of just seven bytes: ics20-1.

I'm increasingly hesitant to impose more structure than the IBC spec describes (more than just ICA or the "IBC transfer" protocol). There's a sweet spot to be found between making APIs with useful guard rails versus types that are flexible enough to express legitimate uses of IBC we haven't yet imagined.

impose more structure than the IBC spec describes

💯 we should stick to the spec. Are there types for the spec that we could import? If not, I propose we have a separate file with just the types from the spec that code implementing parts of it imports.

But for parts of the code that do not support the full spec, shouldn't we document what they are expecting?

turadg · 2024-04-05T21:43:38Z

packages/boot/tools/ibc/mocks.js

+     */
+    msgDelegateResponse: (obj, sequence = 1) => {
+      return {
+        // {"result":"+/cosmos.staking.v1beta1.MsgDelegateResponse"}


what's this comment meant to convey?

These code comments are the decoded acknowledgement packets. I refactored mocks.js and support.ts per your feedback, lmk if this reads better:

https://github.com/Agoric/agoric-sdk/pull/9195/files#diff-6c46d78f5e5132e1c8596f11536961851d788b0ea13d1d60b47f87faf3a6e436R29-R53

turadg · 2024-04-05T21:44:02Z

packages/boot/tools/ibc/mocks.js

+     */
+    acknowledgementPacketFailure: (obj, sequence = 1) => {
+      return {
+        // {"error":"ABCI code: 5: error handling packet: see events for details"} (ErrPacketTimeout?)


ditto, why the commented out code?

turadg · 2024-04-05T21:44:50Z

packages/boot/tools/supports.ts

@@ -367,6 +368,50 @@ export const makeSwingsetTestKit = async (
                  icaMocks.startChannelOpenInit.channelOpenAck(obj),
                );
                return undefined;
+              case 'sendPacket':
+                switch (obj.packet.data) {
+                  case 'eyJ0eXBlIjoxLCJkYXRhIjoiQ2xVS0l5OWpiM050YjNNdWMzUmhhMmx1Wnk1Mk1XSmxkR0V4TGsxelowUmxiR1ZuWVhSbEVpNEtDMk52YzIxdmN6RjBaWE4wRWhKamIzTnRiM04yWVd4dmNHVnlNWFJsYzNRYUN3b0ZkV0YwYjIwU0FqRXciLCJtZW1vIjoiIn0=':


this is pretty specific to particular tests. please make these exported constants so it's easy to see where they're used

#9195 (comment)

packages/orchestration/src/orchestration.js

turadg · 2024-04-05T21:47:35Z

packages/orchestration/src/types.d.ts

+export type DestinationPort = 'icahost' | 'icqhost';
+export type SourcePortPrefix = 'icacontroller' | 'icqcontroller';
+export type SourcePort = `${SourcePortPrefix}-${number}`;
+// TODO improve. sometimes expressed uppercase, others lowercase


+1, I don't get it

I removed this type (ChannelOrdering) in favor of the now exported @typedef {'ORDERED' | 'UNORDERED'} IBCChannelOrdering from @agoric/vats. It seems like the lowercase variant is used when creating an address string, and the uppercase variant in packets. There is no type checking against address strings in this PR, but we could maybe add something like type AddressChannelOrdering = Lowercase<IBCChannelOrdering> if we wanted to do that

It seems like the lowercase variant is used when creating an address string, and the uppercase variant in packets.

Agreed, but I don't understand why

packages/cosmic-proto/test/test-endo.js

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

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

turadg · 2024-04-08T20:30:19Z

packages/orchestration/src/orchestration.js

+          return E.when(
+            E(connection).send(makeTxPacket(msgs, opts)),
+            // if parsePacketAck cannot find a `result` key, it throws
+            ack => parsePacketAck(ack),


note that that throw will not be caught by the catch below

Correct. This was intended to capture a failure on the initial send. Since we are doing extra handling, I've removed the catch

turadg · 2024-04-08T20:31:33Z

packages/orchestration/src/orchestration.js

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


out of scope but it strikes me that .bind( is 99% of the time the JS function bind to this.

@iomekam WDYT of a name that's less likely to be confused?

Maybe bindPort?

agoric-sdk/golang/cosmos/x/vibc/types/receiver.go

Line 146 in 80e450a

case "bindPort":

I can see how E(port).bindPort() might sound odd, though.

redundant > confusing ;-)

bindPort is a perfect name. It's not E(port).bindPort(), it's E(network).bindPort().

packages/orchestration/src/orchestration.js

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

packages/vats/src/ibc.js

packages/boot/tools/ibc/mocks.js

turadg · 2024-04-09T15:37:36Z

packages/orchestration/src/orchestration.js

@@ -51,17 +49,17 @@ const getPower = (powers, name) => {
  return /** @type {OrchestrationPowers[K]} */ (powers.get(name));
 };

+export const Proto3Shape = {
+  typeUrl: M.string(),
+  value: M.string(),


I still think we should have a different key for when value is bytes vs base64 string. e.g. valueB64. not a blocker

turadg · 2024-04-09T15:40:45Z

packages/vats/src/types.d.ts

@@ -106,4 +106,139 @@ export type BridgeManager = {
  ) => ScopedBridgeManager;
 };

-export type * from './ibc.js';
+export type IBCPortID = string;


why did you move these from ibc.js? I suppose to get .ts syntax. I expect there's a better place for these but I can't think of a good suggestion atm

Yes for TS syntax, plus this ballooned to quite a few lines at the top of the file.

I attempted ibc.d.ts in the same directory but this caused issues with things looking for exports from ibc.js. I also did not observe a pattern of multiple .d.ts files in a single package in the repo

…ncallPacket

turadg

🚀

turadg · 2024-04-09T17:57:22Z

packages/boot/test/tools/ibc/test-mocks.js

+import { test } from '@agoric/zoe/tools/prepare-test-env-ava.js';
+import { addParamsIfJsonVersion } from '../../../tools/ibc/mocks.js';
+
+test('addParamsToVersion', t => {


nit: addParamsIfJsonVersion, not worth CI

turadg · 2024-04-09T17:57:53Z

packages/boot/test/tools/ibc/test-mocks.js

+  ];
+
+  for (const { version, expected, message } of scenarios) {
+    t.is(addParamsIfJsonVersion(version, params), expected, message);


in the future consider an Ava macro for multiple test cases

0xpatrickdev commented Apr 4, 2024

View reviewed changes

packages/boot/tools/supports.ts Outdated Show resolved Hide resolved

turadg force-pushed the 9111-proto-messages branch from 2f54b74 to d717eb4 Compare April 4, 2024 22:39

0xpatrickdev force-pushed the 8881-ica-send-message branch from aa39e09 to 4982dda Compare April 4, 2024 22:56

turadg force-pushed the 9111-proto-messages branch from d717eb4 to 7b7e823 Compare April 4, 2024 23:28

0xpatrickdev force-pushed the 8881-ica-send-message branch from 4982dda to d0f7120 Compare April 5, 2024 03:53

0xpatrickdev commented Apr 5, 2024

View reviewed changes

packages/orchestration/src/contracts/stakingAccountHolder.js Outdated Show resolved Hide resolved

turadg force-pushed the 9111-proto-messages branch 2 times, most recently from 32f2624 to 25790ee Compare April 5, 2024 16:05

0xpatrickdev force-pushed the 8881-ica-send-message branch from d0f7120 to e58050b Compare April 5, 2024 17:46

0xpatrickdev marked this pull request as ready for review April 5, 2024 17:53

0xpatrickdev requested review from turadg and michaelfig April 5, 2024 17:54

0xpatrickdev changed the title ~~feat(orchestration): executeEncodeTx~~ feat(orchestration): send message Apr 5, 2024

0xpatrickdev commented Apr 5, 2024

View reviewed changes

packages/cosmic-proto/package.json Outdated Show resolved Hide resolved

michaelfig reviewed Apr 5, 2024

View reviewed changes

turadg force-pushed the 9111-proto-messages branch from 25790ee to e315f6d Compare April 5, 2024 20:44

turadg reviewed Apr 5, 2024

View reviewed changes

0xpatrickdev force-pushed the 8881-ica-send-message branch from 03f697f to a1dc02a Compare April 6, 2024 00:35

0xpatrickdev commented Apr 6, 2024

View reviewed changes

packages/cosmic-proto/test/test-endo.js Outdated Show resolved Hide resolved

turadg force-pushed the 9111-proto-messages branch 3 times, most recently from 6482df5 to 722015f Compare April 8, 2024 14:29

Base automatically changed from 9111-proto-messages to master April 8, 2024 16:10

0xpatrickdev force-pushed the 8881-ica-send-message branch 2 times, most recently from c421d1b to 3ae138b Compare April 8, 2024 19:57

0xpatrickdev requested review from turadg and michaelfig April 8, 2024 20:25

turadg reviewed Apr 8, 2024

View reviewed changes

turadg mentioned this pull request Apr 8, 2024

9207 orchestrator stub #9208

Merged

turadg reviewed Apr 9, 2024

View reviewed changes

0xpatrickdev added 2 commits April 9, 2024 13:27

feat(ibc): add types for IBCEvent, IBCMethod, IBCDowncall, and IBCDow…

54fc93c

…ncallPacket

feat(orchestration): send message from ica

764e4a8

0xpatrickdev force-pushed the 8881-ica-send-message branch from 330fe8c to 38c7bc3 Compare April 9, 2024 17:35

feat(orchestration): stakeAtom delegate

54d830f

0xpatrickdev force-pushed the 8881-ica-send-message branch from 38c7bc3 to 54d830f Compare April 9, 2024 17:50

0xpatrickdev requested a review from turadg April 9, 2024 17:54

turadg approved these changes Apr 9, 2024

View reviewed changes

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

mergify bot merged commit e468caa into master Apr 9, 2024
74 checks passed

mergify bot deleted the 8881-ica-send-message branch April 9, 2024 18:46

0xpatrickdev mentioned this pull request Apr 9, 2024

refactor(network): rename bind to bindPort #9213

Merged

0xpatrickdev mentioned this pull request Apr 29, 2024

Orchestration - Create Generic Chain (Chain Object for non well known chain) #9187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(orchestration): send message #9195

feat(orchestration): send message #9195

0xpatrickdev commented Apr 4, 2024 •

edited

Loading

michaelfig Apr 5, 2024 •

edited

Loading

0xpatrickdev Apr 5, 2024

michaelfig left a comment

michaelfig Apr 5, 2024

michaelfig Apr 5, 2024

0xpatrickdev Apr 5, 2024

turadg commented Apr 5, 2024

0xpatrickdev commented Apr 5, 2024

turadg Apr 5, 2024

turadg Apr 5, 2024

michaelfig Apr 8, 2024

turadg Apr 8, 2024

turadg Apr 5, 2024

0xpatrickdev Apr 6, 2024 •

edited

Loading

turadg Apr 8, 2024

turadg Apr 5, 2024

turadg Apr 5, 2024

0xpatrickdev Apr 6, 2024

turadg Apr 5, 2024

0xpatrickdev Apr 8, 2024 •

edited

Loading

turadg Apr 8, 2024

turadg Apr 8, 2024

0xpatrickdev Apr 9, 2024

turadg Apr 8, 2024

0xpatrickdev Apr 9, 2024 •

edited

Loading

turadg Apr 9, 2024

michaelfig Apr 9, 2024

turadg Apr 9, 2024

turadg Apr 9, 2024

0xpatrickdev Apr 9, 2024

turadg left a comment

turadg Apr 9, 2024

turadg Apr 9, 2024

	const icaControllerNonce = obj?.packet?.source_port?.split('-')?.[1];
	const sourcePortSuffix = obj?.packet?.source_port?.split('-')?.at(-1);
	const destPortSuffix = obj?.packet?.destination_port?.split('-')?.at(-1);

feat(orchestration): send message #9195

feat(orchestration): send message #9195

Conversation

0xpatrickdev commented Apr 4, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

michaelfig Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig 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

turadg commented Apr 5, 2024

0xpatrickdev commented Apr 5, 2024

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 6, 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

0xpatrickdev Apr 8, 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

0xpatrickdev Apr 9, 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

0xpatrickdev commented Apr 4, 2024 •

edited

Loading

michaelfig Apr 5, 2024 •

edited

Loading

0xpatrickdev Apr 6, 2024 •

edited

Loading

0xpatrickdev Apr 8, 2024 •

edited

Loading

0xpatrickdev Apr 9, 2024 •

edited

Loading