Skip to content
This repository has been archived by the owner on Dec 1, 2023. It is now read-only.

Proxy nile node unknown options to starknet-devnet #342

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Proxy nile node unknown options to starknet-devnet #342

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6c9ea02
feat: update node logic
ericnordelo Jan 18, 2023
cfd8ab8
feat: update docs
ericnordelo Jan 18, 2023
0a4bba6
feat: apply updates from review
ericnordelo Jan 19, 2023
e3abdf3
feat: add -h option
ericnordelo Jan 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions docs/modules/ROOT/pages/cli.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
:cairo-lang: link:https://github.com/starkware-libs/cairo-lang[cairo-lang]
:imports: link:https://starknet.io/docs/how_cairo_works/imports.html?highlight=cairo%20path#import-search-paths[Import search paths]
:status: link:#status[status]
:devnet-doc: link:https://shard-labs.github.io/starknet-devnet/docs/guide/run[starknet-devnet documentation]

= CLI Reference

Expand Down Expand Up @@ -287,6 +288,9 @@ Scaffold a simple Nile project.

Run a local {starknet-devnet} node.

NOTE: Accepts all the options that the starknet-devnet node accepts by default. Check the {devnet-doc} for a full
list of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a bit overkill. We're basically saying the same thing below with Starknet Devnet Options. I'm thinking we can remove the note here. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it, removed it, and added it again a couple of times thinking about the design. I'm not too comfortable 100 percent with either of the versions, because I wanted to let it as clear as possible for the user that he can use these arguments from starknet-devnet, but not adding all of them here. At the same time, I feel the user can check the command doc in a quick glance and miss this "Starknet Devnet Options".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with removing the note, but I still have the feeling the user can miss this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha yeah, I see the dilemma. If we remove host and port, there's much less noise around the node section. I think it's enough, no? Here's a crude example of how I'm seeing it:

node

