Move port allocation in network vat to PortAllocator #9228
Conversation
| @@ -168,6 +168,7 @@ export const getManifestForNetwork = (_powers, { networkRef, ibcRef }) => ({ | |||
| zoe: 'zoe', | |||
| provisioning: 'provisioning', | |||
| vatUpgradeInfo: true, | |||
| portAllocator: 'portAllocator', | |||
| @@ -168,6 +168,7 @@ export const getManifestForNetwork = (_powers, { networkRef, ibcRef }) => ({ | |||
| zoe: 'zoe', | |||
| provisioning: 'provisioning', | |||
| vatUpgradeInfo: true, | |||
| portAllocator: 'portAllocator', | |||
There was a problem hiding this comment.
Should this go in produce instead of consume? Then in vat-orchestration, we can consume portAllocator instead of networkVat?
Deploying agoric-sdk with
|
| Latest commit: |
aa5e919
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4437662c.agoric-sdk.pages.dev |
| Branch Preview URL: | https://iomekam-port-allocation.agoric-sdk.pages.dev |
iomekam
force-pushed
the
iomekam-port-allocation
branch
from
f4c84ec
to
787fb5b
Compare
iomekam
changed the title
iomekam
changed the title
There was a problem hiding this comment.
The single-character portId (the 1 in /ibc-port/1) needs to be fixed.
Maybe my hopes for a solution to vanity port names vs. squatting can't be satisfied. Please spend a little time thinking about that, but if there are no clear solutions, let's rip out the Pegasus special case and have it listen on just a generated allocateIBCPort instead.
| const portAllocator = makePortAllocator({ protocol }); | ||
|
|
||
| const ibcPort = await when(portAllocator.allocateIBCPort()); | ||
| t.is(ibcPort.getLocalAddress(), '/ibc-port/1'); |
There was a problem hiding this comment.
This doesn't look like the correct result (the IBC standard states that port identifiers need to be at least 2 characters in length). The local address should be /ibc-port/port-1. Maybe a port- prefix got dropped somewhere in the PRs?
There was a problem hiding this comment.
We actually mock the generatePortID method, and in that implementation, we drop the port prefix. I've updated it to be inline with what we do in production
| const port = await E(networkVat).bindPort('/ibc-port/pegasus'); | ||
| const port = await E(portAllocator).allocateIBCPegasusPort(); |
There was a problem hiding this comment.
This makes me somewhat uncomfortable. It seems to illustrate that allocateIBCPegasusPort() is too specific and doesn't really solve the need for custom port names. Is there a solution that exists between the power of bindPort() and the rigidity of allocateIBCPegasusPort()?
| * - loopback ports: `E(networkVat).bindPort('/local/')` | ||
| * - an echo port: `E(vats.network).bindPort('/ibc-port/echo')` | ||
| * - loopback ports: `E(portAllocator).allocateLocalPort()` | ||
| * - an echo port: `E(portAllocator).allocateIBCPort()` |
There was a problem hiding this comment.
This doesn't produce /ibc-port/echo, so it loses the port identity that the outside world could rely on (like how ICS-20 is usually found at /ibc-port/transfer).
| export type AttenuatedPortAllocator = Pick< | ||
| PortAllocator, | ||
| 'allocateICAControllerPort' | ||
| >; |
There was a problem hiding this comment.
I think we can get rid of this typedef (and Far('PortAllocator', ...) in orchestration-proposal.js) and just use PortAllocator directly. Feel free to leave as is and we can make this change in the course of #9198 where we'll need to add allocateICQControllerPort.
| @@ -152,7 +151,7 @@ export const setupNetworkProtocols = async ( | |||
| await registerNetworkProtocols(vats, dibcBridgeManager); | |||
|
|
|||
| // Add an echo listener on our ibc-port network (whether real or virtual). | |||
| const echoPort = await when(E(vats.network).bindPort('/ibc-port/echo')); | |||
| const echoPort = await when(E(allocator).allocateIBCPort()); | |||
There was a problem hiding this comment.
This looks like it may be a change in behavior - /ibc-port/echo -> /ibc-port/1 - is this intentional/expected?
There was a problem hiding this comment.
Intentional in that it fits with the design, but not intentional in that it breaks the ability to bind to a known /ibc-port/echo port. Trying to iterate on a solution where we can still provision this port without having access to bindPort
Co-authored-by: Michael FIG <[email protected]>
…mekam-port-allocation
closes: #9165
Description
bindPortgives a caller too much power to specify exactly what port they want allocated. With this change, we create a central object,PortAllocatorthat handles handing out ports to the caller. As of today, we are allocating ports for:/ibc-port/ibc-port/icacontroller/ibc-port/pegasusSecurity Considerations
This will prevent
squatting, where callers can come and claim certain ports that they have no association withDocumentation Considerations
We've removed the
bindPortmethod from the network vat. All the contracts inside ofagoric-sdkhave been changed to useportAllocatorTesting Considerations
I've included some unit test testing the new allocate APIs