9063 getChain #9376
9063 getChain #9376
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
a8f6c6c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c2be34bf.agoric-sdk.pages.dev |
| Branch Preview URL: | https://9063-getchain.agoric-sdk.pages.dev |
refs: #9063 ## Description Extracted from #9376 ### Security Considerations <!-- Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls? --> ### Scaling Considerations <!-- Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated? --> ### Documentation Considerations <!-- Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users? --> ### Testing Considerations <!-- Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet? --> ### Upgrade Considerations <!-- What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed? -->
closes: #9207 ## Description While working on #9376 I needed some tests to check how the pieces fit together. None of them actually make progress on #9063 that that branch points to so I'm submitting them under this other PR. The goal of this PR is just to provide unit tests and some cleanup of the app code. It still has a lot of fake stuff. ### Security Considerations <!-- Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls? --> ### Scaling Considerations <!-- Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated? --> ### Documentation Considerations <!-- Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users? --> ### Testing Considerations <!-- Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet? --> ### Upgrade Considerations never yet deployed
turadg
force-pushed
the
9063-getChain
branch
2 times, most recently
from
df15efd
to
17e2072
Compare
leading 'boot' was sharing prefix with `bootstrapTests`
turadg
force-pushed
the
9063-getChain
branch
2 times, most recently
from
c54c889
to
87fff8c
Compare
| state: IBCChannelState.STATE_OPEN, // XXX presumably | ||
| version: channel.version, | ||
| }, | ||
| versions: [], // XXX where to read? |
There was a problem hiding this comment.
@0xpatrickdev prototyped code to get this stuff from the protocol
#8879 (comment)
#8879 (comment)
If we're not getting the info from the protocol, then it's not clear that we should use this structure.
There was a problem hiding this comment.
I think we should remove this field until there's a story that requires it.
Making example data more visible for the links comments:
{
"identifier": "1", // seems like it'd only come into play when there's a v2 of `ibc`.
"features": [
"ORDER_ORDERED", // doesn't seem like something needed at run-time
"ORDER_UNORDERED"
]
}
| import type { CosmosChainInfo } from '../src/cosmos-api.js'; | ||
|
|
||
| // XXX script assumes it's run from the package path | ||
| // XXX .json would be more apt but Endo bundler can't import that |
There was a problem hiding this comment.
.json module support is at least in progress; it's not available yet?
There was a problem hiding this comment.
It didn't work and the issue is open,
| @@ -0,0 +1,47 @@ | |||
| #!/usr/bin/env tsx | |||
There was a problem hiding this comment.
Doesn't the chain-registry package include an already-fetched copy of this stuff?
There was a problem hiding this comment.
ok... I see... we're not adding the whole kitchen sink dependency. Just the library function to fetch the bulk data.
| @@ -0,0 +1,90 @@ | |||
| export default { | |||
There was a problem hiding this comment.
add a "generated by yarn codegen; edits may be clobbered" notice / warning?
| }, | ||
| const knownChains = /** @satisfies {Record<string, ChainInfo>} */ ( | ||
| harden({ | ||
| ...fetchedChainInfo, |
There was a problem hiding this comment.
the fetchedChainInfo seems to be mainnet info; should the agoric chainId change to agoric-3 likewise?
How does it work with local starship-style testing? or in an instagoric-style test network?
There was a problem hiding this comment.
It doesn't yet.
Do you think it should before this lands?
There was a problem hiding this comment.
My intuition is that treating it as build-time now and then changing it to be runtime later will ripple out to lots of callers, an expensive form of churn. So yes, I suppose addressing it now is my preference. But since #9063 is assigned to you, I'm OK if you choose a different route to get there.
In fact, it's not clear that instagoric-style networks are in scope of #9063.
|
|
||
| test('chain-info', async t => { | ||
| const { agoricNames, agoricNamesAdmin } = await makeAgoricNamesAccess(t.log); | ||
| await registerChainNamespace(agoricNamesAdmin, t.log); |
There was a problem hiding this comment.
again, I don't see how this registration can be build time, using mainnet info. I think we need to support test networks. Seems like all but the canonical transfer channel info needs to come from agd query ... or the like.
There was a problem hiding this comment.
chain-registry provides info for mainnet, testnet and devnet. So we can support test networks using that data source.
But I don't see that requirement as in scope for this PR.
| @@ -34,7 +34,6 @@ const knownChains = /** @satisfies {Record<string, ChainInfo>} */ ( | |||
| version: 'ics20-1', | |||
| }, | |||
| versions: [{ identifier: '', features: ['', ''] }], | |||
| delay_period: 0n, | |||
There was a problem hiding this comment.
how is delay_period not like the others? I don't see anything that makes it less essential
There was a problem hiding this comment.
I couldn't find a data source for it and I couldn't find a consumer of it
| 'agoriclocal', | ||
| 'cosmoshub-4', |
There was a problem hiding this comment.
I think these need to come in via config (the arg after powers) so that this can work in a starship config as well as mainnet, instagoric, etc.
Threading this sort of thing all the way thru the script and the getManifest call and such is tedious, but it seems necessary here.
There was a problem hiding this comment.
agree but I'm going to punt on Instagoric and Starship requirements
| state: IBCChannelState.STATE_OPEN, // XXX presumably | ||
| version: channel.version, | ||
| }, | ||
| versions: [], // XXX where to read? |
There was a problem hiding this comment.
I think we should remove this field until there's a story that requires it.
Making example data more visible for the links comments:
{
"identifier": "1", // seems like it'd only come into play when there's a v2 of `ibc`.
"features": [
"ORDER_ORDERED", // doesn't seem like something needed at run-time
"ORDER_UNORDERED"
]
}
| @@ -39,23 +38,26 @@ export type CosmosValidatorAddress = ChainAddress & { | |||
|
|
|||
| /** Represents an IBC Connection between two chains, which can contain multiple Channels. */ | |||
| export type IBCConnectionInfo = { | |||
| id: IBCConnectionID; // e.g. connection-0 | |||
| // XXX really IBCConnectionID but our chain info fetcher doesn't know | |||
There was a problem hiding this comment.
I didn't say we can't. The reason I didn't is that it would require a TypeScript AST generator. This PR uses JSON.stringify which does the job.
| /** @type {StartUpgradableOpts<StakeAtomSF>} */ | ||
| const startOpts = { | ||
| label: 'stakeAtom', | ||
| installation: stakeAtom, | ||
| installation: stakeIca, |
refs: #9063 closes: #8879 ## Description Fast follow on #9376 Tests that uncovered that the JSON.stringify encoding is not a valid vstorage key. There's one character that can be used as a separator `_`, the only vstorage valid char that's not a chain-id valid char. (vstorage chars are superset of chain id chars) Also fixes the chain_id half of the connection key, which had been chain name. Also adds a cache to chain info writer so it doesn't write entries that exist and haven't changed. Some misc test supports as well. ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations Eventually we'll document these new vstorage nodes, but probably by just pointing to some snapshots. ### Testing Considerations The A3P test checks that it works through agd CLI. ### Upgrade Considerations not yet released
refs: #9063
Description
More progress on,
Security Considerations
Sources chain network data from chain-registry.
Scaling Considerations
Writes each chain info node or edge to one vstorage node.
Documentation Considerations
none
Testing Considerations
We'll also have to solve making it work in Starship but I think that's a concern for #8896 or a new ticket.
Upgrade Considerations
Not yet released. When we do want to update the chain info in agoricNames we'll need new work.