support non-Cosmos addresses in ChainHub #11007

turadg · 2025-02-14T23:33:35Z

refs: https://github.com/Agoric/agoric-private/issues/250

Description

Introduces new types and guards to use CAIP-2 and CAIP-10.

Maintains backwards compatibility by widening type guards and transforming types as needed.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

New types will appear in generated docs

Testing Considerations

TODO:

fix the type building problem in dependency-graph
A3P test showing this apply on top of fast-usdc-beta and still working

Upgrade Considerations

This has changes for both the API and the contract.

The A3P upgrade test will cover that the contract continue to work with these changes.

The API is kept backwards compatible through widening params and having type aliases.

cloudflare-workers-and-pages · 2025-02-14T23:33:37Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`79aeda0`
Status:	✅ Deploy successful!
Preview URL:	https://f977a34a.agoric-sdk.pages.dev
Branch Preview URL:	https://250-non-cosmos.agoric-sdk.pages.dev

View logs

packages/orchestration/src/orchestration-api.ts

turadg · 2025-02-18T15:32:05Z

packages/orchestration/src/orchestration-api.ts

  chainId: string;
+  // TODO should we call this accountAddress to align with CAIP-10?
  /** The address value used on-chain */


Vitalik has been calling this a "chain-specific address" ethereum/ERCs#735

0xpatrickdev · 2025-02-20T16:55:21Z

packages/orchestration/src/utils/address.js