----
nile node
----
Run a local [starknet-devnet](https://github.com/Shard-Labs/starknet-devnet/) node.

Starknet Devnet Options
This command exposes...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think that should be enough indeed.

===== Options

- `*--host*`
Expand All @@ -297,12 +301,10 @@ Defaults to 127.0.0.1 (use the address the program outputs on start).
- `*--port*`
+
Specify the port to listen at. Defaults to 5050.
- `*--seed*`
+
Specify the seed for randomness of accounts to be deployed.
- `*--lite-mode*`
+
Applies all lite-mode optimizations by disabling features such as block hash and deploy hash calculation.

===== Starknet Devnet Options
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved

- _This command exposes all the functionalities provided within the starknet-devnet node, like forking mainnet, or specifying the contract class to be used for predeployed accounts. Check the {devnet-doc} for a full description of the arguments._

=== `compile`

Expand Down
3 changes: 3 additions & 0 deletions docs/modules/ROOT/pages/index.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
:pyenv: link:https://github.com/OpenZeppelin/cairo-contracts/blob/release-v0.4.0b/src/openzeppelin/access/ownable/library.cairo[pyenv]
:devnet-doc: link:https://shard-labs.github.io/starknet-devnet/docs/guide/run[starknet-devnet documentation]

= Overview

Expand Down Expand Up @@ -117,6 +118,8 @@ Use `nile node` to run a local devnet node for development.
nile node
----

NOTE: This command exposes all the functionalities provided within the starknet-devnet node, like forking mainnet, or specifying the contract class to be used for predeployed accounts. Check the {devnet-doc} for a full description of the arguments.

==== Deploying an account

. Add an environment variable with the account's private key to the `.env` file.
Expand Down
38 changes: 19 additions & 19 deletions src/nile/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from nile.core.clean import clean as clean_command
from nile.core.compile import compile as compile_command
from nile.core.init import init as init_command
from nile.core.node import get_help_message
from nile.core.node import node as node_command
from nile.core.plugins import load_plugins
from nile.core.run import run as run_command
Expand Down Expand Up @@ -352,28 +353,27 @@ def clean(ctx):
clean_command()


@cli.command()
@click.option("--host", default="127.0.0.1")
@click.option("--port", default=5050)
@click.option("--seed", type=int)
@click.option("--lite_mode", is_flag=True)
@enable_stack_trace
def node(ctx, host, port, seed, lite_mode):
"""Start StarkNet local network.

$ nile node
Start StarkNet local network at port 5050
class NodeCommand(click.Command):
"""Command wrapper to override the default help message."""
Comment on lines +357 to +358
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


$ nile node --host HOST --port 5001
Start StarkNet network on address HOST listening at port 5001
def format_help(self, ctx, formatter):
"""Help message override."""
click.echo(get_help_message())

$ nile node --seed SEED
Start StarkNet local network with seed SEED

$ nile node --lite_mode
Start StarkNet network on lite-mode
"""
node_command(host, port, seed, lite_mode)
@cli.command(
context_settings=dict(
ignore_unknown_options=True,
),
cls=NodeCommand,
)
@click.option("--host", default="127.0.0.1")
@click.option("--port", default=5050)
@click.argument("node_args", nargs=-1, type=click.UNPROCESSED)
@enable_stack_trace
def node(ctx, host, port, node_args):
"""Start StarkNet local network."""
node_command(host, port, node_args)


@cli.command()
Expand Down
29 changes: 21 additions & 8 deletions src/nile/core/node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from nile.common import DEFAULT_GATEWAYS, write_node_json


def node(host="127.0.0.1", port=5050, seed=None, lite_mode=False):
def node(host="127.0.0.1", port=5050, node_args=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since host and port are passed to the devnet in the the same way as the other devnet options (and the defaults match), can't we just stick with node_args as a single catch-all param?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to let the defaults in our control, but if they match I agree it makes sense to remove them.

Copy link
Member Author

@ericnordelo ericnordelo Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought. I will let the host and port options as a commodity if you agree because we are using them here:

        if host == "127.0.0.1":
            network = "localhost"
        else:
            network = host
        gateway_url = f"http://{host}:{port}/"
        if DEFAULT_GATEWAYS.get(network) != gateway_url:
            write_node_json(network, gateway_url)

And if we remove them, I would need to parse the node_args to get the values anyway (is easier to catch them separately as we are currently doing).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh right! Yeah, leaving them in sounds good to me. Depending on how the Nile config turns out down the road, maybe we can revisit this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

"""Start StarkNet local network."""
try:
# Save host and port information to be used by other commands
Expand All @@ -19,13 +19,8 @@ def node(host="127.0.0.1", port=5050, seed=None, lite_mode=False):
write_node_json(network, gateway_url)

command = ["starknet-devnet", "--host", host, "--port", str(port)]

if seed is not None:
command.append("--seed")
command.append(str(seed))

if lite_mode:
command.append("--lite-mode")
if node_args is not None:
command += list(node_args)

# Start network
subprocess.check_call(command)
Expand All @@ -35,3 +30,21 @@ def node(host="127.0.0.1", port=5050, seed=None, lite_mode=False):
"\n\n😰 Could not find starknet-devnet, is it installed? Try with:\n"
" pip install starknet-devnet"
)


def get_help_message():
"""Retrieve and parse the help message from starknet-devnet."""
base = """
Start StarkNet local network.

$ nile node
Start StarkNet local network at port 5050

Options:
"""

raw_message = subprocess.check_output(["starknet-devnet", "--help"]).decode("utf-8")
options_index = raw_message.find("optional arguments:")
options = raw_message[options_index + 19 :]

return base + options
49 changes: 32 additions & 17 deletions tests/commands/test_node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@

import pytest

from nile.core.node import node
from nile.core.node import get_help_message, node

HOST = "127.0.0.1"
PORT = "5050"
SEED = "123"
LITE = "--lite-mode"
HELP_OUTPUT = b"optional arguments:\n options"


@pytest.fixture(autouse=True)
Expand All @@ -19,29 +19,44 @@ def tmp_working_dir(monkeypatch, tmp_path):
return tmp_path


@pytest.mark.parametrize("host", [HOST])
@pytest.mark.parametrize("port", [PORT])
@pytest.mark.parametrize(
"args",
"node_args",
[
({}),
({"seed": SEED}),
({"lite_mode": LITE}),
({"host": HOST, "port": PORT}),
({"host": HOST, "port": PORT, "seed": SEED, "lite_mode": LITE}),
None,
("--seed", SEED),
("--lite-mode"),
("--fork-network", "alpha-mainnet"),
("--fork-block", 4),
],
)
@patch("nile.core.node.subprocess.check_call")
def test_node_call(mock_subprocess, args):
node(**args)

command = ["starknet-devnet", "--host", HOST, "--port", PORT]
if "seed" in args:
command.append("--seed")
command.append(SEED)
if "lite_mode" in args:
command.append(LITE)
def test_node_call(mock_subprocess, host, port, node_args):
node(host, port, node_args)

command = ["starknet-devnet", "--host", host, "--port", port]
if node_args is not None:
command += list(node_args)
mock_subprocess.assert_called_once_with(command)


@patch("nile.core.node.subprocess.check_output", return_value=HELP_OUTPUT)
def test_node_help_message(mock_subprocess):
base = """
Start StarkNet local network.

$ nile node
Start StarkNet local network at port 5050

Options:
"""

help_message = get_help_message()

assert help_message == base + "\n options"


@patch("nile.core.node.subprocess.check_call")
def test_node_error(mock_subprocess, caplog):
logging.getLogger().setLevel(logging.INFO)
Expand Down