-
Notifications
You must be signed in to change notification settings - Fork 206
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
getConnectionInfo by obj (or string) #9494
Conversation
Deploying agoric-sdk with Cloudflare Pages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels more ocappy. Do we have any considerations or concerns around or increased message sizes? I guess it's all in the same vat so not a concern?
ordering: 1 /* Order.ORDER_UNORDERED */, | ||
state: 3 /* IBCConnectionState.STATE_OPEN */, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tangential to the PR - i think it would read better in vstorage if we used ORDER_UNORDERED
and STATE_OPEN
strings instead of integers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
Right. Any outstanding concerns? WDYT of the questions posed in the description? |
13e02f0
to
794991b
Compare
794991b
to
8acaef6
Compare
const [_, osmosis, connectionInfo] = await getChainsAndConnection( | ||
chainHub, | ||
'agoric', | ||
'osmosis', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition 👏 .
Noting that this doesn't help limit roundtrips for scenarios where we only have a chainId
, like here:
agoric-sdk/packages/orchestration/src/exos/local-chain-account-kit.js
Lines 241 to 245 in 8f1c7bd
const agoricChainInfo = await chainHub.getChainInfo('agoric'); | |
const { transferChannel } = await chainHub.getConnectionInfo( | |
agoricChainInfo.chainId, | |
destination.chainId, | |
); |
This is motivating factor for considering including chainName
on ChainAddress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, already a step ahead! #9498
* @template {string} K | ||
* @typedef {K extends keyof KnownChains | ||
* ? Omit<KnownChains[K], 'connections'> | ||
* : ChainInfo} ActualChainInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nb: not crazy about this name, but don't have any better suggestions
8acaef6
to
c99d58f
Compare
@@ -1,7 +1,7 @@ | |||
import { makeTracer } from '@agoric/internal'; | |||
import { makeStorageNodeChild } from '@agoric/internal/src/lib-chainStorage.js'; | |||
import { E } from '@endo/far'; | |||
import { makeChainHub } from '../utils/chainHub.js'; | |||
import { getChainsAndConnection, makeChainHub } from '../utils/chainHub.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking on this more, wrt to host code requirements for async-flow, it may make more sense to have this directly on makeChainHub
instead of a helper. NB for this PR but something to consider.
4b39f1f
to
7921211
Compare
7921211
to
f3f87b1
Compare
incidental
Description
It seemed a little silly that we always have to deference
.chainId
. This does that within thegetConnectionInfo
call.It also adds a
getChainsAndConnection
helper. It's outside of the ChainHub object to keep that surface minimal.Should we also do it in
registerConnection
?Have a helper to do it every chainId param?
Security Considerations
none
Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
Covered already
Upgrade Considerations
none