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

Fix network handling #303

Merged
merged 15 commits into from
Dec 16, 2022
Merged
36 changes: 23 additions & 13 deletions src/nile/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,25 +28,35 @@
# subject to change
"0x041a78e741e5af2fec34b695679bc6891742439f7afb8484ecd7766661ad02bf"
)
DEFAULT_GATEWAYS = {
"localhost": "http://127.0.0.1:5050/",
"goerli2": "https://alpha4-2.starknet.io",
"integration": "https://external.integration.starknet.io",
}


def get_gateways():
"""Get the StarkNet node details."""
try:
if os.path.exists(NODE_FILENAME):
with open(NODE_FILENAME, "r") as f:
gateway = json.load(f)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
gateway = json.load(f)
custom_gateways = json.load(f)

The name is a bit confusing being possible to have multiple custom gateways in the node.json file.

return gateway

except FileNotFoundError:
with open(NODE_FILENAME, "w") as f:
networks = {
Copy link
Member

Choose a reason for hiding this comment

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

In the try block above, gateway should be in plural.

Also, I think this is still not working for sending transactions if goerli2 is not in the node.json file, but the file exists. OTHER_NETWORKS are added just when the file is not found. Maybe return it in the try block as well?

Copy link
Member

Choose a reason for hiding this comment

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

If we do this, I think it makes sense to remove writing the DEFAULT_NETWORKS to the node.json file at all, because it will be part of GATEWAYS always anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to remove writing the DEFAULT_NETWORKS to the node.json file

It seems difficult to handle (at least, to cleanly handle) localhost network calls when the node.json is not present with this approach^. I think we'd have to add conditionals to get_gateway_url and get_feeder_url in starknet_cli.py. Maybe there's a better way. If it's agreeable, I'd say let's make another issue to track that for the future.

Copy link
Member

@ericnordelo ericnordelo Nov 26, 2022

Choose a reason for hiding this comment

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

Why do we need to handle localhost calls when node.json is not present with this approach? The suggestion is to remove DEFAULT_NETWORKS, which are goerli2 and integration, which gateway_urls won't probably change often. localhost calls would still be going through node.json. Sorry if I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgive me, I did not provide enough context in my response.

The idea was to handle localhost separately from DEFAULT_NETWORKS through some sort of conditional. Otherwise, a node.json will be created (if it's not already) for txs querying the DEFAULT_NETWORKS. This doesn't break anything; however, it's not ideal. This occurs because GATEWAYS.get() returns None when a key does not exist instead of an error that we can handle.

I'm interested in the networks config idea.

Copy link
Member

Choose a reason for hiding this comment

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

Got it.

My issue with the current PR state is that it is not addressing the real issue IMO, which is the UX using Nile. We can use a different approach than the one I suggested in this thread (mine is probably not the best for sure), but letting this as it is, I fear users will upgrade Nile, and they will struggle with sending transactions to goerli2 or integration. You and I both know how to fix it, but I feel the error they will receive is not well documented (and I think we could prevent the issue by default). Is the details what makes great tooling IMO (hardhat vs truffle).

Copy link
Member

@ericnordelo ericnordelo Nov 28, 2022

Choose a reason for hiding this comment

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

We need to give some sort of priority to UX issues like this because they harm adoption. Even if we have a lot of cool functionalities, if we do have not a solid UX, users won't use them IMO as a dev.

I'm of course ok with letting the solution like this if you guys think is enough.

Copy link
Contributor

@martriay martriay Dec 1, 2022

Choose a reason for hiding this comment

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

this is my middle ground suggestion:

  • let's not write any node.json. it's confusing pollution to the user, and it's information nile already knows. no more writing conflicts nor retrocompatibility issues.
  • let's still read node.json. if there's any user custom network, let's add it (or overwrite any default). this is the stem for an actual nile config file, but retrocompatible and it buys us time to design properly, while fixing this.

thoughts?

"localhost": "http://127.0.0.1:5050/",
"goerli2": "https://alpha4-2.starknet.io",
"integration": "https://external.integration.starknet.io",
}
f.write(json.dumps(networks, indent=2))

return networks
gateways = {**DEFAULT_GATEWAYS, **gateway}
return gateways
else:
return DEFAULT_GATEWAYS


def write_node_json(network, gateway_url):
"""Create or update node.json with custom network."""
if not os.path.exists(NODE_FILENAME):
with open(NODE_FILENAME, "w") as fp:
json.dump({network: gateway_url}, fp)
else:
with open(NODE_FILENAME, "r+") as fp:
gateways = json.load(fp)
gateways[network] = gateway_url
fp.seek(0)
json.dump(gateways, fp, indent=2)


GATEWAYS = get_gateways()
Expand Down
10 changes: 3 additions & 7 deletions src/nile/core/node.py
Original file line number Diff line number Diff line change
@@ -1,25 +1,21 @@
"""Command to start StarkNet local network."""
import json
import logging
import subprocess

from nile.common import NODE_FILENAME
from nile.common import DEFAULT_GATEWAYS, write_node_json


def node(host="127.0.0.1", port=5050, seed=None, lite_mode=False):
ericnordelo marked this conversation as resolved.
Show resolved Hide resolved
"""Start StarkNet local network."""
try:
# Save host and port information to be used by other commands
file = NODE_FILENAME
if host == "127.0.0.1":
network = "localhost"
else:
network = host
gateway_url = f"http://{host}:{port}/"
gateway = {network: gateway_url}

with open(file, "w+") as f:
json.dump(gateway, f)
if DEFAULT_GATEWAYS.get(network) != gateway_url:
write_node_json(network, gateway_url)

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

Expand Down
11 changes: 6 additions & 5 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ async def test_node_forwards_args(mock_subprocess):
)
async def test_node_runs_gateway(opts, expected):
# Node life
seconds = 15
seconds = 20

