feat: `CosmosChainInfo` includes `pfmEnabled: bool` #10329

0xpatrickdev · 2024-10-23T21:41:04Z

refs: #10006
refs: #10445

Description

adds pfmEnabled: bool to CosmosChainInfo to support multi-hop forwarding logic
adds and exports withChainCapabilities helper that mixes in PfmEnabled and IcqEnabled constants to ChainInfo
adds and exports registerChainsAndAssets helper that registers info in a chainHub in a contract startFn
implements chainHub initialization in fast-usdc and send-anywhere contracts

Security Considerations

chain-capabilities.js is authoritative, but consumers have the ability to provide their own data. It's not published to vstorage and will be mixed in to local ChainHub's in example contracts that rely on it.

Scaling Considerations

Authors must maintain chain-capabilities.js over time

Documentation Considerations

documented via typedoc

Testing Considerations

updates snapshot tests

Upgrade Considerations

Library code, part of an NPM Orch release

cloudflare-workers-and-pages · 2024-10-23T21:42:25Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`118bc6a`
Status:	✅ Deploy successful!
Preview URL:	https://305f3f1e.agoric-sdk.pages.dev
Branch Preview URL:	https://10006-transfer-pfm.agoric-sdk.pages.dev

View logs

packages/orchestration/src/chain-capabilities.js

dckc · 2024-10-23T23:17:13Z

packages/orchestration/src/exos/local-orchestration-account.js

-          { opts, amount, destination },
+          { opts, denomAmount, destination, isPfm },


is renaming the amount property a breaking change?

probably doesn't matter yet, but we should keep this sort of thing in mind

This change will be part of a new PR, see comment, but thanks for pointing this out.

This interface is part of .transfer()'s internals, and doesn't get exposed to a consumer. I believe that means it's not a breaking change, but let me know if that's off base. Exo upgrade considerations are something I'm always looking to better understand.

The state record gets written to virtual storage. Upgraded exos have to use the same state properties, IIUC.
I think our baggage test captures this. Once the contract is deployed, we're committed to the state in baggage. We can evolve it, but only in certain limited ways.

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

0xpatrickdev · 2024-11-11T16:07:27Z

