feat(orchestration): add more chain infos for fusdc #10845

samsiegart · 2025-01-14T08:03:57Z

closes #10736
refs #10629

Description

Adds remaining cosmos chains for fusdc as listed in https://github.com/Agoric/product-tasks/issues/186#issuecomment-2442824919.
Adds icaEnabled property to CosmosChainInfo
Makes the connection infos in vstorage support chain IDs with underscores (_). Since we use underscores as a separator in the vstorage key, we sanitize the chain IDs by doubling the underscores (similar to double-quote in csv)
Don't use all the chain infos in bootstrap tests, only a subset, since CI can't handle the input size

Security Considerations

We're adding non-ica-enabled chains now, so #10629 is relevant.

Scaling Considerations

packages/orchestration/src/fetched-chain-info.js is quite large, hopefully not too large?

Documentation Considerations

Not sure if these chains need to be documented somewhere.

Testing Considerations

Just did yarn codegen. Not sure how to test this for real.

Upgrade Considerations

Updating the chain infos will presumably require a core-eval

cloudflare-workers-and-pages · 2025-01-15T05:50:08Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`bcc450c`
Status:	✅ Deploy successful!
Preview URL:	https://2f334f36.agoric-sdk.pages.dev
Branch Preview URL:	https://fu-chain-info.agoric-sdk.pages.dev

View logs

samsiegart · 2025-01-15T18:24:06Z

Problem: When adding all the chain infos, it causes an E2BIG error in CI. This happens during the bootstrap test "send-anywhere", as evalProposal tries spawning a process with the very large chain info list as an input (see log).

It works on my computer, just not in CI. So, I wonder:

Can we bump the CI memory limit somehow?
Or only use a subset of the chain infos for this test?
Is there a risk that this won't work on-chain?

Edit:
Went with the option only use a subset of the chain infos for this test.

samsiegart · 2025-01-16T06:19:48Z

Finally got test passing. Had to make some decisions (see description) so please take a look @0xpatrickdev @turadg

turadg · 2025-01-16T15:18:17Z

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

