chore: fusdc multichain support #10626

0xpatrickdev · 2024-12-04T22:25:23Z

refs: #10597

Description

Primary changes:

@agoric/orchestration: exposes intermediateRecipient: ChainAddress['value'] in ForwardOpts, part of IBCMsgTransferOpts, allowing callers to provide their own value. Currently defaults to "pfm"
@agoric/fast-usdc: creates a Noble ICA in the StartFn so we have an address value for intermediateRecipient. Noble's chain does not permit non-bech32 values.

Ancillary:

@agoric/boot's bridgeUtils helpers:
- setAckBehavior helper to control whether a IBCDowncallMethod acknowledgments are queued or inbounded immediately
- setBech32Prefix helper to control address prefix for ICA accounts
- ICA addresses are unique per account
@agoric/fast-usdc: remove blockTimestamp from CctpTxEvidence

Security Considerations

No new considerations

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Updated existing tests.

Upgrade Considerations

N/A, library code part of FUSDC or NPM Orch release

0xpatrickdev · 2024-12-04T22:26:11Z

packages/orchestration/src/exos/remote-chain-facade.js

-            const stakingDenom = remoteChainInfo.stakingTokens?.[0]?.denom;
-            if (!stakingDenom) throw Fail`chain info lacks staking denom`;


I can't recall why this was a check. It doesn't seem we rely on stakingDenom (anymore?)

I'm struggling to understand what this test is trying to achieve / how we can replace it:
https://github.com/Agoric/agoric-sdk/blob/fc802adc06082eb0618f4a2d58d91ac380512352/packages/orchestration/test/facade-durability.test.ts#L83_L122

IIRC having a stakingDenom was a proxy for some other guarantee about the chain, but I don't remember what that is and it seems like it's not necessary (if it ever was) because the other tests are passing.

I think I'm now remembering it was a proxy for which chains can make an ica / have staking actions.

It reminds me to create a ticket for CosmosChainInfo['icaEnabled'] (this is true for all current chains in KnownChains). This is where we'd perform the check. Edit: #10629

For something like delegate: never, we can continue rely on the CosmosChainAccountMethods type that looks for stakingTokens.

It seems to date back to...

9281 durable orchestrator #9532

I used git log --reverse -S'chain info lacks staking denom' -- src/exos/remote-chain-facade.js to find the 1st version it appeared in.

cloudflare-workers-and-pages · 2024-12-04T22:29:23Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`1847618`
Status:	✅ Deploy successful!
Preview URL:	https://94aac3dd.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-fusdc-multichain-support.agoric-sdk.pages.dev

View logs

turadg · 2024-12-04T23:02:17Z

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

replying to the commit message… maybe the flags tooling in agd-tools should be more flexible?

also, typo 'arugment'

I gave this a shot and came up short. Would welcome someone else picking it up.

Relatedly - excited/hopeful to start using @agoric/client-utils in multichain-testing.

turadg · 2024-12-04T23:03:07Z

packages/orchestration/src/cosmos-api.ts

