test: `send-anywhere` pfm scenarios #10591

0xpatrickdev · 2024-12-01T21:01:45Z

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 well-known interchain denoms should be registered as vbank assets in multichain-testing environment #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 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.

- to facililate registering the same denom across multiple chains in `ChainHub` - denoms are unique to a chain, but not all chains

- since denoms are only unique to a chain and all chains, require `holdingChainName` parameter for asset info lookups

- shape used for pfm ForwardInfo options

- provide `chainInfo` and `assetInfo` in commonPrivateArgs for contracts to use - register common assets used in testing - provide `populateChainHub` function for use in exo unit testing in favor of `registerAgoricBld`

this test can no longer rely on the IST brand being available, so we - added checks to ensure brands are accepted for `.transfer` and `.send` - filed #10449 since this surfaced a bug in `amountToCoin` - use Moolah issuer for "no denom for brand" failure path tests

- enables sending offers with brands

- chainInfo and assetInfo are provided as `commonPrivateArgs`. include them in `ContractMeta`

- auto-stake-it initializes `chainHub` with data so existing tests that use `.transfer()` pass

- no longer needed since we call `registerChainsAndAssets`

- no longer relies on buggy agoricNames - continues to test async-flow resumability and cross-chain vow settlement across upgrade Co-authored-by: 0xPatrick <[email protected]>

- test/bootstrapTests/orchestration.test.ts used a mixed of test and test.serial - the test without `.serial` interleaved and was confusing to debug - this mainly updates inline snapshots, which return different account and channel identifiers since all tests in this file share the same context

- basic-flows.contract.js is provided with `chainInfo` and `assetInfo` in `privateArgs` via builder options - needed for tests that use `localAccount.transfer()`, now reliant on asset info, to pass

cloudflare-workers-and-pages · 2024-12-02T00:05:46Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`fc4e81a`
Status:	✅ Deploy successful!
Preview URL:	https://9fbd8ebf.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-register-interchain-bank.agoric-sdk.pages.dev

View logs

- create `register-interchain-bank-assets.js` for multichain-testing environment - allow users to submit offers using new brands like OSMO, ION - override existing brands like ATOM (ibc/toyatom) with starship denom - create `make-bank-asset-info.ts` to gather `InterchainAssetOptions` from starship env - update `make start` and ci `multichain-e2e-template.yaml` to call these scripts - deploy-cli.ts `deployBuilder` accepts `builderOpts`

- adds helper to fund agoric faucet with interchain tokens - allows callers to request `OSMO`, `ATOM`, etc, via `provisionSmartWallet`

dckc

looks good.

the comments here are mostly thinking-out-loud.

Putting register-interchain-bank-assets.js under packages/builders/scripts seems pretty odd; it's not a script. So I hope you'll move that. But I can still get behind this if you don't.

dckc · 2024-12-02T20:41:43Z

multichain-testing/Makefile

+register-bank-assets:
+	node_modules/.bin/tsx scripts/fetch-starship-chain-info.ts && \
+	node_modules/.bin/tsx scripts/deploy-cli.ts src/register-interchain-bank-assets.builder.js \
+		assets="$$(node_modules/.bin/tsx scripts/make-bank-asset-info.ts)"
+


what does it cost to get the scripts to know how to execute themselves?

Suggested change

register-bank-assets:

node_modules/.bin/tsx scripts/fetch-starship-chain-info.ts && \

node_modules/.bin/tsx scripts/deploy-cli.ts src/register-interchain-bank-assets.builder.js \

assets="$$(node_modules/.bin/tsx scripts/make-bank-asset-info.ts)"

register-bank-assets:

./scripts/fetch-starship-chain-info.ts && \

./scripts/deploy-cli.ts src/register-interchain-bank-assets.builder.js \

assets="$$(./scripts/make-bank-asset-info.ts)"

I'll see where we land in #10605 and incorporate that approach

dckc · 2024-12-02T20:44:37Z

multichain-testing/Makefile

@@ -101,5 +106,5 @@ wait-for-pods:
 	node_modules/.bin/tsx scripts/pod-readiness.ts

 .PHONY: start
-start: install wait-for-pods port-forward fund-provision-pool override-chain-registry
+start: install wait-for-pods port-forward fund-provision-pool override-chain-registry register-bank-assets


Does deploy-cli.ts know how to run multiple evals in one chain governance proposal? How about combining override-chain-registry and register-bank-assets?

Good idea. I won't tackle that here but I'll hope to make this update soon

dckc · 2024-12-02T20:48:59Z

multichain-testing/scripts/make-bank-asset-info.ts