+  if (parts.length >= 3) {
+    return {
+      chainId: `${parts[0]}:${parts[1]}`,
+      accountAddress: parts.slice(2).join(':'), // Handles cases where the address contains colons


Do we expect this to be the case (colon in address value), or are we being defensive? Maybe better to throw if the length is > 3?

0xpatrickdev · 2025-02-20T16:57:58Z

packages/orchestration/src/cosmos-api.ts

+ *    agoric1megzytg65cyrgzs6fvzxgrcqvwwl7ugpt62346
+ *    cosmosvaloper1npm9gvss52mlmk
+ */
+export type CosmosAddress = `${string}1${string}`;


Maybe we should call this Bech32Address, or even just Bech32? For Ethereum, we could do:

export type (Hex|HexAddress) = `0x${string}`

Maybe too hungarian, but bech32 is also used outside of cosmos

0xpatrickdev · 2025-02-20T17:02:19Z

packages/orchestration/src/orchestration-api.ts

-// TODO should this be AccountId to align with CAIP-10?
-/** An address on some blockchain, e.g., cosmos, eth, etc. */
+/** à la CAIP-2 */
+export type ChainId = `${string}:${string}`;


How does this work for the backwards compat case? i.e. agoric-3 instead of cosmos:agoric-3? The answer might be it doesn't, and this is probably fine, but looking to better understand

One of these is always namespaced. CosmosChainAddress['chainId'] is just agoric-3.

Looks like this comment was reviewing by commit (good idea). In another commit it was changed to ScopedChainId to clarify.

0xpatrickdev · 2025-02-20T17:45:54Z

packages/orchestration/src/exos/ica-account-kit.js

@@ -191,7 +191,6 @@ export const prepareIcaAccountKit = (zone, { watch, asVow }) =>
          this.state.chainAddress = harden({
            value: address || UNPARSABLE_CHAIN_ADDRESS,
            chainId: this.state.chainId,


Would we want to prefix this with cosmos?

I had considered that but it would make more cases to handle. In CosmosChainAddress, chainId is always within the cosmos namespace. The CAIP-10 form always has a namespace.

0xpatrickdev · 2025-02-20T17:52:20Z

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

+            destination =
+              typeof destination === 'string'
+                ? chainHub.makeChainAddress(destination)
+                : destination;


iiuc - destination can be a non-cosmos chain?

I would expect makeTransferRoute to throw if it sees a non-cosmos namespace. is this path tested?

Good point. This is just for IBC. I'll handle make that more explicit

oh good, it was already checking

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

Line 534 in 24525be

assert.equal(namespace, 'cosmos');

but I'll leave a comment here to make the reader aware

0xpatrickdev · 2025-02-20T18:46:04Z

packages/orchestration/src/orchestration-api.ts

@@ -200,7 +229,7 @@ export interface OrchestrationAccountCommon {
   * the transfer is rejected (insufficient funds, timeout)
   */
  transfer: (
-    destination: ChainAddress,
+    destination: AccountId | CosmosChainAddress,


Suggested change

destination: AccountId | CosmosChainAddress,

destination: AccountIdable,

cloudflare-workers-and-pages · 2025-02-21T15:30:39Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`8a4659b`
Status:	⚡️ Build in progress...

View logs

cloudflare-workers-and-pages · 2025-02-21T15:35:09Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`8a4659b`
Status:	✅ Deploy successful!
Preview URL:	https://37eb86ad.agoric-sdk.pages.dev
Branch Preview URL:	https://250-non-cosmos.agoric-sdk.pages.dev

View logs

dckc

I'm uneasy with using syntax that endo patterns can't verify.

In particular, there a few places where the static and dynamic checks don't line up here, and I'm not confident that it's defensively correct in all the relevant cases.

If we're going to use special string syntax, let's please use string as the argument type where the pattern guard says M.string() and then use assertAccountId(x) or the like to validate inside the method.

dckc · 2025-02-21T18:03:02Z

packages/orchestration/test/exos/chain-hub-transfer-routes.test.ts

+    const amt: DenomAmount = harden({ denom: uusdcOnAgoric, value: 100n });
+    t.deepEqual(chainHub.makeTransferRoute(dest, amt, 'agoric'), {
+      receiver: 'noble1234',
+      sourceChannel: 'channel-62',


To be sure that 'channel-62' is right, I'd have to look it up in fetched-chain-info.
To make it more handy, how about adding something like:

const agoricToNoble = knownChains.agoric.connections['noble-1'].transferChannel; t.is(agoricToNoble.id, 'channel-62');

dckc · 2025-02-21T18:10:37Z

packages/builders/test/snapshots/orchestration-imports.test.js.md

+            },
+          },
+          {
+            encoding: Object @match:string {


can the encoding of a cosmos address be anything other than bech32?

the pattern could express that.

and the corresponding static type

given it's ignored, what is the value of erroring when it's not "bech32"?

The corresponding static type doesn't have it at all.

good point. never mind.

packages/builders/test/snapshots/orchestration-imports.test.js.md

packages/fast-usdc/test/exos/settler.test.ts

packages/fast-usdc/src/types.ts

dckc · 2025-02-21T18:47:48Z

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

@@ -547,7 +557,7 @@ export const makeChainHub = (zone, agoricNames, vowTools) => {
     *
     * XXX consider accepting AmountArg #10449
     *
-     * @param {CosmosChainAddress} destination
+     * @param {AccountIdable} destination


The pattern guard doesn't guarantee the AccountIdable type.

dckc · 2025-02-21T18:50:10Z

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

+      return `cosmos:${reference}:${accountAddress}`;
+    },
+    /**
+     * @param {AccountId | Bech32Address} partialId CAIP-10 account ID or a


The M.string() dynamic check doesn't guarantee this static type.

dckc · 2025-02-21T18:51:10Z

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

@@ -576,6 +586,11 @@ export const makeChainHub = (zone, agoricNames, vowTools) => {

      const holdingChainId = chainInfos.get(srcChainName).chainId;

+      destination =
+        typeof destination === 'string'
+          ? chainHub.makeChainAddress(destination)


makeChainAddress takes a AccountId | Bech32Address but only M.string() is guaranteed by dynamic checks so far.

dckc · 2025-02-21T18:57:00Z

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

+        return /** @type {AccountId} */ (partialId);
+      }
+
+      const accountAddress = /** @type {Bech32Address} */ (partialId);


what justifies this cast? partialId is only statically known to be a string, AFAICT.

dckc · 2025-02-21T19:01:33Z

packages/orchestration/test/exos/chain-hub.test.ts

+  const CAIP10_ID =
+    'cosmos:osmosis-1:osmo1ht7u569vpuryp6utadsydcne9ckeh2v8dkd38v5hptjl3u2ewppqc6kzgd';


what if given a chainId that's different from the one know to the chainHub for this bech32 prefix? e.g. cosmos:osmosis-2:osmo1ht....

If that doesn't throw, it seems like there's a bit of a hazard that callers will forget to keep that possibility in mind.

Good case to define and test. ChainHub is only keeping track of the Cosmos chain-id when none is specified. If someone specifies osmosis-1, that's valid, even if ChainHub says "when you see osmo1 without a chain-id use osmosis-3". Right?

That's a coherent design, yes.

turadg requested review from dckc and 0xpatrickdev February 14, 2025 23:34

turadg force-pushed the 250-non-cosmos branch from f486d77 to e7aebd1 Compare February 18, 2025 14:58

turadg commented Feb 18, 2025

View reviewed changes

packages/orchestration/src/orchestration-api.ts Outdated Show resolved Hide resolved

turadg commented Feb 18, 2025

View reviewed changes

turadg force-pushed the 250-non-cosmos branch 3 times, most recently from c4096c3 to 24525be Compare February 20, 2025 01:34

turadg added the force:integration Force integration tests to run on PR label Feb 20, 2025

turadg marked this pull request as ready for review February 20, 2025 02:10

turadg requested a review from a team as a code owner February 20, 2025 02:10

0xpatrickdev reviewed Feb 20, 2025

View reviewed changes

turadg changed the title ~~support non-Cosmos chains in ChainHub~~ support non-Cosmos addresses in ChainHub Feb 20, 2025

turadg force-pushed the 250-non-cosmos branch 2 times, most recently from 3add2cf to 8a4659b Compare February 21, 2025 15:30

turadg requested a review from 0xpatrickdev February 21, 2025 15:30

dckc reviewed Feb 21, 2025

View reviewed changes

Chris-Hibbert mentioned this pull request Feb 21, 2025

248 orchestration depositForBurn #11037

Draft

turadg added 9 commits February 21, 2025 16:24

refactor: extract makeTransferRoute tests

af0114a

docs: EvmHash and EvmAddress

ed8e8f3

chore: remove 'encoding' from ChainAddress

954d39c

feat(types): CosmosAddress

605bbd3

feat: support CAIP-10 AccountId

e13f1de

feat: resolveChainId

18050a9

feat: rename CosmosChainAddress

0ff491f

feat(types): AccountIdable

fc73730

feat: chain-agnostic transfer API

3752404

turadg added 4 commits February 21, 2025 16:25

docs: getBech32Prefix

4946e2b

refactor: better parseAccountId

a068cb8

test: Add comprehensive tests for resolveAccountId method

1cd114d

chore: assert EUD type

79aeda0

turadg force-pushed the 250-non-cosmos branch from 8a4659b to 79aeda0 Compare February 22, 2025 00:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support non-Cosmos addresses in ChainHub #11007

support non-Cosmos addresses in ChainHub #11007

turadg commented Feb 14, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 14, 2025 •

edited

Loading

turadg Feb 18, 2025

0xpatrickdev Feb 20, 2025

0xpatrickdev Feb 20, 2025

0xpatrickdev Feb 20, 2025

turadg Feb 20, 2025

0xpatrickdev Feb 20, 2025

turadg Feb 20, 2025 •

edited

Loading

0xpatrickdev Feb 20, 2025

turadg Feb 20, 2025

turadg Feb 20, 2025

0xpatrickdev Feb 20, 2025

cloudflare-workers-and-pages bot commented Feb 21, 2025

cloudflare-workers-and-pages bot commented Feb 21, 2025

dckc left a comment

dckc Feb 21, 2025

dckc Feb 21, 2025

turadg Feb 21, 2025

dckc Feb 21, 2025

turadg Feb 21, 2025

dckc Feb 22, 2025

dckc Feb 21, 2025

dckc Feb 21, 2025

dckc Feb 21, 2025

dckc Feb 21, 2025

dckc Feb 21, 2025

turadg Feb 21, 2025

dckc Feb 22, 2025

	destination: AccountId \| CosmosChainAddress,
	destination: AccountIdable,

		const CAIP10_ID =
		'cosmos:osmosis-1:osmo1ht7u569vpuryp6utadsydcne9ckeh2v8dkd38v5hptjl3u2ewppqc6kzgd';

support non-Cosmos addresses in ChainHub #11007

Are you sure you want to change the base?

support non-Cosmos addresses in ChainHub #11007

Conversation

turadg commented Feb 14, 2025 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Feb 14, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Feb 20, 2025 • 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

cloudflare-workers-and-pages bot commented Feb 21, 2025

Deploying agoric-sdk with Cloudflare Pages

cloudflare-workers-and-pages bot commented Feb 21, 2025

Deploying agoric-sdk with Cloudflare Pages

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

turadg commented Feb 14, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 14, 2025 •

edited

Loading

turadg Feb 20, 2025 •

edited

Loading