Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Move port allocation in network vat to PortAllocator #9228
Move port allocation in network vat to PortAllocator #9228
Changes from 10 commits
837c632
84b4fd4
787fb5b
1dc5cef
4a12122
38bcf1f
6cb9010
f9d8ce1
f7a4fac
988253b
36b520f
fce3edb
96b8508
252a503
7e69bed
a7993a8
4a64a8a
aa5e919
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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 aport-
prefix got dropped somewhere in the PRs?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.
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 productionThere 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.
I think we can get rid of this typedef (and
Far('PortAllocator', ...)
inorchestration-proposal.js
) and just usePortAllocator
directly. Feel free to leave as is and we can make this change in the course of #9198 where we'll need to addallocateICQControllerPort
.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 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 ofbindPort()
and the rigidity ofallocateIBCPegasusPort()
?