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(orchestration): add more chain infos for fusdc #10845

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samsiegart
Copy link
Contributor

@samsiegart samsiegart commented Jan 14, 2025

closes #10736
refs #10629

Description

  • Adds remaining cosmos chains for fusdc as listed in https://github.com/Agoric/product-tasks/issues/186#issuecomment-2442824919.
  • Adds icaEnabled property to CosmosChainInfo
  • Makes the connection infos in vstorage support chain IDs with underscores (_). Since we use underscores as a separator in the vstorage key, we sanitize the chain IDs by doubling the underscores (similar to double-quote in csv)
  • Don't use all the chain infos in bootstrap tests, only a subset, since CI can't handle the input size
  • Skip some more bootstrap tests in xs runtime because they fail with unsettled value for "kp2526" (or similar error)

Security Considerations

We're adding non-ica-enabled chains now, so #10629 is relevant.

Scaling Considerations

packages/orchestration/src/fetched-chain-info.js is quite large, hopefully not too large?

Documentation Considerations

Not sure if these chains need to be documented somewhere.

Testing Considerations

Just did yarn codegen. Not sure how to test this for real.

Upgrade Considerations

Updating the chain infos will presumably require a core-eval

Copy link

cloudflare-workers-and-pages bot commented Jan 15, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9f28f36
Status: ✅  Deploy successful!
Preview URL: https://15782099.agoric-sdk.pages.dev
Branch Preview URL: https://fu-chain-info.agoric-sdk.pages.dev

View logs

@samsiegart samsiegart added the force:integration Force integration tests to run on PR label Jan 15, 2025
@samsiegart samsiegart force-pushed the fu-chain-info branch 2 times, most recently from 24c3c4e to 513d5ba Compare January 15, 2025 09:24
@samsiegart
Copy link
Contributor Author

samsiegart commented Jan 15, 2025

Problem: When adding all the chain infos, it causes an E2BIG error in CI. This happens during the bootstrap test "send-anywhere", as evalProposal tries spawning a process with the very large chain info list as an input (see log).

It works on my computer, just not in CI. So, I wonder:

  • Can we bump the CI memory limit somehow?
  • Or only use a subset of the chain infos for this test?
  • Is there a risk that this won't work on-chain?

Edit:
Went with the option only use a subset of the chain infos for this test.

@samsiegart samsiegart force-pushed the fu-chain-info branch 3 times, most recently from 29e7ea1 to 766a6ba Compare January 15, 2025 19:51
@samsiegart
Copy link
Contributor Author

Finally got test passing. Had to make some decisions (see description) so please take a look @0xpatrickdev @turadg

@@ -331,7 +331,15 @@ test.serial('basic-flows', async t => {
'@agoric/builders/scripts/orchestration/init-basic-flows.js',
[
'--chainInfo',
JSON.stringify(withChainCapabilities(fetchedChainInfo)),
JSON.stringify(
withChainCapabilities({
Copy link
Member

Choose a reason for hiding this comment

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

this is repeated below. It would help legibility and maintainability if this value were named like fetchedChainInfo is. please extract to const like minimalSubsetChainInfo or what have you.

I see it's used in multiple modules so consider making a new one for named chain info subsets which tests and contracts could import from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just put this in tools for reuse

Comment on lines 390 to 391
// XXX for some reason the code after this when run under XS fails with:
// message: 'unsettled value for "kp2526"',
Copy link
Member

Choose a reason for hiding this comment

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

We have an issue now which suffices and has the advantage of an easy query for cleanup.

Suggested change
// XXX for some reason the code after this when run under XS fails with:
// message: 'unsettled value for "kp2526"',
// UNTIL https://github.com/Agoric/agoric-sdk/issues/10847

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just removed all this code because of cab3fbe

packages/boot/test/fast-usdc/fast-usdc.test.ts Outdated Show resolved Hide resolved
});

// chain info, assets and ibc data will be downloaded dynamically by invoking fetchUrls method
await client.fetchUrls();

const chainInfo = await convertChainInfo(client);
const icaChainSet = new Set(icaChainNames);
Copy link
Member

Choose a reason for hiding this comment

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

consider icaChainNames itself being a Set. there really is no ordering to them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 80 to 81
* {@link CHAIN_ID_SEPARATOR} in the chain ID to make it clear that it's part of
* the chain ID rather than a separator.
Copy link
Member

Choose a reason for hiding this comment

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

it's not just making it clear. it's making it a reversible encoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the wording to be more precise

Comment on lines +87 to +90
chainId.replaceAll(
CHAIN_ID_SEPARATOR,
`${CHAIN_ID_SEPARATOR}${CHAIN_ID_SEPARATOR}`,
);
Copy link
Member

Choose a reason for hiding this comment

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

on the fence about this suggestion, but since this expression is so close to the const I think it could be clearer:

Suggested change
chainId.replaceAll(
CHAIN_ID_SEPARATOR,
`${CHAIN_ID_SEPARATOR}${CHAIN_ID_SEPARATOR}`,
);
chainId.replaceAll('_', '__');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean but I prefer keeping the variable here

* @see {@link https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-2.md}
*/
const CHAIN_ID_SEPARATOR = '_';
export const sanitizeChainId = chainId =>
Copy link
Member

Choose a reason for hiding this comment

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

it's not sanitizing it. it's encoding it so it can appear in the tuple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the wording

@@ -95,6 +96,7 @@ export const convertChainInfo = async registry => {
stakingTokens: chain.staking?.staking_tokens,
// UNTIL https://github.com/Agoric/agoric-sdk/issues/9326
icqEnabled: chain.chain_name === 'osmosis',
icaEnabled: icaEnabled(chain.chain_name),
Copy link
Member

Choose a reason for hiding this comment

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

the same name is being used for a property and a query function. please rename the query function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

chains,
ibcData: ibc.data,
},
() => true,
Copy link
Member

Choose a reason for hiding this comment

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

this happens to be true but should it be assumed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might as well not assume. Just changed this to use the imported list of icaEnabled chains

Copy link
Member

@0xpatrickdev 0xpatrickdev left a comment

Choose a reason for hiding this comment

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

Can we add a test to FUSDC that ensures there's a channel between noble and all of the chains in the ticket? This would ensure chain-registry has all of the information we're after.

NB: in registry.js, the script prints "multiple preferred transfer channels" as a console.warn. It be worth auditing more closely at this time and raising if we find multiple valid channels for noble -> another remote chain.

@@ -12,7 +12,7 @@ const outputFile = 'src/fetched-chain-info.js';
/**
* Names for which to fetch info
*/
const chainNames = [
export const icaChainNames = new Set([
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for including this distinction.

I would suggest keeping a single list here and moving icaChains to chain-capabilities.js as IcaEnabled.

It's a bit of duplication, but in #10329 it was agreed it was best to keep non-chain registry info out of fetch-chain-info / fetched-chain-info.

Copy link
Member

Choose a reason for hiding this comment

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

I forgot about that. Thanks for catching it @0xpatrickdev .

});

// chain info, assets and ibc data will be downloaded dynamically by invoking fetchUrls method
await client.fetchUrls();

const chainInfo = await convertChainInfo(client);
const chainInfo = await convertChainInfo(client, name =>
icaChainNames.has(name),
Copy link
Member

Choose a reason for hiding this comment

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

Can we leave this part out for now? We should add icaEnabled via withChainCapabilities instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fusdc: chain and asset info for mainnet proposal
3 participants