+const main = () => {
+  if (!starshipChainInfo) {
+    throw new Error(
+      'starshipChainInfo not found. run `make override-chain-registry` or `node_modules/.bin/tsx scripts/fetch-starship-chain-info.ts` first.',


the node_modules/.bin/ again jumps out at me... it looks like we're peeking into the internals of something.

Suggested change

'starshipChainInfo not found. run `make override-chain-registry` or `node_modules/.bin/tsx scripts/fetch-starship-chain-info.ts` first.',

'starshipChainInfo not found. run `make override-chain-registry` or `./scripts/fetch-starship-chain-info.ts` first.',

dckc · 2024-12-02T20:57:09Z

multichain-testing/tools/faucet-tools.ts

+      const fundAndTransfer = makeFundAndTransfer(
+        t,
+        retryUntilCondition,
+        useChain,
+      );


hoist this above return?

dckc · 2024-12-02T21:17:41Z

packages/builders/scripts/testing/register-interchain-bank-assets.js

@@ -0,0 +1,190 @@
+/**
+ * @file register-interchain-bank-assets.js


This doesn't look like a builder script. It looks like on-chain core-eval code.

I adjusted the @file comment to mention this. Are you suggesting this should live in a different directory?

dckc · 2024-12-02T21:40:19Z

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

@@ -179,7 +179,7 @@ const ChainIdArgShape = M.or(
 const DefaultPfmTimeoutOpts = harden(
  /** @type {const} */ ({
    retries: 3,
-    timeout: '10min',
+    timeout: /** @type {GoDuration} */ ('10m'),


odd... @type {const} isn't enough?

Good catch const should be sufficient here

dckc · 2024-12-02T21:42:18Z

packages/orchestration/src/proposals/start-send-anywhere.js

@@ -82,13 +81,32 @@ export const startSendAnywhere = async (
    }),
  );

+  const safeLookup = async p =>


the name safeLookup suggests that the .lookup(...) call should be in this function. safeFulfill maybe?

dckc · 2024-12-02T21:46:12Z

multichain-testing/src/register-interchain-bank-assets.builder.js

+  const parseAssets = () => {
+    if (typeof assets !== 'string') {
+      throw Error(
+        'must provide --assets=JSON.stringify({ denom: Denom; keyword: string; decimalPlaces: number; }[])',


Is this really a keyword? Or is it a misnomer where we really mean issuerName?

Suggested change

'must provide --assets=JSON.stringify({ denom: Denom; keyword: string; decimalPlaces: number; }[])',

'must provide --assets=JSON.stringify({ denom: Denom; issuerName: string; decimalPlaces: number; }[])',

I remember this comment in the add-vault proposal. I'll look to make the corresponding change in the builder so we don't continue to carry the tech debt

dckc · 2024-12-02T22:30:50Z

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

-test.serial(sendAnywhereScenario, 'cosmoshub', 1);
-test.serial(sendAnywhereScenario, 'cosmoshub', 2);
+test.serial(sendAnywhereScenario, 'osmosis', 1, 'IST');
+test.serial(sendAnywhereScenario, 'osmosis', 2, 'ATOM'); // exercises PFM (agoric -> cosmoshub -> osmosis)


it took me a while to grok what this test does, but I think I get it now.

For ref:
https://github.com/Agoric/agoric-sdk/actions/runs/12123427233/job/33798929261?pr=10591#step:13:3141

dckc · 2024-12-02T22:34:53Z

multichain-testing/tools/ibc-transfer.ts

@@ -118,14 +119,14 @@ export const makeIBCTransferMsg = (
 };

 export const createFundedWalletAndClient = async (
-  t: ExecutionContext,
+  log: (...args: unknown[]) => void,
  chainName: string,
  useChain: MultichainRegistry['useChain'],
 ) => {
  const { chain, creditFromFaucet, getRpcEndpoint } = useChain(chainName);
  const wallet = await createWallet(chain.bech32_prefix);


createWallet(...) seems to generate random key material using ambient authority.

ah... ok; it takes key material as a default arg, a la...

transitional approach to OCap Discipline #10038

I'd rather see this test take advantage of that and pass in the key material, but I suppose we'll get there in due course.

Agree, but none of these tests rely on a specific key currently. For now, I've made mnemonic an optional parameter to createFundedWalletAndClient.

0xpatrickdev and others added 21 commits November 29, 2024 16:38

feat: assetInfo as array of entries

8ec70a0

- to facililate registering the same denom across multiple chains in `ChainHub` - denoms are unique to a chain, but not all chains

feat!: getAsset and getDenomInfo require holdingChainName param

fec9ebc

- since denoms are only unique to a chain and all chains, require `holdingChainName` parameter for asset info lookups

feat: multichain-testing makeAssetInfo helper

c12d868

feat: ForwardOptsShape

0f58076

- shape used for pfm ForwardInfo options

feat(local-orchestration-account): support multi-hop pfm transfers

8e84ee5

test: chainHub test support

e5a6c15

- provide `chainInfo` and `assetInfo` in commonPrivateArgs for contracts to use - register common assets used in testing - provide `populateChainHub` function for use in exo unit testing in favor of `registerAgoricBld`

timestamp durable

ab2cf5a

feat: send-anywhere includes IST and BLD brands

8f8120a

- enables sending offers with brands

chore: update send-anywhere baggage snapshot

dac0dc6

feat: auto-stake-it calls registerChainsAndAssets

f4fa95a

feat: swap.contract.js allows chainInfo and assetInfo

416f146

- chainInfo and assetInfo are provided as `commonPrivateArgs`. include them in `ContractMeta`

test(boot): restart send-anywhere includes asset info

e503e50

feat: auto-stake-it.contract.js inits chainHub

fdcda70

- auto-stake-it initializes `chainHub` with data so existing tests that use `.transfer()` pass

chore(fusdc): remove write-chain-info.js

f240dd5

- no longer needed since we call `registerChainsAndAssets`

test: send-anywhere contract upgrade

307700a

- no longer relies on buggy agoricNames - continues to test async-flow resumability and cross-chain vow settlement across upgrade Co-authored-by: 0xPatrick <[email protected]>

test(send-anywhere): explicitly check result key is undefined

75319ad

feat: basic-flows.contract.js seeds chainHub

9f21c28

- basic-flows.contract.js is provided with `chainInfo` and `assetInfo` in `privateArgs` via builder options - needed for tests that use `localAccount.transfer()`, now reliant on asset info, to pass

test(fast-usdc): support pfm transfers

d89a727

test: deposit-withdraw requires asset info

6c5db7f

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

0xpatrickdev force-pushed the pc/register-interchain-bank-assets branch from 5b9326d to 46e6875 Compare December 2, 2024 00:05

0xpatrickdev added 4 commits December 2, 2024 11:13

chore: pfm timeout is Go time duration string

a3d4993

test: faucetTools.fundFaucet

a70c763

- adds helper to fund agoric faucet with interchain tokens - allows callers to request `OSMO`, `ATOM`, etc, via `provisionSmartWallet`

test: send-anywhere pfm scenarios

fc4e81a

0xpatrickdev force-pushed the pc/register-interchain-bank-assets branch from 46e6875 to fc4e81a Compare December 2, 2024 16:13

0xpatrickdev marked this pull request as ready for review December 2, 2024 16:15

0xpatrickdev requested a review from a team as a code owner December 2, 2024 16:15

0xpatrickdev requested review from AgoricTriage and dckc December 2, 2024 16:15

dckc approved these changes Dec 2, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/10445-transfer-pfm branch 4 times, most recently from 2368fb7 to 94e4b37 Compare December 3, 2024 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: `send-anywhere` pfm scenarios #10591

test: `send-anywhere` pfm scenarios #10591

0xpatrickdev commented Dec 1, 2024 •

edited

Loading

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

edited

Loading

dckc left a comment

dckc Dec 2, 2024

0xpatrickdev Dec 2, 2024

dckc Dec 2, 2024

0xpatrickdev Dec 2, 2024

dckc Dec 2, 2024

dckc Dec 2, 2024

dckc Dec 2, 2024

0xpatrickdev Dec 3, 2024

dckc Dec 2, 2024

0xpatrickdev Dec 2, 2024

dckc Dec 2, 2024

dckc Dec 2, 2024

0xpatrickdev Dec 2, 2024

dckc Dec 2, 2024

dckc Dec 2, 2024

0xpatrickdev Dec 3, 2024

	'starshipChainInfo not found. run `make override-chain-registry` or `node_modules/.bin/tsx scripts/fetch-starship-chain-info.ts` first.',
	'starshipChainInfo not found. run `make override-chain-registry` or `./scripts/fetch-starship-chain-info.ts` first.',

		@@ -0,0 +1,190 @@
		/**
		* @file register-interchain-bank-assets.js

	'must provide --assets=JSON.stringify({ denom: Denom; keyword: string; decimalPlaces: number; }[])',
	'must provide --assets=JSON.stringify({ denom: Denom; issuerName: string; decimalPlaces: number; }[])',

test: send-anywhere pfm scenarios #10591

Are you sure you want to change the base?

test: send-anywhere pfm scenarios #10591

Conversation

0xpatrickdev commented Dec 1, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

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

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

test: `send-anywhere` pfm scenarios #10591

test: `send-anywhere` pfm scenarios #10591

0xpatrickdev commented Dec 1, 2024 •

edited

Loading

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

edited

Loading