types!: Chain takes KnownChainName generic parameter instead of ChainInfo
#10213
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
a75f59e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0a85f71d.agoric-sdk.pages.dev |
| Branch Preview URL: | https://9992-chain-generic.agoric-sdk.pages.dev |
|
0xpatrickdev
force-pushed
the
9992-chain-generic
branch
from
f591f2c to
a75f59e
Compare
| @@ -126,6 +126,7 @@ const prepareOrchestratorKit = ( | |||
| return asVow(() => chainByName.get(name)); | |||
| } | |||
| if (name === 'agoric') { | |||
| // @ts-expect-error types of property 'chainId' are incompatible (string vs a chainId const) | |||
There was a problem hiding this comment.
Wasn't sure on this one. Is it addressable?
There was a problem hiding this comment.
This is because the runtime supports more than KnownChains. A bunch of the places that were changed from string to KnownChainName I think should stay string and things that really expect a KnownChainName can look in knownChains to figure out whether it is or isn't.
The alternative is that we make developers do some build step in their local env to define KnownChainName that's different than the one in agoric-sdk. Let's punt on that because we can expect agoric-sdk to know all the chains pretty quickly. We can push NPM releases pretty fast. And in a pinch they could patch the package.
Though considering that… maybe we should just have a type ChainName and it default to all the known ones but someone can override it with another they know.
Let's discuss
| // "makeAccount" suggests an operation within a vat | ||
| /** | ||
| * Creates a new account on the remote chain. | ||
| * @returns an object that controls a new remote account on Chain | ||
| */ | ||
| makeAccount: () => Promise<OrchestrationAccount<CI>>; | ||
| makeAccount: () => Promise<OrchestrationAccount<C>>; | ||
| // FUTURE supply optional port object; also fetch port object | ||
|
|
||
| query: CI extends { icqEnabled: true } | ||
| ? ICQQueryFunction | ||
| : CI['chainId'] extends `agoric${string}` | ||
| ? QueryManyFn | ||
| query: C extends 'agoric' | ||
| ? QueryManyFn | ||
| : KnownChains[C] extends { icqEnabled: true } | ||
| ? ICQQueryFunction | ||
| : never; | ||
|
|
||
| // TODO provide a way to get the local denom/brand/whatever for this chain |
There was a problem hiding this comment.
Can we delete these or create issues? They seem like noise at this point
// "makeAccount" suggests an operation within a vat
// FUTURE supply optional port object; also fetch port object
// TODO provide a way to get the local denom/brand/whatever for this chain
There was a problem hiding this comment.
agree, issues or delete. Please figure out whether they're still relevant (if so file, if not delete)
| @@ -126,6 +126,7 @@ const prepareOrchestratorKit = ( | |||
| return asVow(() => chainByName.get(name)); | |||
| } | |||
| if (name === 'agoric') { | |||
| // @ts-expect-error types of property 'chainId' are incompatible (string vs a chainId const) | |||
There was a problem hiding this comment.
This is because the runtime supports more than KnownChains. A bunch of the places that were changed from string to KnownChainName I think should stay string and things that really expect a KnownChainName can look in knownChains to figure out whether it is or isn't.
The alternative is that we make developers do some build step in their local env to define KnownChainName that's different than the one in agoric-sdk. Let's punt on that because we can expect agoric-sdk to know all the chains pretty quickly. We can push NPM releases pretty fast. And in a pinch they could patch the package.
Though considering that… maybe we should just have a type ChainName and it default to all the known ones but someone can override it with another they know.
Let's discuss
| // "makeAccount" suggests an operation within a vat | ||
| /** | ||
| * Creates a new account on the remote chain. | ||
| * @returns an object that controls a new remote account on Chain | ||
| */ | ||
| makeAccount: () => Promise<OrchestrationAccount<CI>>; | ||
| makeAccount: () => Promise<OrchestrationAccount<C>>; | ||
| // FUTURE supply optional port object; also fetch port object | ||
|
|
||
| query: CI extends { icqEnabled: true } | ||
| ? ICQQueryFunction | ||
| : CI['chainId'] extends `agoric${string}` | ||
| ? QueryManyFn | ||
| query: C extends 'agoric' | ||
| ? QueryManyFn | ||
| : KnownChains[C] extends { icqEnabled: true } | ||
| ? ICQQueryFunction | ||
| : never; | ||
|
|
||
| // TODO provide a way to get the local denom/brand/whatever for this chain |
There was a problem hiding this comment.
agree, issues or delete. Please figure out whether they're still relevant (if so file, if not delete)
closes: #9992
Description
Changes generic parameter for
ChainfromChainInfotoKnownChainName. TheKnownChainNametype iskeyof KnownChains, the fetched-chain-info from cosmos chain-registry.Security Considerations
n/a
Scaling Considerations
n/a, types
Documentation Considerations
Helps make typedoc docs more readable.
Testing Considerations
Updates existing type tests.
Upgrade Considerations
library code for an npm orch release