@@ -331,7 +331,15 @@ test.serial('basic-flows', async t => {
      '@agoric/builders/scripts/orchestration/init-basic-flows.js',
      [
        '--chainInfo',
-        JSON.stringify(withChainCapabilities(fetchedChainInfo)),
+        JSON.stringify(
+          withChainCapabilities({


this is repeated below. It would help legibility and maintainability if this value were named like fetchedChainInfo is. please extract to const like minimalSubsetChainInfo or what have you.

I see it's used in multiple modules so consider making a new one for named chain info subsets which tests and contracts could import from.

Just put this in tools for reuse

turadg · 2025-01-16T15:19:12Z

packages/boot/test/fast-usdc/fast-usdc.test.ts

+    // XXX for some reason the code after this when run under XS fails with:
+    // message: 'unsettled value for "kp2526"',


We have an issue now which suffices and has the advantage of an easy query for cleanup.

Suggested change

// XXX for some reason the code after this when run under XS fails with:

// message: 'unsettled value for "kp2526"',

// UNTIL https://github.com/Agoric/agoric-sdk/issues/10847

Just removed all this code because of cab3fbe

packages/boot/test/fast-usdc/fast-usdc.test.ts

turadg · 2025-01-16T15:23:49Z

packages/orchestration/scripts/fetch-chain-info.ts

 });

 // chain info, assets and ibc data will be downloaded dynamically by invoking fetchUrls method
 await client.fetchUrls();

-const chainInfo = await convertChainInfo(client);
+const icaChainSet = new Set(icaChainNames);


consider icaChainNames itself being a Set. there really is no ordering to them

turadg · 2025-01-16T15:25:04Z

packages/orchestration/src/exos/chain-hub.js

+ * {@link CHAIN_ID_SEPARATOR} in the chain ID to make it clear that it's part of
+ * the chain ID rather than a separator.


it's not just making it clear. it's making it a reversible encoding.

Fixed the wording to be more precise

turadg · 2025-01-16T15:26:13Z

packages/orchestration/src/exos/chain-hub.js

+  chainId.replaceAll(
+    CHAIN_ID_SEPARATOR,
+    `${CHAIN_ID_SEPARATOR}${CHAIN_ID_SEPARATOR}`,
+  );


on the fence about this suggestion, but since this expression is so close to the const I think it could be clearer:

Suggested change

chainId.replaceAll(

CHAIN_ID_SEPARATOR,

`${CHAIN_ID_SEPARATOR}${CHAIN_ID_SEPARATOR}`,

);

chainId.replaceAll('_', '__');

I see what you mean but I prefer keeping the variable here

turadg · 2025-01-16T15:27:38Z

packages/orchestration/src/exos/chain-hub.js

 * @see {@link https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-2.md}
 */
-const CHAIN_ID_SEPARATOR = '_';
+export const sanitizeChainId = chainId =>


it's not sanitizing it. it's encoding it so it can appear in the tuple

Fixed the wording

turadg · 2025-01-16T15:29:01Z

packages/orchestration/src/utils/registry.js

@@ -95,6 +96,7 @@ export const convertChainInfo = async registry => {
      stakingTokens: chain.staking?.staking_tokens,
      // UNTIL https://github.com/Agoric/agoric-sdk/issues/9326
      icqEnabled: chain.chain_name === 'osmosis',
+      icaEnabled: icaEnabled(chain.chain_name),


the same name is being used for a property and a query function. please rename the query function.

turadg · 2025-01-16T15:29:53Z

multichain-testing/scripts/fetch-starship-chain-info.ts

+    chains,
+    ibcData: ibc.data,
+  },
+  () => true,


this happens to be true but should it be assumed?

Might as well not assume. Just changed this to use the imported list of icaEnabled chains

0xpatrickdev

Can we add a test to FUSDC that ensures there's a channel between noble and all of the chains in the ticket? This would ensure chain-registry has all of the information we're after.

NB: in registry.js, the script prints "multiple preferred transfer channels" as a console.warn. It be worth auditing more closely at this time and raising if we find multiple valid channels for noble -> another remote chain.

0xpatrickdev · 2025-01-16T21:19:28Z

packages/orchestration/scripts/fetch-chain-info.ts

@@ -12,7 +12,7 @@ const outputFile = 'src/fetched-chain-info.js';
 /**
 * Names for which to fetch info
 */
-const chainNames = [
+export const icaChainNames = new Set([


Thanks for including this distinction.

I would suggest keeping a single list here and moving icaChains to chain-capabilities.js as IcaEnabled.

It's a bit of duplication, but in #10329 it was agreed it was best to keep non-chain registry info out of fetch-chain-info / fetched-chain-info.

I forgot about that. Thanks for catching it @0xpatrickdev .

0xpatrickdev · 2025-01-16T21:25:40Z

packages/orchestration/scripts/fetch-chain-info.ts

 });

 // chain info, assets and ibc data will be downloaded dynamically by invoking fetchUrls method
 await client.fetchUrls();

-const chainInfo = await convertChainInfo(client);
+const chainInfo = await convertChainInfo(client, name =>
+  icaChainNames.has(name),


Can we leave this part out for now? We should add icaEnabled via withChainCapabilities instead.

packages/boot/test/fast-usdc/fast-usdc.test.ts

samsiegart

Can we add a test to FUSDC that ensures there's a channel between noble and all of the chains in the ticket? This would ensure chain-registry has all of the information we're after.

Added a test, however not every connection seems to exists. I documented which ones in the test. We'll need to follow up on those.

NB: in registry.js, the script prints "multiple preferred transfer channels" as a console.warn. It be worth auditing more closely at this time and raising if we find multiple valid channels for noble -> another remote chain.

I manually reviewed and couldn't find any such warnings involving noble. However, there's some missing preferred channels as I mentioned above.

samsiegart · 2025-01-21T18:53:48Z

packages/orchestration/scripts/fetch-chain-info.ts

 });

 // chain info, assets and ibc data will be downloaded dynamically by invoking fetchUrls method
 await client.fetchUrls();

-const chainInfo = await convertChainInfo(client);
+const chainInfo = await convertChainInfo(client, name =>
+  icaChainNames.has(name),


samsiegart · 2025-01-21T18:53:54Z

packages/orchestration/scripts/fetch-chain-info.ts

@@ -12,7 +12,7 @@ const outputFile = 'src/fetched-chain-info.js';
 /**
 * Names for which to fetch info
 */
-const chainNames = [
+export const icaChainNames = new Set([


samsiegart · 2025-01-21T18:56:50Z

I'll need to update test snapshots but if you could, please take another look @0xpatrickdev thanks!

turadg · 2025-01-21T18:55:45Z

packages/orchestration/src/chain-capabilities.js

@@ -38,23 +38,43 @@ const PfmEnabled = /** @type {const} */ ({
 });
 harden(PfmEnabled);

+const IcaEnabled = /** @type {const} */ ({


from where is this info derived? what's the process to update it?

Manually derived. To update automatically we might be able to get a public RPC endpoint for each chain from the chain registry, then query the endpoints for the ICA info. However, the RPC endpoints in chain registry are often unreliable.

That's cool. I should have been more clear… these are questions I think the source code should answer. Please document.

I thought we maybe had a date in the @file declaration, but am seeing it's just @file Contains ChainInfo that not available from a well-known chain registry.. To @turadg's feedback, I would personally be satisfied if we put a Last (Verified|Updated) Date to denote that this info can go stale.

turadg · 2025-01-21T18:56:53Z

packages/fast-usdc/test/chain-info.test.js

+
+// List of chains to test to ensure an IBC connection exists from noble to them
+// XXX: Some missing connections.
+const fastUsdcDestinationChains = [


should this be available outside the test? e.g. for the contract to handle differently a destination that isn't in this list?

Not sure. Possibly something to ensure we have a testing plan for... if we get a destination that's not supported.

If the contract is presented with an unknown destination, we should get ADVANCE_FAILED and something like no connection info found for... in the swingset logs.

That's to say, i don't think we need to do anything here: "for the contract to handle differently a destination that isn't in this list"

However, this is something FEs might find useful. It's not in our current design, but I could imagine some version of this info going into vstorage.

0xpatrickdev · 2025-01-22T01:05:34Z

multichain-testing/scripts/fetch-starship-chain-info.ts

@@ -5,6 +5,7 @@ import fsp from 'node:fs/promises';
 import prettier from 'prettier';

 import { convertChainInfo } from '@agoric/orchestration/src/utils/registry.js';
+import { icaChainNames } from '@agoric/orchestration/scripts/fetch-chain-info.ts';


Is this just chainNames now? And does convertChainInfo still take a 2nd arg? force-integration should be a cheapish way to verify this change is correct

Oh yea this is all unnecessary now, and the test was in fact failing because this import doesn't exist anymore.

0xpatrickdev · 2025-01-22T01:09:40Z

packages/fast-usdc/test/chain-info.test.js

+
+// List of chains to test to ensure an IBC connection exists from noble to them
+// XXX: Some missing connections.
+const fastUsdcDestinationChains = [


If the contract is presented with an unknown destination, we should get ADVANCE_FAILED and something like no connection info found for... in the swingset logs.

That's to say, i don't think we need to do anything here: "for the contract to handle differently a destination that isn't in this list"

However, this is something FEs might find useful. It's not in our current design, but I could imagine some version of this info going into vstorage.

0xpatrickdev · 2025-01-22T01:11:43Z

packages/fast-usdc/test/chain-info.test.js

+const testNobleConnection = test.macro({
+  exec(t, input) {
+    const info = fetchedChainInfo[input];
+    const { chainId } = info;
+    const connection = fetchedChainInfo.noble.connections[chainId];
+    t.truthy(connection);
+  },
+  title(_, input) {
+    return `Connection from noble to ${input}`;
+  },
+});
+
+for (const chain of fastUsdcDestinationChains) {
+  test(testNobleConnection, chain);
+}


Thank you 🙏

0xpatrickdev · 2025-01-22T01:14:50Z

packages/orchestration/src/chain-capabilities.js

@@ -38,23 +38,43 @@ const PfmEnabled = /** @type {const} */ ({
 });
 harden(PfmEnabled);

+const IcaEnabled = /** @type {const} */ ({


I thought we maybe had a date in the @file declaration, but am seeing it's just @file Contains ChainInfo that not available from a well-known chain registry.. To @turadg's feedback, I would personally be satisfied if we put a Last (Verified|Updated) Date to denote that this info can go stale.

samsiegart requested a review from a team as a code owner January 14, 2025 08:03

samsiegart requested review from AgoricTriage and 0xpatrickdev January 14, 2025 08:03

samsiegart force-pushed the fu-chain-info branch from 1802ebd to 111a842 Compare January 15, 2025 05:50

samsiegart added the force:integration Force integration tests to run on PR label Jan 15, 2025

samsiegart force-pushed the fu-chain-info branch 2 times, most recently from 24c3c4e to 513d5ba Compare January 15, 2025 09:24

samsiegart force-pushed the fu-chain-info branch 4 times, most recently from 766a6ba to 8057723 Compare January 16, 2025 05:26

samsiegart requested a review from turadg January 16, 2025 06:19

turadg reviewed Jan 16, 2025

View reviewed changes

turadg approved these changes Jan 16, 2025

View reviewed changes

0xpatrickdev reviewed Jan 16, 2025

View reviewed changes

packages/boot/test/fast-usdc/fast-usdc.test.ts Show resolved Hide resolved

samsiegart commented Jan 21, 2025

View reviewed changes

turadg reviewed Jan 21, 2025

View reviewed changes

samsiegart requested a review from 0xpatrickdev January 21, 2025 18:56

0xpatrickdev approved these changes Jan 22, 2025

View reviewed changes

samsiegart force-pushed the fu-chain-info branch 2 times, most recently from e2d535a to a5066f0 Compare January 22, 2025 06:49

samsiegart added the automerge:rebase Automatically rebase updates, then merge label Jan 22, 2025

feat(orchestration): add more chain infos for fusdc

bcc450c

samsiegart force-pushed the fu-chain-info branch from a5066f0 to bcc450c Compare January 22, 2025 07:09

mergify bot merged commit 8fb6eaf into master Jan 22, 2025
83 checks passed

mergify bot deleted the fu-chain-info branch January 22, 2025 08:02

		// XXX for some reason the code after this when run under XS fails with:
		// message: 'unsettled value for "kp2526"',

	// XXX for some reason the code after this when run under XS fails with:
	// message: 'unsettled value for "kp2526"',
	// UNTIL https://github.com/Agoric/agoric-sdk/issues/10847

		* {@link CHAIN_ID_SEPARATOR} in the chain ID to make it clear that it's part of
		* the chain ID rather than a separator.

feat(orchestration): add more chain infos for fusdc #10845

feat(orchestration): add more chain infos for fusdc #10845

Conversation

samsiegart commented Jan 14, 2025 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Jan 15, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

samsiegart commented Jan 15, 2025 • edited Loading

samsiegart commented Jan 16, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsiegart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsiegart commented Jan 21, 2025

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

samsiegart commented Jan 14, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 15, 2025 •

edited

Loading

samsiegart commented Jan 15, 2025 •

edited

Loading