Fix network handling #303

andrew-fleming · 2022-11-23T20:37:57Z

Resolves #297.

The current flow of network handling creates the awkward issue of an unresolved query in perpetuity when the node.json exists but the network key does not. This PR, therefore, proposes to add goerli2 and integration gateways to the node.json in the nile node command. This means that the goerli2 and integration gateways will be accessible irrespective of the node.json file's existence.

martriay

good patch. but i think the real solution is to centralize file writes to avoid this kind of bug. it's also a bit awkward that get_gateways writes anywhere, we should either refactor or rename.

andrew-fleming · 2022-11-25T01:07:44Z

Agreed on centralizing file writes and with that, a refactor on get_gateways. Should we keep this patch for now or go for the refactor in this PR?

martriay · 2022-11-25T01:28:49Z

if you think it's small enough, let's squeeze it here. otherwise the patch + tracking the issue is enough.

andrew-fleming · 2022-11-25T01:52:28Z

if you think it's small enough, let's squeeze it here.

On it!

ericnordelo

Thanks for the fix @andrew-fleming! Left some comments.

src/nile/common.py

ericnordelo · 2022-11-25T11:45:10Z

src/nile/common.py

@@ -39,11 +43,7 @@ def get_gateways():

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


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?

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.

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.

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.

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.

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).

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.

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?

src/nile/common.py

src/nile/core/node.py

tests/test_common.py

ericnordelo · 2022-11-25T11:49:36Z

src/nile/common.py

-                "goerli2": "https://alpha4-2.starknet.io",
-                "integration": "https://external.integration.starknet.io",
-            }
+            networks = {"localhost": "http://127.0.0.1:5050/", **OTHER_NETWORKS}


If we add DEFAULT_GATEWAYS to queries even when the node.json exists:

Suggested change

networks = {"localhost": "http://127.0.0.1:5050/", **OTHER_NETWORKS}

networks = {"localhost": "http://127.0.0.1:5050/"}

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

ericnordelo

@andrew-fleming The refactor looks good, but this PR is not solving the linked issue imo. Even when the error message differs from the last merges, the expected behavior, for me at least, is goerli2 transactions working by default (there are no docs telling users they need to add this manually to node.json afaik).

I think we have enough content with these different networks to add a nile.ini (or different name) required config file with networks instead of deleting/creating a node.json file.

Adding these networks by default to the ini file and documenting this would be a huge improvement to the UX IMO.

Interested in what you guys think about this two thoughts:

Issue resolution that I think could be solved by returning the DEFAULT_GATEWAYS in the get_gateways method even when they are not in the node.json file.
Replace node.json with nile.ini file with a networks config.

src/nile/core/node.py

andrew-fleming · 2022-11-28T06:36:42Z

this PR is not solving the linked issue imo. Even when the error message differs from the last merges, the expected behavior, for me at least, is goerli2 transactions working by default (there are no docs telling users they need to add this manually to node.json afaik).

The patch does solve that^. The user doesn't have to add anything manually. The flow was that if a node.json exists, localhost and DEFAULT_NETWORKS would already be written in the json. If the node.json doesn't exist, it would be created with all three networks ready to go. Please correct me if I misunderstood the comment.

This PR now (since after that review) doesn't write the DEFAULT_NETWORKS to the node.json. Instead, it follows the suggestion of returning the contents of node.json (which consists only of localhost) with the DEFAULT_GATEWAYS dictionary from common.py.

Issue resolution that I think could be solved by returning the DEFAULT_GATEWAYS in the get_gateways method even when they are not in the node.json file.

Replace node.json with nile.ini file with a networks config.

The latest pushed changes follow #1. The one minor issue with this flow is that a node.json will be created (if it doesn't already exist) when querying DEFAULT_NETWORKS.

If we want to go for #2, we should adjust the issue size or create a new issue :)

ericnordelo · 2022-11-28T12:31:43Z

The patch does solve that^. The user doesn't have to add anything manually. The flow was that if a node.json exists, localhost and DEFAULT_NETWORKS would already be written in the json. If the node.json doesn't exist, it would be created with all three networks ready to go. Please correct me if I misunderstood the comment.

This is assuming users would follow the flow, but my concern was also with upgrades, like what happened to me: Dev already have a nile project, already having a node.json file, they try to use goerli2, cryptic error not documented.

If we want to go for #2, we should adjust the issue size or create a new issue :)

Totally agree. I think we should prioritize this as possible (not necessarily my proposed solution, but something to improve the network handling). cc @martriay

martriay · 2022-12-01T04:45:41Z

src/nile/common.py



 def get_gateways():
    """Get the StarkNet node details."""
    try:
        with open(NODE_FILENAME, "r") as f:
            gateway = json.load(f)
-            return gateway
-
+            gateways = {**gateway, **DEFAULT_GATEWAYS}


what happens if there's a conflicting goerli2 or integration network? would it be overwritten by this?

If there's a conflicting network, the DEFAULT_GATEWAYS network would overwrite it in the local variable gateways when the gateways dict is passed to the querying function. Thus, "goerli2": "new_url" in node.json will return the goerli2 from DEFAULT_NETWORKS

I'll switch the order so that the node.json's gateway overwrites the DEFAULT_GATEWAYS network of the same name

martriay · 2022-12-01T04:53:21Z

src/nile/common.py

+    """Create node.json and return gateways."""
+    with open(NODE_FILENAME, "w") as f:
+        gateway = {network: gateway_url}
+        f.write(json.dumps(gateway))


no more indent=2?

The idea was to only have one gateway in node.json, but I'll add the indent back so it's formatted for users who want to add more networks

martriay · 2022-12-01T05:00:54Z

src/nile/common.py

@@ -39,11 +43,7 @@ def get_gateways():

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


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?

ericnordelo · 2022-12-01T12:35:41Z

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?

Just confirming. You mean not writing to the node.json file after creating it, but keeping all network configurations and reading from it, right? I'd keep all the networks in a network config, to allow changing the RPC providers (gateway_urls) if we have more than one at some point for the same network (I assume we'll have).

martriay · 2022-12-01T17:53:02Z

Just confirming. You mean not writing to the node.json file after creating it, but keeping all network configurations and reading from it, right? I'd keep all the networks in a network config, to allow changing the RPC providers (gateway_urls) if we have more than one at some point for the same network (I assume we'll have).

I mean not write/create any node.json files anymore, but take it into account if there's one already (e.g. with a custom network). this would be retrocompatible and fix the UX issue you point out, without us introducing any weird, fragile logic to overwrite/fix existing node.json files, and also provide a way for users to add their custom ones.

node.json would be the network file you suggest, my approach only simplifies it so we can ship a fix asap. this node.json file can eventually evolve into a full config file #73, but i'm leaving it for later to avoid the design + refactor time it entails.

andrew-fleming · 2022-12-02T08:33:09Z

Here's a few notes on the latest changes aiming to address the conversations regarding this PR @martriay @ericnordelo:

The default localhost now belongs in DEFAULT_GATEWAYS.
node.json will only be written when nile node receives a different host or port i.e. nile node --port 5001.
- In the event that the localhost key points to a different value (this key/value entry would only exist in the node.json), the network will favor the node.json value.
- When the node.json receives new entries, the previous entries will not be deleted. Should the node.json receive a duplicate key, however, the new value will overwrite the old.

martriay · 2022-12-06T03:20:30Z

src/nile/common.py

+        with open(NODE_FILENAME, "r") as fp:
+            gateways = json.load(fp)
+            gateways[network] = gateway_url
+
+        with open(NODE_FILENAME, "w") as fp:
+            json.dump(gateways, fp, indent=2)


Suggested change

with open(NODE_FILENAME, "r") as fp:

gateways = json.load(fp)

gateways[network] = gateway_url

with open(NODE_FILENAME, "w") as fp:

json.dump(gateways, fp, indent=2)

with open(NODE_FILENAME, "w") as fp:

gateways = json.load(fp)

gateways[network] = gateway_url

json.dump(gateways, fp, indent=2)

isn't this simpler? why indent is only explicit here?

This is simpler^, but the w will remove the prior contents. This gives me an idea though

src/nile/common.py

Co-authored-by: Martín Triay <martriay@gmail.com>

ericnordelo

Looking good! Left just a small suggestion and I think we are good to go.

ericnordelo · 2022-12-11T14:46:13Z

src/nile/common.py



 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)


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.

ericnordelo

LGTM!

andrew-fleming added 2 commits November 22, 2022 17:23

add other networks to node.json by default

80d71be

clean up test

78f8288

martriay previously approved these changes Nov 23, 2022

View reviewed changes

martriay requested a review from ericnordelo November 25, 2022 01:28

ericnordelo reviewed Nov 25, 2022

View reviewed changes

andrew-fleming dismissed martriay’s stale review via d1277cf November 25, 2022 18:40

andrew-fleming and others added 2 commits November 25, 2022 13:40

Apply suggestions from code review

d1277cf

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

centralize file writes

e13dfe2

ericnordelo reviewed Nov 26, 2022

View reviewed changes

src/nile/core/node.py Show resolved Hide resolved

andrew-fleming added 2 commits November 26, 2022 15:27

remove writing DEFAULT_NETWORKS to node.json

c63c871

fix formatting

789a3ec

andrew-fleming mentioned this pull request Nov 28, 2022

Improve network handling #312

Open

martriay reviewed Dec 1, 2022

View reviewed changes

andrew-fleming added 4 commits December 2, 2022 02:40

compare gateway to default

45b1fb9

fix get_gateways, refactor create_node_json

a86c777

fix node timer and test

34e5a15

fix node_json and get_gateways tests

48987b6

fix test

ba6b005

martriay previously approved these changes Dec 6, 2022

View reviewed changes

Update src/nile/common.py

2784701

Co-authored-by: Martín Triay <martriay@gmail.com>

andrew-fleming dismissed martriay’s stale review via 2784701 December 6, 2022 04:12

Merge branch 'OpenZeppelin:main' into fix-network-handling

9e86a79

improve write_node_json

ecf66a8

martriay previously approved these changes Dec 10, 2022

View reviewed changes

martriay requested a review from ericnordelo December 10, 2022 06:34

ericnordelo previously approved these changes Dec 11, 2022

View reviewed changes

change gateways variable to custom_gateways

1fa494a

andrew-fleming dismissed stale reviews from ericnordelo and martriay via 1fa494a December 12, 2022 06:27

andrew-fleming requested a review from ericnordelo December 12, 2022 06:29

ericnordelo approved these changes Dec 12, 2022

View reviewed changes

martriay merged commit 8ca995a into OpenZeppelin:main Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix network handling #303

Fix network handling #303

andrew-fleming commented Nov 23, 2022

martriay left a comment

andrew-fleming commented Nov 25, 2022

martriay commented Nov 25, 2022

andrew-fleming commented Nov 25, 2022

ericnordelo left a comment

ericnordelo Nov 25, 2022

ericnordelo Nov 25, 2022

andrew-fleming Nov 25, 2022

andrew-fleming Nov 25, 2022

ericnordelo Nov 26, 2022 •

edited

Loading

andrew-fleming Nov 28, 2022

ericnordelo Nov 28, 2022

ericnordelo Nov 28, 2022 •

edited

Loading

martriay Dec 1, 2022 •

edited

Loading

ericnordelo Nov 25, 2022

ericnordelo left a comment •

edited

Loading

andrew-fleming commented Nov 28, 2022 •

edited

Loading

ericnordelo commented Nov 28, 2022

martriay Dec 1, 2022

andrew-fleming Dec 1, 2022

martriay Dec 1, 2022

andrew-fleming Dec 1, 2022

martriay Dec 1, 2022 •

edited

Loading

ericnordelo commented Dec 1, 2022

martriay commented Dec 1, 2022 •

edited

Loading

andrew-fleming commented Dec 2, 2022

martriay Dec 6, 2022

andrew-fleming Dec 6, 2022

ericnordelo left a comment

ericnordelo Dec 11, 2022

ericnordelo left a comment

	networks = {"localhost": "http://127.0.0.1:5050/", **OTHER_NETWORKS}
	networks = {"localhost": "http://127.0.0.1:5050/"}

Fix network handling #303

Fix network handling #303

Conversation

andrew-fleming commented Nov 23, 2022

martriay left a comment

Choose a reason for hiding this comment

andrew-fleming commented Nov 25, 2022

martriay commented Nov 25, 2022

andrew-fleming commented Nov 25, 2022

ericnordelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo Nov 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

martriay Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo left a comment • edited Loading

Choose a reason for hiding this comment

andrew-fleming commented Nov 28, 2022 • edited Loading

ericnordelo commented Nov 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martriay Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

ericnordelo commented Dec 1, 2022

martriay commented Dec 1, 2022 • edited Loading

andrew-fleming commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

ericnordelo Nov 26, 2022 •

edited

Loading

ericnordelo Nov 28, 2022 •

edited

Loading

martriay Dec 1, 2022 •

edited

Loading

ericnordelo left a comment •

edited

Loading

andrew-fleming commented Nov 28, 2022 •

edited

Loading

martriay Dec 1, 2022 •

edited

Loading

martriay commented Dec 1, 2022 •

edited

Loading