@@ -334,6 +334,8 @@ export interface IBCMsgTransferOptions {
  timeoutTimestamp?: MsgTransfer['timeoutTimestamp'];
  memo?: string;
  forwardOpts?: {
+    /** The recipient address for the intermediate transfer. Defaults to 'pfm' unless specified */


nice forward and backward compatible solution

turadg · 2024-12-04T23:08:57Z

packages/orchestration/src/exos/remote-chain-facade.js

-            const stakingDenom = remoteChainInfo.stakingTokens?.[0]?.denom;
-            if (!stakingDenom) throw Fail`chain info lacks staking denom`;


IIRC having a stakingDenom was a proxy for some other guarantee about the chain, but I don't remember what that is and it seems like it's not necessary (if it ever was) because the other tests are passing.

packages/boot/tools/supports.ts

dckc

I need to understand better what intermediateRecipient is used for.

None of the rest of the comments are important/critical.

dckc · 2024-12-04T22:53:13Z

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

@@ -236,7 +237,7 @@ export default async (homeP, endowments) => {
  // @ts-expect-error ensured by options
  const {
    values: {
-      oracle: oracleArgs,
+      oracles: oracleArgs,


the oracleArgs alias no longer seems appropriate; it's a single arg now.

dckc · 2024-12-04T22:59:39Z

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

-const oraclesUsage = 'use --oracle name:address ...';
+const oraclesUsage = 'use --oracles "["name:address","name:address"]"';


This seems like a UX regression, and the motivation seems to be an internal testing API. Seems like the tail wagging the dog. But oh well. The script has a limited userbase too.

Meanwhile, if we're using JSON on the command line, why not get rid of the name:address syntax while we're at it? How about --oracles '{"name":"address", ...}'.

Removed this commit from the PR

dckc · 2024-12-04T23:01:30Z

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

@@ -266,7 +267,7 @@ export default async (homeP, endowments) => {
    }
    if (!oracleArgs) throw Error(oraclesUsage);
    return Object.fromEntries(
-      oracleArgs.map(arg => {
+      JSON.parse(oracleArgs).map(arg => {


as noted above if we're using JSON.parse, we might as well get rid of the regex and Object.fromEntries.

dckc · 2024-12-04T23:06:22Z

packages/fast-usdc/src/exos/advancer.js

@@ -116,6 +116,7 @@ export const prepareAdvancerKit = (
     *   notifyFacet: import('./settler.js').SettlerKit['notify'];
     *   borrowerFacet: LiquidityPoolKit['borrower'];
     *   poolAccount: HostInterface<OrchestrationAccount<{chainId: 'agoric'}>>;
+     *   intermediateRecipient: ChainAddress;


we seem to throw away all but the .value without checking it. Just use a string?

better to check it, but even without checking it there's utility in knowing it's not just a string but a ChainAddress

dckc · 2024-12-04T23:08:32Z

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

-  /** @type {HostInterface<OrchestrationAccount<{chainId: 'agoric';}>>[]} */
-  const [poolAccount, settlementAccount] = await vowTools.when(
-    vowTools.all([poolAccountV, settleAccountV]),
+  /** @type {[HostInterface<OrchestrationAccount<{chainId: 'noble-1';}>>, HostInterface<OrchestrationAccount<{chainId: 'agoric';}>>, HostInterface<OrchestrationAccount<{chainId: 'agoric';}>>]} */


chainId is noble-1 but not agoric-3?

This is some tech debt that would hopefully be solved in #9992. agoric is a special case in the types currently since checks do ~startsWith (to support agoriclocal and agoric-3).

Agree this is confusing, updated to use agoric-3 here.

dckc · 2024-12-04T23:14:23Z

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

@@ -280,6 +285,11 @@ test('slow path: forward to EUD; remove pending tx', async t => {
        value: 'osmo183dejcnmkka5dzcu9xw6mywq0p2m5peks28men',
      },
      usdc.units(150),
+      {
+        forwardOpts: {
+          intermediateRecipient: 'noble1test',


Using the same property name for a string here and a ChainAddress elsewhere is hurting my tiny brain.

Agree, in hindsight this was a poor decision. Updated everything to use the ChainAddress object.

dckc · 2024-12-04T23:22:52Z

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

@@ -690,6 +690,7 @@ export const prepareLocalOrchestrationAccountKit = (
              'agoric',
              forwardOpts,
            );
+            trace('got transfer route', q(route).toString());


using .toString() with q(...) looks odd somehow. hm.

dckc · 2024-12-04T23:41:45Z

packages/orchestration/src/exos/remote-chain-facade.js

-            const stakingDenom = remoteChainInfo.stakingTokens?.[0]?.denom;
-            if (!stakingDenom) throw Fail`chain info lacks staking denom`;


It seems to date back to...

9281 durable orchestrator #9532

I used git log --reverse -S'chain info lacks staking denom' -- src/exos/remote-chain-facade.js to find the 1st version it appeared in.

dckc · 2024-12-04T23:48:18Z

packages/orchestration/src/typeGuards.js

@@ -245,6 +245,7 @@ export const ForwardOptsShape = M.splitRecord(
  {
    timeout: M.string(),
    retries: M.number(),
+    intermediateRecipient: M.string(),


again, my brain goes: not ChainAddressShape?
I can see that it's correct, but using the same name for 2 things is hurting my brain.

dckc · 2024-12-04T23:55:55Z

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

+  trace('poolAccount', poolAccount);
+  trace('nobleAccount', nobleAccount);
+
+  const intermediateRecipient = await vowTools.when(


This address... it's not used for anything other than bech32 checksum validation, right?

How about using bech32.encode('agoric', Array(20).fill(0)) aka agoric1qqqqqqqqqqqqqqqqqqqqhlgkpj?

This address... it's not used for anything other than bech32 checksum validation, right?

My answer to "How about using... ?" completely depends on the answer to the above (which I suspect is used for something other than checksum validation). I don't know the answer to the above.

- for some chains, "pfm" is not a valid `recipient` for PFM transfers - this change allows callers to provide a value

turadg · 2024-12-05T22:04:51Z

packages/boot/tools/supports.ts

+  /** inbound messages are delivered immediately */
+  Immediate: 'IMMEDIATE',
+} as const;
+type AckBehaviorType = (typeof AckBehavior)[keyof typeof AckBehavior];


We have a way to make the type name be the same as the value name in JSDoc. In .ts one would usually do enum. I'm not sure how to do it in .ts without enum.

turadg · 2024-12-05T22:08:06Z

packages/boot/tools/supports.ts

+            shouldAckImmediately(bridgeId, 'startChannelOpenInit')
+              ? inbound(BridgeId.DIBC, message)
+              : pushInbound(BridgeId.DIBC, message);


ternaries are for expressions, not control flow.

Suggested change

shouldAckImmediately(bridgeId, 'startChannelOpenInit')

? inbound(BridgeId.DIBC, message)

: pushInbound(BridgeId.DIBC, message);

const handle = shouldAckImmediately(bridgeId, 'startChannelOpenInit') ? inbound : pushInbound;

handle(BridgeId.DIBC, message);

- adds helper method that allows callers to change the way certain bridges, like DIBC, handle acknowledgements - defaults to QUEUED, where callers need to explicity use `flushInboundQueue`. adds the ability to set to IMMEDIATE, where bridge messages are immediately acknowledged on receipt

- noble only accepts bech32 addresses for the recipient field in ibc transfers - this change creates an ICA noble to facilitate the requirement - if an IBC transfer fails, we expect it to return the origin (agoric `localOrchAccount`), not the `intermediateRecipient`

- adds ability to use something besides "cosmos" for mock ICA address prefix

mergify · 2024-12-06T00:07:31Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ integration-test-result
☑️ linear-history

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

0xpatrickdev · 2024-12-06T00:10:01Z

@mergify requeue

mergify · 2024-12-06T00:10:06Z

requeue

☑️ This pull request is already queued

0xpatrickdev commented Dec 4, 2024

View reviewed changes

0xpatrickdev marked this pull request as ready for review December 4, 2024 22:35

0xpatrickdev requested a review from a team as a code owner December 4, 2024 22:35

0xpatrickdev requested review from AgoricTriage, turadg and dckc December 4, 2024 22:35

turadg reviewed Dec 4, 2024

View reviewed changes

dckc reviewed Dec 4, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/fusdc-multichain-support branch from d58d038 to 2614509 Compare December 5, 2024 00:31

0xpatrickdev added 2 commits December 5, 2024 12:11

feat: ForwardOpts accepts intermediateRecipient

eb975f1

- for some chains, "pfm" is not a valid `recipient` for PFM transfers - this change allows callers to provide a value

chore: improve trace logs

24bd0cf

0xpatrickdev force-pushed the pc/fusdc-multichain-support branch from 2614509 to 75ce4a9 Compare December 5, 2024 17:11

0xpatrickdev added 2 commits December 5, 2024 12:13

chore: remove blockTimestamp from CctpTxEvidence

80d279e

chore: cosmos-orch-account does not require staking denom

9664aaa

0xpatrickdev force-pushed the pc/fusdc-multichain-support branch 2 times, most recently from 42ac99a to be4b63b Compare December 5, 2024 20:41

0xpatrickdev requested review from dckc and turadg December 5, 2024 20:52

turadg approved these changes Dec 5, 2024

View reviewed changes

0xpatrickdev added 4 commits December 5, 2024 18:17

chore: bridgeUtils.setBech32Prefix

64a6dce

- adds ability to use something besides "cosmos" for mock ICA address prefix

chore: bridge supports unique address per ICA

1847618

0xpatrickdev force-pushed the pc/fusdc-multichain-support branch from be4b63b to 1847618 Compare December 5, 2024 23:20

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Dec 5, 2024

mergify bot merged commit ef9088c into master Dec 6, 2024
91 checks passed

mergify bot deleted the pc/fusdc-multichain-support branch December 6, 2024 00:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fusdc multichain support #10626

chore: fusdc multichain support #10626

0xpatrickdev commented Dec 4, 2024 •

edited

Loading

0xpatrickdev Dec 4, 2024

0xpatrickdev Dec 4, 2024

turadg Dec 4, 2024

0xpatrickdev Dec 4, 2024 •

edited

Loading

dckc Dec 4, 2024

cloudflare-workers-and-pages bot commented Dec 4, 2024 •

edited

Loading

turadg Dec 4, 2024

0xpatrickdev Dec 4, 2024

turadg Dec 4, 2024

turadg Dec 4, 2024

dckc left a comment

dckc Dec 4, 2024

dckc Dec 4, 2024

0xpatrickdev Dec 5, 2024

dckc Dec 4, 2024

dckc Dec 4, 2024

turadg Dec 5, 2024

dckc Dec 4, 2024

0xpatrickdev Dec 5, 2024

dckc Dec 4, 2024

0xpatrickdev Dec 5, 2024

dckc Dec 4, 2024

dckc Dec 4, 2024

dckc Dec 4, 2024

0xpatrickdev Dec 5, 2024

dckc Dec 4, 2024

michaelfig Dec 5, 2024

turadg Dec 5, 2024

turadg Dec 5, 2024

mergify bot commented Dec 6, 2024

0xpatrickdev commented Dec 6, 2024

mergify bot commented Dec 6, 2024

		const stakingDenom = remoteChainInfo.stakingTokens?.[0]?.denom;
		if (!stakingDenom) throw Fail`chain info lacks staking denom`;

		const oraclesUsage = 'use --oracle name:address ...';
		const oraclesUsage = 'use --oracles "["name:address","name:address"]"';

chore: fusdc multichain support #10626

chore: fusdc multichain support #10626

Conversation

0xpatrickdev commented Dec 4, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Dec 4, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Dec 6, 2024

0xpatrickdev commented Dec 6, 2024

mergify bot commented Dec 6, 2024

☑️ This pull request is already queued

0xpatrickdev commented Dec 4, 2024 •

edited

Loading

0xpatrickdev Dec 4, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 4, 2024 •

edited

Loading