host = opts.get("--host", "127.0.0.1")
port = opts.get("--port", "5050")
Expand Down Expand Up @@ -177,10 +177,11 @@ async def test_node_runs_gateway(opts, expected):
assert status == 200

# Assert network and gateway_url is correct in node.json file
file = NODE_FILENAME
with open(file, "r") as f:
gateway = json.load(f)
assert gateway.get(network) == expected
if expected != "http://127.0.0.1:5050/":
file = NODE_FILENAME
with open(file, "r") as f:
gateway = json.load(f)
assert gateway.get(network) == expected


@pytest.mark.asyncio
Expand Down
61 changes: 60 additions & 1 deletion tests/test_common.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,17 @@
"""Tests for common library."""
import json

import pytest

from nile.common import parse_information, prepare_params, stringify
from nile.common import (
DEFAULT_GATEWAYS,
NODE_FILENAME,
write_node_json,
get_gateways,
parse_information,
prepare_params,
stringify,
)

NETWORK = "goerli"
ARGS = ["1", "2", "3"]
Expand All @@ -12,6 +22,12 @@
STDOUT_2 = "SDTOUT_2"


@pytest.fixture(autouse=True)
def tmp_working_dir(monkeypatch, tmp_path):
monkeypatch.chdir(tmp_path)
return tmp_path


@pytest.mark.parametrize(
"args, expected",
[
Expand Down Expand Up @@ -48,3 +64,46 @@ def test_parse_information():

_a, _b = parse_information(target)
assert _a, _b == (a, b)


@pytest.mark.parametrize(
"network, url, gateway",
[
(None, None, {}),
("localhost", "5051", {"localhost": "5051"}),
("host", "port", {"host": "port"}),
],
)
def test_get_gateways(network, url, gateway):
if network is not None:
write_node_json(network, url)

gateways = get_gateways()
expected = {**DEFAULT_GATEWAYS, **gateway}
assert gateways == expected

# Check that node.json gateway returns in the case of duplicate keys
if network == "localhost":
assert expected["localhost"] != "5050"
assert expected["localhost"] == "5051"


@pytest.mark.parametrize(
"args1, args2, gateways",
[
(
["NETWORK1", "URL1"],
["NETWORK2", "URL2"],
{"NETWORK1": "URL1", "NETWORK2": "URL2"},
),
],
)
def test_write_node_json(args1, args2, gateways):
# Check that node.json is created and adds keys
write_node_json(*args1)
write_node_json(*args2)

with open(NODE_FILENAME, "r") as fp:
result = fp.read()
expected = json.dumps(gateways, indent=2)
assert result == expected