IBC types and refactoring #9191
Conversation
There was a problem hiding this comment.
Thanks! This PR brings to light many of the subtle details of the IBC+network implementation. I really appreciate this effort, since it will result in better clarity.
I just took a quick look right now, but will drop some comments here and return to it later.
packages/vats/src/ibc.js
Outdated
| async onListen(_port, localAddr, _listenHandler) { | ||
| /** | ||
| * @param {Port} _port | ||
| * @param {LocalIbcAddress} localAddr |
There was a problem hiding this comment.
Implementation signatures of these callbacks, like onListen's localAddr parameter, must be compatible with their interface signatures, specified in packages/network/src/types.js. IBC is just the first of several possible supported network implementations.
packages/vats/tools/ibc-utils.js
Outdated
| export const REMOTE_ADDR_RE = | ||
| /^(\/ibc-hop\/[^/]+)*\/ibc-port\/([^/]+)\/(ordered|unordered)\/([^/]+)$/s; | ||
| harden(REMOTE_ADDR_RE); | ||
| /** @typedef {`/ibc-hop/${string}/ibc-port/${string}/{'ordered'|'unordered'}/${string}`} RemoteIbcAddress */ |
There was a problem hiding this comment.
This is a valiant attempt at expressing the type of an IBC multiaddr, whose basic format is described in agoric-sdk/packages/network/src/multiaddr.js.
The REMOTE_ADDR_RE correctly specifies a remote address, but unfortunately, RemoteIbcAddress looks like it disallows certain details of the RE's structure, namely:
- zero or more
/ibc-hop/${nonSlashString}prefixes, and - all but the final
${string}cannot contain a slash.
Also, there's a simple typo (missing $ in ${'ordered'|'unordered'}).
| ) => { | ||
| const ibcHops = hops.map(hop => `/ibc-hop/${hop}`).join('/'); | ||
| return /** @type {RemoteIbcAddress} */ ( | ||
| `${ibcHops}/ibc-port/${rPortID}/${order.toLowerCase()}/${rVersion}/ibc-channel/${rChannelID}` |
There was a problem hiding this comment.
Do we want to distinguish between requested and negotiated (active) address? In the beginning of the channel lifecycle, I believe we'll want to omit /ibc-channel/${rChannelID}
There was a problem hiding this comment.
Bumping this comment. It is maybe out of scope for the PR, but if we had encodeRequestedRemoteIbcAddress it could be used in makeICAChannelAddress in orchestration/src/utils/address.js.
Impartial to what you call these functions, but i think "requested" and "negotiated" are good keywords to use to distinguish between the behavior.
There was a problem hiding this comment.
I don't have a well enough informed opinion on this yet. Open to it in a future PR
| }; | ||
| harden(encodeRemoteIbcAddress); | ||
|
|
||
| export const encodeLocalIbcAddress = (portID, order, version) => { |
There was a problem hiding this comment.
I think we might be able to add types for portID and rPortID. Maybe something like:
type LocalPortID = `icacontroller-${number}` | `icqcontroller-${number}`;
type RemotePortID = 'transfer' | 'icahost' | 'icqhost';There was a problem hiding this comment.
I disagree here. That would be overspecification, which is a pitfall that ibc-go made when first specifying ICA. The names of ports (Identifiers) should be as general as what the IBC spec allows, not a restricted set of "what we think people will use".
There was a problem hiding this comment.
Good feedback, thank you. Maybe these definitions should instead live in orchestration and extend the more generalized specification in here?
packages/vats/tools/ibc-utils.js
Outdated
| const match = remoteAddr.match( | ||
| /^(\/ibc-hop\/[^/]+)*\/ibc-port\/([^/]+)\/(ordered|unordered)\/([^/]+)$/s, | ||
| ); |
There was a problem hiding this comment.
Why not:
| const match = remoteAddr.match( | |
| /^(\/ibc-hop\/[^/]+)*\/ibc-port\/([^/]+)\/(ordered|unordered)\/([^/]+)$/s, | |
| ); | |
| const match = remoteAddr.match(REMOTE_ADDR_RE); |
If that's not readable enough, we could switch to using named capturing groups
| * @typedef {object} Watcher | ||
| * @property {(value: T) => Vow<TResult1> | PromiseVow<TResult1> | TResult1} [onFulfilled] | ||
| * @property {(value: T, context?: C) => Vow<TResult1> | PromiseVow<TResult1> | TResult1} [onFulfilled] |
|
@turadg - This PR appears to be stuck. It's had a merge label for > 24 hours |
Co-Authored-By: Michael FIG <[email protected]>
refs: #9042
Description
In the course of reviewing #9114 it wasn't clear what the
/ibc-hopencoding spec was. This PR separates anibc-utils.jsto try to encapsulate that. @iomekam @michaelfig can you provide some additional documentation on its design?This also adds types I used to understand the code, dropping
anybindings from 338 to 86.One type change was in
vowsto add the context param.Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations