Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: chainHub.makeTransferRoute() #10584

Merged
merged 5 commits into from
Nov 28, 2024
Merged

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Nov 27, 2024

refs: #10445
refs: #10006

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.

@0xpatrickdev 0xpatrickdev marked this pull request as ready for review November 27, 2024 19:32
@0xpatrickdev 0xpatrickdev requested a review from a team as a code owner November 27, 2024 19:32
Copy link

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

Deploying agoric-sdk with  Cloudflare Pages  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

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const TransferRouteShape = M.splitRecord(
{
sourcePort: M.string(),
sourceChannel: M.string(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Comment on lines 200 to 203
token: {
amount: M.string(),
denom: M.string(),
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* `memo` for `MsgTransfer`.
*
* @typedef {object} TransferRoute
* @property {string} sourcePort
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@0xpatrickdev 0xpatrickdev Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 518 to 520
if (!chainInfos.has(holdingChainName)) {
log(`chain info not found for holding chain: ${holdingChainName}`);
return undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Amended to throw.

receiver: destination.value,
port: issuerToDest.transferChannel.portId,
channel: issuerToDest.transferChannel.channelId,
...{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the extra nesting?

denom: denomAmount.denom,
},
/**
* purposely using invalid bech32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a TODO that references #10580

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assetOn is not exported... ok.

Comment on lines -48 to +54
const infoWithBrand = info.brandKey
? { ...info, brand: brands[info.brandKey] }
: info;
const { brandKey, ...rest } = info;
const infoWithBrand = brandKey
? { ...rest, brand: brands[brandKey] }
: rest;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

I meant to say something about that earlier.

@0xpatrickdev 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 0xpatrickdev requested a review from dckc November 27, 2024 21:01
@0xpatrickdev 0xpatrickdev changed the title feat: chainHub.getTransferRoute() feat: chainHub.makeTransferRoute() Nov 27, 2024
@0xpatrickdev 0xpatrickdev force-pushed the pc/chainhub-get-transfer-route branch from 4bd202a to ab62bd1 Compare November 27, 2024 21:18
Comment on lines 387 to 403
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;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Comment on lines 504 to 508
if (!chainInfos.has(holdingChainName)) {
throw makeError(
`chain info not found for holding chain: ${q(holdingChainName)}`,
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onward...

@0xpatrickdev 0xpatrickdev force-pushed the pc/chainhub-get-transfer-route branch from 542c043 to f1d83a1 Compare November 27, 2024 23:53
@0xpatrickdev 0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Nov 27, 2024
@0xpatrickdev 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
- 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)
@0xpatrickdev 0xpatrickdev force-pushed the pc/chainhub-get-transfer-route branch from cd59676 to 1717b2a Compare November 28, 2024 00:35
@mergify mergify bot merged commit 17503a9 into master Nov 28, 2024
81 checks passed
@mergify mergify bot deleted the pc/chainhub-get-transfer-route branch November 28, 2024 01:18
mergify bot added a commit that referenced this pull request Dec 4, 2024
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.
mergify bot added a commit that referenced this pull request Dec 4, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants