-
Notifications
You must be signed in to change notification settings - Fork 37
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
Random port on integrated devnet #202
base: master
Are you sure you want to change the base?
Conversation
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.
Should we update any of the tests for this?
@@ -18,6 +18,7 @@ export class DockerDevnet extends DockerServer { | |||
} | |||
|
|||
protected async getContainerArgs(): Promise<string[]> { | |||
return this.devnetArgs || []; | |||
const containerArgs = this.devnetArgs || []; |
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.
What happens if the user provided --port
in args
in hardhat config?
hre.config.networks.integratedDevnet.url = networkUrl; | ||
const { hostname, port } = new URL(hre.config.starknet.networkUrl); | ||
const isPortFree = await isFreePort(parseInt(port)); | ||
if (!isPortFree) { |
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.
Why this check? Couldn't it surprise the user if we just silently change the specified port. If the port is occupied, integrated-devnet spawner will inform the user about that, right?
My understanding was that we should find a free port only if the user didn't provide a URL? Now I'm actually thinking, is it even possible for the user to omit the URL? Can the type extensions file be modified to allow that?
As already discussed, this doesn't have to be a part of release-0.7.0 if we keep URL as a config option. Currently waiting for response from hardhat: NomicFoundation/hardhat#3221 |
New issue to followup on hardhat that could resolve this. NomicFoundation/hardhat#3323 |
Usage related changes
Development related changes
Checklist:
test
directory (with a test case consisting ofnetwork.json
,hardhat.config.ts
,check.sh
)plugin
branch ofstarknet-hardhat-example
:test.sh
to use my example repo branchtest.sh
to to use the original branch (after the example repo PR has been merged)