Reviewers, we are moving the .transfer() changes to a new PR (tracked by #10445) and keeping this scoped to ChainInfo changes.

dckc

Putting stuff that does not come from the chain registry into fetched-chain-info doesn't seem right to me.

Whatever consumes fetched-chain-info should mix in PFM_ENABLED, IMO.

dckc · 2024-11-12T16:14:22Z

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

-#!/usr/bin/env tsx
+#!/usr/bin/env TS_BLANK_SPACE_EMIT=false node --import ts-blank-space/register


interesting!

dckc · 2024-11-12T16:19:28Z

packages/orchestration/src/utils/registry.js

+      icqEnabled: ICQ_ENABLED.includes(
+        // @ts-expect-error string not assignable to `ICQ_ENABLED` const


suggestion: cast ICQ_ENABLED to string[] instead.

dckc · 2024-11-12T16:21:22Z

packages/orchestration/src/fetched-chain-info.js

@@ -9,6 +9,7 @@ export default /** @type {const} } */ ({
      },
    ],
    icqEnabled: false,
+    pfmEnabled: true,


Putting stuff that does not come from the chain registry into fetched-chain-info doesn't seem right to me.

Whatever consumes fetched-chain-info should mix in PFM_ENABLED, IMO.

more changes included

turadg

Thanks for the clean commits. LGTM.

- in order to determine whether its possible to route a transfer through a particular ibc chain, we must know if they have the PFM module installed

- adds `IcqEnabled` and `PfmEnabled` constants to support `CosmosChainInfo` - this data is not available via a well-known registry like cosmos/chain-registry, but necessary for the Orchestration API - exports `withChainCapabilities` helper so consumers can include this data with fetched-chain-info.js

- use `ts-blank-space` since `tsx` is no longer in `node_modules/.bin` - prefer yarn command instead of standalone script

- helper function that takes chainInfo and assetInfo and registers the data in ChainHub

- `withOrchestration` the baggage that was provided to it by the original caller - needed so users of `withOrchestration` can access baggage

- split start-send-anywhere.js into proposal and start script so we can pass options - init-send-anywhere.js takes `--chainInfo` and `--assetInfo` options - send-anywhere.contrac.js registers chainInfo and assetInfo from privateArgs in ChainHub

- this test is flaky and will be addressed as part of #10565

dckc

this is how far I got before a meeting

dckc · 2024-11-26T16:42:29Z

multichain-testing/test/send-anywhere.test.ts

+    [`ibc/${denomHash({ denom: 'uatom', channelId: chainInfo.agoric.connections['gaialocal'].transferChannel.channelId })}`]:
+      {
+        baseName: 'cosmoshub',
+        chainName: 'agoric',
+        baseDenom: 'uatom',
+      },


This doesn't connect the relevant brand.

dckc · 2024-11-26T16:46:56Z

multichain-testing/test/send-anywhere.test.ts


 test.before(async t => {
  const { deleteTestKeys, setupTestKeys, ...rest } = await commonSetup(t);
  deleteTestKeys(accounts).catch();
  const wallets = await setupTestKeys(accounts);
  t.context = { ...rest, wallets, deleteTestKeys };
  const { startContract } = rest;
-  await startContract(contractName, contractBuilder);
+
+  const assetInfo = {


a helper for this comes to mind:

const assetInfo = Object.fromEntries([ assetOn('uosmo', 'osmosis'), assetOn('uosmo', 'osmosis', 'agoric', chainInfo), assetOn('uatom', 'cosmoshub'), assetOn('uatom', 'cosmoshub', 'agoric', 'chainInfo), ])

dckc · 2024-11-26T17:36:58Z

multichain-testing/test/send-anywhere.test.ts

+
+  await startContract(contractName, contractBuilder, {
+    chainInfo: JSON.stringify(withChainCapabilities(chainInfo)),
+    assetInfo: JSON.stringify(assetInfo),


Using LegibleCapData would let us include brands in assetInfo.

agoric-sdk/packages/fast-usdc/src/utils/config-marshal.js

Lines 69 to 73 in 8657c4c

/**

* The smallCaps body is a string, which simplifies some usage.

* But it's hard to read and write.

*

* The parsed structure makes a convenient notation for configuration etc.

dckc · 2024-11-26T17:39:57Z

packages/builders/scripts/fast-usdc/init-fast-usdc.js

 */

 const { keys } = Object;

+const defaultAssetInfo = {


again:

const defaultAssetInfo = Object.fromEntries([ assetOn('uusdc', 'noble'), assetOn('uusdc', 'noble', 'agoric', chainInfo), assetOn('uusdc', 'noble', 'osmosis', chainInfo), ]);

dckc · 2024-11-26T17:43:23Z

packages/builders/scripts/fast-usdc/init-fast-usdc.js

+      baseName: 'noble',
+      chainName: 'agoric',
+      baseDenom: 'uusdc',
+      brandKey: 'USDC',


What consumes brandKey? What's the type of defaultAssetInfo?

brandKey is another example of the sort of thing that LegibleCapData is designed to address.

For background, see

bootstrap config: uniform treatment of contract terms, governance parameters #7309

This is good feedback. I went with brandKey since I'm not well versed in this area.

I created a ticket to track the removal of brandKey in favor of this approach: #10580

Thanks. I think it's an entirely internal interface, so it's not a big deal.

dckc · 2024-11-26T17:45:07Z

packages/fast-usdc/src/fast-usdc.start.js

@@ -33,6 +35,8 @@ const contractName = 'fastUsdc';
 *   oracles: Record<string, string>;
 *   feeConfig: FeeConfig;
 *   feedPolicy: FeedPolicy & Passable;
+ *   chainInfo: Record<string, CosmosChainInfo & Passable>;
+ *   assetInfo: Record<Denom, DenomDetail & {brandKey?: string}>;


FastUSDCConfig shouldn't need brandKey. It's already designed to work with LegibleCapData.

#10329 (comment)

dckc · 2024-11-26T17:53:15Z

packages/orchestration/src/utils/chain-hub-helper.js

+  assetInfo,
+) => {
+  if (!chainInfo) {
+    console.log('No chain info provided, returning early.');


How about logging the keys of chainInfo in any case?

Good idea, I see you've tackled this in #10572

dckc · 2024-11-26T17:53:41Z

packages/orchestration/src/utils/chain-hub-helper.js

+      }
+    }
+  }
+


log registeredPairs for connection info?

dckc · 2024-11-26T17:54:21Z

packages/orchestration/src/utils/chain-hub-helper.js

+  }
+
+  if (!assetInfo) {
+    console.log('No asset info provided, returning early.');


again, how about logging the keys of assetInfo in any case?
Logging IBC denoms is kinda noisy, but probably not too bad.

dckc · 2024-11-26T17:55:27Z

packages/orchestration/src/utils/chain-hub-helper.js

+ * @param {ChainHub} chainHub
+ * @param {Record<string, Brand<'nat'>>} brands
+ * @param {Record<string, CosmosChainInfo> | undefined} chainInfo
+ * @param {Record<Denom, DenomDetail & { brandKey?: string }> | undefined} assetInfo


Using LegibleCapData would avoid the special case for brandKey here.

0xpatrickdev force-pushed the 10006-transfer-pfm branch from 1475022 to efe74b3 Compare October 23, 2024 21:42

0xpatrickdev force-pushed the 10006-transfer-pfm branch from efe74b3 to 0395222 Compare October 23, 2024 22:07

dckc reviewed Oct 23, 2024

View reviewed changes

packages/orchestration/src/chain-capabilities.js Outdated Show resolved Hide resolved

dckc reviewed Oct 23, 2024

View reviewed changes

0xpatrickdev force-pushed the 10006-transfer-pfm branch 2 times, most recently from d019841 to 9e08694 Compare October 29, 2024 16:02

turadg added this to the FU1: package integration milestone Nov 1, 2024

turadg assigned iomekam Nov 1, 2024

turadg mentioned this pull request Nov 1, 2024

PFM transfer covering remote chain A's view of remote B's denoms #10006

Open

turadg reviewed Nov 6, 2024

View reviewed changes

packages/orchestration/scripts/fetch-chain-info.ts Show resolved Hide resolved

0xpatrickdev changed the title ~~feat(local-orchestration-account): transfer supports multi-hop pfm routes~~ feat: CosmosChainInfo includes pfmEnabled: bool Nov 11, 2024

0xpatrickdev force-pushed the 10006-transfer-pfm branch from 38ded26 to bfd8405 Compare November 11, 2024 16:23

0xpatrickdev assigned 0xpatrickdev and unassigned iomekam Nov 11, 2024

0xpatrickdev force-pushed the 10006-transfer-pfm branch 2 times, most recently from e0fbda5 to d8057f2 Compare November 11, 2024 16:32

0xpatrickdev marked this pull request as ready for review November 11, 2024 16:38

0xpatrickdev requested a review from a team as a code owner November 11, 2024 16:38

0xpatrickdev requested review from AgoricTriage, turadg and dckc November 11, 2024 16:38

0xpatrickdev added the force:integration Force integration tests to run on PR label Nov 12, 2024

0xpatrickdev force-pushed the 10006-transfer-pfm branch 4 times, most recently from 5617071 to 784feb2 Compare November 12, 2024 01:39

dckc reviewed Nov 12, 2024

View reviewed changes

0xpatrickdev force-pushed the 10006-transfer-pfm branch from 784feb2 to d08f946 Compare November 13, 2024 22:26

0xpatrickdev force-pushed the 10006-transfer-pfm branch 3 times, most recently from 2c1661d to b746eee Compare November 26, 2024 00:18

turadg approved these changes Nov 26, 2024

View reviewed changes

0xpatrickdev force-pushed the 10006-transfer-pfm branch 6 times, most recently from 737a964 to d6db026 Compare November 26, 2024 02:58

0xpatrickdev and others added 11 commits November 26, 2024 02:07

feat: CosmosChainInfo includes pfmEnabled?: boolean

e1c35da

- in order to determine whether its possible to route a transfer through a particular ibc chain, we must know if they have the PFM module installed

fix: yarn codegen script

9eea3fd

- use `ts-blank-space` since `tsx` is no longer in `node_modules/.bin` - prefer yarn command instead of standalone script

add pfmEnabled to shape

c1b406c

feat: registerChainsAndAssets

e72782d

- helper function that takes chainInfo and assetInfo and registers the data in ChainHub

feat: withOrchestration returns baggage

e4a6c6d

- `withOrchestration` the baggage that was provided to it by the original caller - needed so users of `withOrchestration` can access baggage

feat: export DenomDetailShape

2dfddb3

feat: parameterize fusdc with chainInfo and assetInfo

e5a8b64

feat: fusdc assetInfo and chainInfo by netname

afb4f34

feat: support builderOpts

032f98a

feat: send-anywhere inits chainHub

2fa2f75

- split start-send-anywhere.js into proposal and start script so we can pass options - init-send-anywhere.js takes `--chainInfo` and `--assetInfo` options - send-anywhere.contrac.js registers chainInfo and assetInfo from privateArgs in ChainHub

0xpatrickdev force-pushed the 10006-transfer-pfm branch from d6db026 to 2fa2f75 Compare November 26, 2024 07:07

test: disable exitOffer tool reclaims stuck payment

118bc6a

- this test is flaky and will be addressed as part of #10565

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Nov 26, 2024

mergify bot merged commit 8657c4c into master Nov 26, 2024
86 of 95 checks passed

mergify bot deleted the 10006-transfer-pfm branch November 26, 2024 17:06

dckc reviewed Nov 26, 2024

View reviewed changes

This was referenced Nov 27, 2024

orchestration / fast-usdc: remove brandKey from assetInfo in privateArgs #10580

Open

ChainHub method should be sync #10602

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `CosmosChainInfo` includes `pfmEnabled: bool` #10329

feat: `CosmosChainInfo` includes `pfmEnabled: bool` #10329

0xpatrickdev commented Oct 23, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 23, 2024 •

edited

Loading

dckc Oct 23, 2024

0xpatrickdev Nov 11, 2024

dckc Nov 26, 2024

0xpatrickdev commented Nov 11, 2024 •

edited

Loading

dckc left a comment

dckc Nov 12, 2024

dckc Nov 12, 2024

dckc Nov 12, 2024

turadg left a comment

dckc left a comment

dckc Nov 26, 2024

dckc Nov 26, 2024

dckc Nov 26, 2024

dckc Nov 26, 2024

dckc Nov 26, 2024

0xpatrickdev Nov 27, 2024

dckc Nov 27, 2024

dckc Nov 26, 2024

0xpatrickdev Nov 27, 2024

dckc Nov 26, 2024

0xpatrickdev Nov 27, 2024

dckc Nov 27, 2024

dckc Nov 26, 2024

dckc Nov 26, 2024

dckc Nov 26, 2024

		{ opts, amount, destination },
		{ opts, denomAmount, destination, isPfm },

		#!/usr/bin/env tsx
		#!/usr/bin/env TS_BLANK_SPACE_EMIT=false node --import ts-blank-space/register

		icqEnabled: ICQ_ENABLED.includes(
		// @ts-expect-error string not assignable to `ICQ_ENABLED` const

	/**
	* The smallCaps body is a string, which simplifies some usage.
	* But it's hard to read and write.
	*
	* The parsed structure makes a convenient notation for configuration etc.

feat: CosmosChainInfo includes pfmEnabled: bool #10329

feat: CosmosChainInfo includes pfmEnabled: bool #10329

Conversation

0xpatrickdev commented Oct 23, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Oct 23, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev commented Nov 11, 2024 • edited Loading

dckc 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 left a comment

Choose a reason for hiding this comment

dckc 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

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

feat: `CosmosChainInfo` includes `pfmEnabled: bool` #10329

feat: `CosmosChainInfo` includes `pfmEnabled: bool` #10329

0xpatrickdev commented Oct 23, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 23, 2024 •

edited

Loading

0xpatrickdev commented Nov 11, 2024 •

edited

Loading