feat: `chainHub.makeTransferRoute()` #10584

0xpatrickdev · 2024-11-27T19:31:49Z

Description

Adds .makeTransferRoute(destination: ChainAddress, amount: DenomAmount, holdingChainName: string) to ChainHub to facilitate building IBC MsgTransfer parameters for single-hop and multi-hop (pfm) routes.

Returns synchronously using local chainHub data to facilitate ease of use at call sites and support future plans to make every call synchronous. It achieves this by interfacing with the chainInfos and connInfos map stores directly instead of the public interface methods that currently fall back to remote calls to agoricNames.

Assumes the longest route will only be 1 intermediary hop (through the issuing chain). Does not support unwinding nested denoms (e.g. noble uusdc sent directly from osmosis to agoric agoric(osmosis(noble(uusdc)))).

Security Considerations

An incorrect implementation could result in loss of funds. #9324 remains open to determine sensible defaults for timeout parameters.

Scaling Considerations

Nothing new, but each contract's chainHub will accumulate quite a bit of data .

Documentation Considerations

JSdoc and tests

Testing Considerations

Includes unit tests covering all codepaths and known scenarios.

Upgrade Considerations

N/A, library code. This feature is needed for FUSDC.

cloudflare-workers-and-pages · 2024-11-27T19:34:53Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`1717b2a`
Status:	✅ Deploy successful!
Preview URL:	https://4b223eef.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-chainhub-get-transfer-rou.agoric-sdk.pages.dev

View logs

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

dckc

I'd like to understand why return undefined rather than throw.

dckc · 2024-11-27T19:36:28Z

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

+ * @property {IBCChannelID} sourceChannel
+ * @property {Coin} token
+ * @property {string} receiver - destination chain address value
+ * @property {ForwardInfo} [forwardInfo] - contains pfm forwarding info


Suggested change

* @property {ForwardInfo} [forwardInfo] - contains pfm forwarding info

* @property {ForwardInfo} [forwardInfo] - contains PFM forwarding info

dckc · 2024-11-27T19:39:36Z

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

