Proxy nile node unknown options to starknet-devnet#342
Proxy nile node unknown options to starknet-devnet#342ericnordelo wants to merge 4 commits intoOpenZeppelin:mainfrom
nile node unknown options to starknet-devnet#342Conversation
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Great work, Eric! I left a couple questions :)
docs/modules/ROOT/pages/cli.adoc
Outdated
| NOTE: Accepts all the options that the starknet-devnet node accepts by default. Check the {devnet-doc} for a full | ||
| list of them. | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
I'm ok with removing the note, but I still have the feeling the user can miss this.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Thanks. I think that should be enough indeed.
|
|
||
|
|
||
| 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): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I just wanted to let the defaults in our control, but if they match I agree it makes sense to remove them.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| class NodeCommand(click.Command): | ||
| """Command wrapper to override the default help message.""" |
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Looks good to go!
2 participants
Fixes #293
This PR proposes to make available to the
nile nodeinterface all the options thatstraknet-devnetaccepts by default.This enables forking, but also the ability to specify the initial balances, the number of predeployed accounts, the gas price to be used, the account class to be used, etc...
Ex:
Previous lite mode flag was labeled
--lite_mode, and now it must be named--lite-mode, so this is a small breaking change.