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

test: send-anywhere pfm scenarios #10591

Merged
merged 4 commits into from
Dec 4, 2024
Merged

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Dec 1, 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:

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 0xpatrickdev added the force:integration Force integration tests to run on PR label Dec 1, 2024
@0xpatrickdev 0xpatrickdev force-pushed the pc/register-interchain-bank-assets branch from 5b9326d to 46e6875 Compare December 2, 2024 00:05
Copy link

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

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 748883d
Status: ✅  Deploy successful!
Preview URL: https://70c6cb23.agoric-sdk.pages.dev
Branch Preview URL: https://pc-register-interchain-bank.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev force-pushed the pc/register-interchain-bank-assets branch from 46e6875 to fc4e81a Compare December 2, 2024 16:13
@0xpatrickdev 0xpatrickdev marked this pull request as ready for review December 2, 2024 16:15
@0xpatrickdev 0xpatrickdev requested a review from a team as a code owner December 2, 2024 16:15
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.

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.

Comment on lines 82 to 86
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)"

Copy link
Member

Choose a reason for hiding this comment

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

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

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'll see where we land in #10605 and incorporate that approach

Copy link
Member

Choose a reason for hiding this comment

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

yeah; don't worry about it for this PR

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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.',
Copy link
Member

Choose a reason for hiding this comment

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

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.',

Comment on lines 33 to 37
const fundAndTransfer = makeFundAndTransfer(
t,
retryUntilCondition,
useChain,
);
Copy link
Member

Choose a reason for hiding this comment

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

hoist this above return?

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

Choose a reason for hiding this comment

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

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

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 adjusted the @file comment to mention this. Are you suggesting this should live in a different directory?

Copy link
Member

Choose a reason for hiding this comment

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

yes. it doesn't seem to belong in builder/scripts/

not a hill I'd die on.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch const should be sufficient here

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

const safeLookup = async p =>
Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@0xpatrickdev 0xpatrickdev force-pushed the pc/10445-transfer-pfm branch 4 times, most recently from 2368fb7 to 94e4b37 Compare December 3, 2024 18:59
@0xpatrickdev 0xpatrickdev force-pushed the pc/register-interchain-bank-assets branch from fc4e81a to 410f064 Compare December 3, 2024 21:55
@0xpatrickdev 0xpatrickdev force-pushed the pc/10445-transfer-pfm branch from a1132f4 to a706d28 Compare December 3, 2024 22:34
@0xpatrickdev 0xpatrickdev force-pushed the pc/register-interchain-bank-assets branch 2 times, most recently from 095b0dd to d1ee50a Compare December 3, 2024 22:38
@0xpatrickdev 0xpatrickdev force-pushed the pc/10445-transfer-pfm branch from a706d28 to a2d491f Compare December 3, 2024 23:43
@0xpatrickdev 0xpatrickdev force-pushed the pc/register-interchain-bank-assets branch from d1ee50a to a117976 Compare December 4, 2024 00:17
@0xpatrickdev 0xpatrickdev removed the force:integration Force integration tests to run on PR label Dec 4, 2024
Base automatically changed from pc/10445-transfer-pfm to master December 4, 2024 00:27
Copy link

github-actions bot commented Dec 4, 2024

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

@0xpatrickdev 0xpatrickdev force-pushed the pc/register-interchain-bank-assets branch from a117976 to 3c55dbb Compare December 4, 2024 00:28
@0xpatrickdev 0xpatrickdev added force:integration Force integration tests to run on PR automerge:rebase Automatically rebase updates, then merge labels Dec 4, 2024
@0xpatrickdev 0xpatrickdev force-pushed the pc/register-interchain-bank-assets branch 3 times, most recently from 788cb28 to 4399100 Compare December 4, 2024 02:32
- 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`
@0xpatrickdev 0xpatrickdev force-pushed the pc/register-interchain-bank-assets branch from 4399100 to 748883d Compare December 4, 2024 02:52
@mergify mergify bot merged commit cf1d435 into master Dec 4, 2024
81 checks passed
@mergify mergify bot deleted the pc/register-interchain-bank-assets branch December 4, 2024 03:38
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 force:integration Force integration tests to run on PR
Projects
None yet
2 participants