test(orchestration): ibc mocking utils #9628

0xpatrickdev · 2024-07-01T14:19:58Z

closes: #9572

Description

Provides testing utilities for simulating IBC bridge events and packets in @agoric/orchestration .

packages/orchestration/test/network-fakes.test.ts

packages/orchestration/test/network-fakes.ts

turadg

This will greatly help our package testing coverage. I wonder about how we can DRY with RunUtils tests à la #8963. I also wonder about how to improve or at least maintain fidelity of these mocks.

turadg · 2024-07-01T14:39:54Z

packages/cosmic-proto/src/helpers.ts

@@ -103,7 +103,7 @@ export const typeUrlToGrpcPath = (typeUrl: Any['typeUrl']) => {
 type RequestQueryOpts = Partial<Omit<RequestQuery, 'path' | 'data'>>;

 export const toRequestQueryJson = (
-  msg: QueryBalanceRequestProtoMsg,
+  msg: { value: Uint8Array; typeUrl: string },


The type described here is Any

agoric-sdk/packages/cosmic-proto/src/codegen/google/protobuf/any.ts

Lines 117 to 120 in 4c51569

typeUrl: string;

/** Must be a valid serialized protocol buffer of the above specified type. */

value: Uint8Array;

}

Suggested change

msg: { value: Uint8Array; typeUrl: string },

msg: Any,

Please maintain use of aliases as documentation of what these types are, that they aren't ad-hoc.

Though I'd encourage keeping QueryBalanceRequestProtoMsg since that's the only query we yet support. We can add a union for other messages. I think you only had to make this Any because buildQueryPacketString didn't constrain its param, and I think it would be better to instead use QueryBalanceRequestProtoMsg in both places as a form of documentation. It doesn't affect future compatibility because it doesn't affect runtime.

If as we add more request types the union of them becomes unwieldy we could define a message type using RequestTypeUrl

It you don't use QueryBalanceRequestProtoMsg remember to remove its import.

The Any suggestion is great, thank you. I intended to leverage existing types as much as possible but missed this one. I'm less convinced we need to include QueryBalanceRequestProtoMsg, but I've included it in the union anyway.

In somewhat related code I chose to type out Encoder. If there's something we can instead import from telescope, that'd be great. We needed something that accepted generic params so we can supply any *Msg and get type inference for the second argument (Partial<T>) of buildMsgResponseString and buildQueryResponseString.

turadg · 2024-07-01T14:41:56Z

packages/orchestration/src/utils/packet.js

@@ -108,7 +108,7 @@ export function parseQueryPacket(response) {
  return harden(
    responses.map(resp => ({
      ...resp,
-      height: String(resp.index),
+      height: String(resp.height),


incidentally, I'm surprised there isn't a function already to do this encoding.

Good callout - see 1962218

turadg · 2024-07-01T14:43:28Z

packages/orchestration/test/examples/stake-ica.contract.test.ts

+} from '@agoric/cosmic-proto/cosmos/bank/v1beta1/query.js';
+import { commonSetup } from '../supports.js';
+import { type StakeIcaTerms } from '../../src/examples/stakeIca.contract.js';
+import chainInfo from '../../src/fetched-chain-info.js';


please name fetchedChainInfo to match its module and make more clear the source when reading within getChainTerms

Rename done and included fetchChainInfo as an optional second argument typed as Record<string, CosmosChainInfo> - ChainInfo was giving a hard time and this is a cosmos-specific contract

turadg · 2024-07-01T14:45:55Z

packages/orchestration/test/examples/stake-ica.contract.test.ts

+    await E(ibcBridge).setMockAck({
+      [buildQueryPacketString([
+        QueryBalanceRequest.toProtoMsg({
+          address: chainAddress.address,
+          denom: 'uosmo',
+        }),
+      ])]: buildQueryResponseString(QueryBalanceResponse, {
+        balance: { amount: '0', denom: 'uosmo' },
+      }),
+    });


this is hard to parse visually. consider pulling out the key and maybe the value into named bindings

I've made an edit to enhance readability, but don't expect this to be the final abstraction. We might limit verbosity with something like:

mocks.delegate((reqPartial, respPartial, [error]) => [ReqString, RespString])

that can be massaged into a record.

We also might want to do do these instantiations in test.before(), so the actual test code isn't cluttered.

turadg · 2024-07-01T14:48:34Z

packages/orchestration/test/ibc-mocks.test.ts

+  t.is(
+    buildMsgErrorString(),
+    'eyJlcnJvciI6IkFCQ0kgY29kZTogNTogZXJyb3IgaGFuZGxpbmcgcGFja2V0OiBzZWUgZXZlbnRzIGZvciBkZXRhaWxzIn0=',
+    'ABCI code: 5: error handling packet',


what's this test message meant to convey?

On our current version of ibc-go, it seems the error reported is module error code without the corresponding codespace. Codespace is how we'd determine if error code 5 means sdk/5 insufficient funds, or or bank/5 send transactions are disabled.
It seems like we may not be able to make much sense of these until we are on ibc-go@^8.3.x, but need to consider an interim solution. Here is a ticket to capture the issue: #9629

turadg · 2024-07-01T14:50:23Z

packages/orchestration/test/network-fakes.ts

+      blockHeight: 289,
+      blockTime: 1712180320,


could these be random or incrementing? I don't see any testing of these particular values

They could be, but yes, we are not testing anything against blockTime or blockHeight since application code does not currently rely on them.

These values seems like they could be useful for testing packet timeouts, but I suspect we can cover that path by using buildMsgErrorString and implicitly throwing a timeout error through the testing utils.

For testing something like whether IBCMsgTransferOptions are honored/correctly specified for .transfer(), e2e tests with sim chains seem like a better place for this.

turadg · 2024-07-01T14:51:50Z

packages/orchestration/test/service.test.ts

    ),
-    { message: /\\"data\\":\\"(.*)\\"memo\\":\\"\\"/ },
-    'TODO do not use echo connection',


packages/vats/src/types.d.ts

cloudflare-workers-and-pages · 2024-07-01T19:32:34Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`0bd7161`
Status:	✅ Deploy successful!
Preview URL:	https://881f280c.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-9572-orch-ibc-mocks.agoric-sdk.pages.dev

View logs

0xpatrickdev · 2024-07-01T20:01:17Z

I wonder about how we can DRY with RunUtils tests à la #8963.

Agree. Something at least directionally helpful would be to have ibc-mocks live in one place (@agoric/orchestration, i think) and have package/boot depend on it:
https://github.com/Agoric/agoric-sdk/blob/26c8b3daaf4f38ddcf0e0bb25945a1af89302504/packages/boot/tools/ibc/mocks.js

This was something considered but I recalled a suggestion to the effect of ~"every package should be responsible for it's own testing setup". Mocks / fixtures seems like they could be an exception to this, though.

LMK if you'd like to see that in this PR.

I also wonder about how to improve or at least maintain fidelity of these mocks.

Agree, interested to hear more thoughts on this.

turadg · 2024-07-01T23:41:39Z

packages/orchestration/src/utils/packet.js

-      value: encodeBase64(resp.value),
-    })),
-  );
+  return harden(responses.map(ResponseQuery.toJSON));


turadg · 2024-07-01T23:43:12Z

packages/orchestration/test/examples/stake-ica.contract.test.ts

@@ -104,16 +111,19 @@ test('makeAccount, getAddress, getBalances, getBalance', async t => {
    t.regex(chainAddress.address, /osmo1/);
    t.like(chainAddress, { chainId: 'osmosis-1' });

-    await E(ibcBridge).setMockAck({
-      [buildQueryPacketString([
+    const buildMocks = () => {


good thinking to encapsulate the temporary values. another option is an IIFE

0xpatrickdev requested a review from turadg July 1, 2024 14:21

0xpatrickdev commented Jul 1, 2024

View reviewed changes

packages/orchestration/test/network-fakes.test.ts Outdated Show resolved Hide resolved

0xpatrickdev commented Jul 1, 2024

View reviewed changes

packages/orchestration/test/network-fakes.ts Outdated Show resolved Hide resolved

turadg reviewed Jul 1, 2024

View reviewed changes

turadg approved these changes Jul 1, 2024

View reviewed changes

0xpatrickdev added 6 commits July 1, 2024 20:39

test(orchestration): include ibc bridge in fake network

235fd37

test(orchestration): tools for creating ibc mocks

426c3a1

chore: rename stake-ica.contract.test

5202bfa

test(orchestration): mock unique ica addresses

aec6a77

test(orchestration): custom ibc mocks per test

aa0508e

fix: parseQueryPacket should use ResponseQuery.toJSON

0bd7161

0xpatrickdev force-pushed the pc/9572-orch-ibc-mocks branch from 89befbd to 0bd7161 Compare July 2, 2024 00:39

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Jul 2, 2024

mergify bot merged commit 5d7774e into master Jul 2, 2024
85 checks passed

mergify bot deleted the pc/9572-orch-ibc-mocks branch July 2, 2024 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(orchestration): ibc mocking utils #9628

test(orchestration): ibc mocking utils #9628

0xpatrickdev commented Jul 1, 2024

turadg left a comment •

edited

Loading

turadg Jul 1, 2024

0xpatrickdev Jul 1, 2024

turadg Jul 1, 2024

0xpatrickdev Jul 1, 2024

turadg Jul 1, 2024

0xpatrickdev Jul 1, 2024

turadg Jul 1, 2024

0xpatrickdev Jul 1, 2024

turadg Jul 1, 2024

0xpatrickdev Jul 1, 2024

turadg Jul 1, 2024

0xpatrickdev Jul 1, 2024

turadg Jul 1, 2024

cloudflare-workers-and-pages bot commented Jul 1, 2024 •

edited

Loading

0xpatrickdev commented Jul 1, 2024 •

edited

Loading

turadg Jul 1, 2024

turadg Jul 1, 2024

	typeUrl: string;
	/** Must be a valid serialized protocol buffer of the above specified type. */
	value: Uint8Array;
	}

test(orchestration): ibc mocking utils #9628

test(orchestration): ibc mocking utils #9628

Conversation

0xpatrickdev commented Jul 1, 2024

Description

turadg left a comment • 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

cloudflare-workers-and-pages bot commented Jul 1, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

0xpatrickdev commented Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 1, 2024 •

edited

Loading

0xpatrickdev commented Jul 1, 2024 •

edited

Loading