+const TransferRouteShape = M.splitRecord(
+  {
+    sourcePort: M.string(),
+    sourceChannel: M.string(),


The type says IBCChannelID. Do we have a shape for that?

Interesting... the one in orch is marked internal.

/** @internal */ export const IBCChannelIDShape = M.string();

dckc · 2024-11-27T19:41:55Z

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

+    token: {
+      amount: M.string(),
+      denom: M.string(),
+    },


A CoinShape would be handy... especially to document the strange use of string for amount.

I hope we're properly defensive about that; that is: when we accept an amount string argument, we don't assume that it's a properly formatted numeral.

getTransferRoute (guarded) takes denomArg which contains a bigint. We cast to a string in this function.

In a future PR, localOrchAccount will probably trust that chainHub did this correctly and not verify the string. Alternatively, we can return a DenomAmount and transform it at the getTransferRoute(). The former is preferably to me but LMK if you still have concerns.

depends on what localOrchAccount is relying on chainHub to do. If it's relying on chainHub to make data to be sent out, then very well. But if localOrchAccount is going to parse the numeral, I would expect it to verify with Nat(BigInt(numeral)).

dckc · 2024-11-27T19:43:17Z

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

+ * `memo` for `MsgTransfer`.
+ *
+ * @typedef {object} TransferRoute
+ * @property {string} sourcePort


Suggested change

* @property {string} sourcePort

* @property {string} sourcePort typically 'transfer'

dckc · 2024-11-27T19:44:33Z

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

+ * @property {string} sourcePort
+ * @property {IBCChannelID} sourceChannel
+ * @property {Coin} token
+ * @property {string} receiver - destination chain address value


Is this really any string? I saw a note from @dtribble that some parts of the IBC world check the bech32 checksum of some address fields.

In TS I'm not sure how to express this, maybe @turadg has ideas. Runtime checks are certainly an option but we can also rely on the cosmos layer to surface bech32 errors.

Here, receiver is not required to be valid bech32. If we have forwardInfo this will be "pfm". Supposes this part could be expressed in the types. Edit: TransferRoute now uses a discriminated union, but still takes no opinion on whether a string is bech32.

dckc · 2024-11-27T19:50:36Z

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

+      if (!chainInfos.has(holdingChainName)) {
+        log(`chain info not found for holding chain: ${holdingChainName}`);
+        return undefined;


Why not throw? Why is it OK for the caller use this with an unregistered chain name?

Likewise for the rest. I can't see any reason that the caller shouldn't establish the preconditions.

I cam to the same realization shortly after putting this up:
#10584 (comment)

Amended to throw.

dckc · 2024-11-27T19:56:37Z

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

+          receiver: destination.value,
+          port: issuerToDest.transferChannel.portId,
+          channel: issuerToDest.transferChannel.channelId,
+          ...{


why the extra nesting?

dckc · 2024-11-27T19:58:56Z

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

+          denom: denomAmount.denom,
+        },
+        /**
+         * purposely using invalid bech32


This comment is nice... but I'd rather see this comment on the type.

dckc · 2024-11-27T20:04:21Z

packages/orchestration/src/utils/asset.js

+ * @param {string} [chainName]
+ * @param {Record<string, CosmosChainInfo>} [infoOf]
+ * @param {string} [brandKey]
+ * @returns {[string, DenomDetail & { brandKey?: string }]}


I'm not comfortable baking brandKey into the orch API.

Added a TODO that references #10580

But that would be a breaking change to a high-visibility API.

Is brandKey essential for this PR? Can we just use brands?

brandKey is an existing convention from registerChainsAndAssets, landed on master in e72782d. The ticket mentions the three contracts expecting it their build scripts.

assetOn is not currently exported from @agoric/orchestration

I'm juggling too much. Somehow I got the impression that brandKey hadn't escaped fast-usdc.

assetOn is not exported... ok.

dckc · 2024-11-27T20:05:38Z

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

-    const infoWithBrand = info.brandKey
-      ? { ...info, brand: brands[info.brandKey] }
-      : info;
+    const { brandKey, ...rest } = info;
+    const infoWithBrand = brandKey
+      ? { ...rest, brand: brands[brandKey] }
+      : rest;


👏

I meant to say something about that earlier.

dckc · 2024-11-27T22:08:17Z

packages/orchestration/src/cosmos-api.ts

+export type TransferRoute =
+  | {
+      /** typically, `transfer` */
+      sourcePort: string;
+      sourceChannel: IBCChannelID;
+      token: Coin;
+      receiver: typeof PFM_RECEIVER;
+      /** contains PFM forwarding info */
+      forwardInfo: ForwardInfo;
+    }
+  | {
+      /** typically, `transfer` */
+      sourcePort: string;
+      sourceChannel: IBCChannelID;
+      token: Coin;
+      receiver: string;
+    };


nice.

could be a bit more concise:

Suggested change

export type TransferRoute =

| {

/** typically, `transfer` */

sourcePort: string;

sourceChannel: IBCChannelID;

token: Coin;

receiver: typeof PFM_RECEIVER;

/** contains PFM forwarding info */

forwardInfo: ForwardInfo;

}

| {

/** typically, `transfer` */

sourcePort: string;

sourceChannel: IBCChannelID;

token: Coin;

receiver: string;

};

export type TransferRoute = {

/** typically, `transfer` */

sourcePort: string;

sourceChannel: IBCChannelID;

token: Coin;

} & (

| {

receiver: typeof PFM_RECEIVER;

/** contains PFM forwarding info */

forwardInfo: ForwardInfo;

}

| {

receiver: string;

});

dckc · 2024-11-27T22:10:13Z

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

+      if (!chainInfos.has(holdingChainName)) {
+        throw makeError(
+          `chain info not found for holding chain: ${q(holdingChainName)}`,
+        );
+      }


The usual idiom is:

Suggested change

if (!chainInfos.has(holdingChainName)) {

throw makeError(

`chain info not found for holding chain: ${q(holdingChainName)}`,

);

}

chainInfos.has(holdingChainName) || Fail`chain info not found for holding chain: ${q(holdingChainName)}`;

Is this one of those places where the type checker doesn't grok?

Done. There is one condition (getAsset can return undefined) but I casted

dckc

onward...

Co-authored-by: Dan Connolly <[email protected]>

- json structure of pfm memo

- also, dont iterate over connections if there arent any

- chainHub helper to return .transfer details for ibc transfer that can have multiple hops (via pfm)

refs: #10445 ## Description - leverages `ChainHub.makeTransferRoute` to support PFM routing in `LocalOrchAccount.transfer()` - updates test harness to supply chain and asset info for `chainHub` in exo and contract tests - refactors send-anywhere `contract-upgrade.test.ts` to no longer rely on a buggy `agoricNames` - updates several orchestration examples contracts to seed their own `chainHub` to facilitate boot and multichain testing ### Security Considerations No new considerations from these changes. ### Scaling Considerations No new considerations from these changes. ### Documentation Considerations None ### Testing Considerations Includes unit tests. See #10584 for more robust testing of the pfm route logic. Existing `multichain-testing` will cover potential regressions (single-hop) transfers. Multi-hop tests are forthcoming. ### Upgrade Considerations N/A, library code. Part of NPM Orch or FUSDC release.

closes: #9966 closes: #10445 ## Description Adds multi-hop (PFM) scenarios to the `examples/send-anywhere.contract.js` multichain (e2e) test. To support this change, this PR also includes: - a proposal for registering interchain assets in vbank (closes #9966). aims for production quality but is only used in tests - a `fundFaucet` helper in `multichain-testing` so developers can request ATOM, OSMO, etc in `provisionSmartWallet` - a `GoDuration` type in `@agoric/orchestration` that captures basic Go [time duration strings](https://pkg.go.dev/time#ParseDuration) and an update to `DefaultPfmTimeoutOpts` (10min -> 10m) ### Security Considerations `@agoric/builders/scripts/testing/register-interchain-bank-assets.js` allows callers overwrite assets in `vbank` and `agoricNames`. It's only intended for testing, and shouldn't be used in production. A production version might guard against accidental overrides. ### Scaling Considerations None, mostly test code. Adds a little CI time to `multichain-testing` for the extra CoreEval. ### Documentation Considerations None ### Testing Considerations Includes an E2E to test in `multichain-testing` that leverages `register-interchain-bank-assets.js`. Also includes the first E2E test of PFM functionality added in #10584 and #10571. ### Upgrade Considerations None, library code an NPM orch or FUSDC release.

0xpatrickdev marked this pull request as ready for review November 27, 2024 19:32

0xpatrickdev requested a review from a team as a code owner November 27, 2024 19:32

0xpatrickdev requested review from AgoricTriage, turadg, dckc and samsiegart November 27, 2024 19:32

0xpatrickdev mentioned this pull request Nov 27, 2024

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

Open

0xpatrickdev commented Nov 27, 2024

View reviewed changes

packages/orchestration/test/exos/chain-hub.test.ts Outdated Show resolved Hide resolved

dckc reviewed Nov 27, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/chainhub-get-transfer-route branch 2 times, most recently from 649df1c to 4bd202a Compare November 27, 2024 20:59

0xpatrickdev requested a review from dckc November 27, 2024 21:01

0xpatrickdev changed the title ~~feat: chainHub.getTransferRoute()~~ feat: chainHub.makeTransferRoute() Nov 27, 2024

0xpatrickdev force-pushed the pc/chainhub-get-transfer-route branch from 4bd202a to ab62bd1 Compare November 27, 2024 21:18

dckc reviewed Nov 27, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/chainhub-get-transfer-route branch 2 times, most recently from f01678c to 542c043 Compare November 27, 2024 22:56

dckc approved these changes Nov 27, 2024

View reviewed changes

samsiegart approved these changes Nov 27, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/chainhub-get-transfer-route branch from 542c043 to f1d83a1 Compare November 27, 2024 23:53

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

0xpatrickdev force-pushed the pc/chainhub-get-transfer-route branch from f1d83a1 to cd59676 Compare November 28, 2024 00:17

0xpatrickdev and others added 5 commits November 28, 2024 00:35

feat: assetOn DenomDetail helper

05fe515

Co-authored-by: Dan Connolly <[email protected]>

types: ForwardInfo

a6c1e9e

- json structure of pfm memo

fix: brandKey not part of DenomDetail

9a65478

- also, dont iterate over connections if there arent any

feat: chainHub.makeTransferRoute

0215b6f

- chainHub helper to return .transfer details for ibc transfer that can have multiple hops (via pfm)

chore: include ForwardInfoShape and CoinShape in snapshots

1717b2a

0xpatrickdev force-pushed the pc/chainhub-get-transfer-route branch from cd59676 to 1717b2a Compare November 28, 2024 00:35

mergify bot merged commit 17503a9 into master Nov 28, 2024
81 checks passed

mergify bot deleted the pc/chainhub-get-transfer-route branch November 28, 2024 01:18

This was referenced Nov 28, 2024

feat: local-orch-account .transfer() supports pfm routes #10571

Merged

test: send-anywhere pfm scenarios #10591

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `chainHub.makeTransferRoute()` #10584

feat: `chainHub.makeTransferRoute()` #10584

0xpatrickdev commented Nov 27, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 27, 2024 •

edited

Loading

dckc left a comment

dckc Nov 27, 2024

dckc Nov 27, 2024

dckc Nov 27, 2024

0xpatrickdev Nov 27, 2024

dckc Nov 27, 2024

dckc Nov 27, 2024

dckc Nov 27, 2024

0xpatrickdev Nov 27, 2024 •

edited

Loading

dckc Nov 27, 2024

0xpatrickdev Nov 27, 2024

dckc Nov 27, 2024

dckc Nov 27, 2024

dckc Nov 27, 2024

0xpatrickdev Nov 27, 2024

dckc Nov 27, 2024

0xpatrickdev Nov 27, 2024

dckc Nov 27, 2024

dckc Nov 27, 2024

dckc Nov 27, 2024

0xpatrickdev Nov 27, 2024

dckc Nov 27, 2024

0xpatrickdev Nov 27, 2024

dckc left a comment

	* @property {ForwardInfo} [forwardInfo] - contains pfm forwarding info
	* @property {ForwardInfo} [forwardInfo] - contains PFM forwarding info

	* @property {string} sourcePort
	* @property {string} sourcePort typically 'transfer'

feat: chainHub.makeTransferRoute() #10584

feat: chainHub.makeTransferRoute() #10584

Conversation

0xpatrickdev commented Nov 27, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Nov 27, 2024 • edited Loading

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

0xpatrickdev Nov 27, 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

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

dckc left a comment

Choose a reason for hiding this comment

feat: `chainHub.makeTransferRoute()` #10584

feat: `chainHub.makeTransferRoute()` #10584

0xpatrickdev commented Nov 27, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 27, 2024 •

edited

Loading

0xpatrickdev Nov 27, 2024 •

edited

Loading