-
Notifications
You must be signed in to change notification settings - Fork 76
Fix network handling #303
Fix network handling #303
Changes from 6 commits
80d71be
78f8288
d1277cf
e13dfe2
c63c871
789a3ec
45b1fb9
a86c777
34e5a15
48987b6
ba6b005
2784701
9e86a79
ecf66a8
1fa494a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,25 +28,31 @@ | |
# subject to change | ||
"0x041a78e741e5af2fec34b695679bc6891742439f7afb8484ecd7766661ad02bf" | ||
) | ||
DEFAULT_GATEWAYS = { | ||
"goerli2": "https://alpha4-2.starknet.io", | ||
"integration": "https://external.integration.starknet.io", | ||
} | ||
|
||
|
||
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} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if there's a conflicting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there's a conflicting network, the I'll switch the order so that the node.json's gateway overwrites the |
||
return gateways | ||
except FileNotFoundError: | ||
with open(NODE_FILENAME, "w") as f: | ||
networks = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It seems difficult to handle (at least, to cleanly handle) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to handle There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I'm interested in the networks config idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is my middle ground suggestion:
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 | ||
gateway = create_node_json() | ||
gateways = {**gateway, **DEFAULT_GATEWAYS} | ||
return gateways | ||
|
||
|
||
def create_node_json(network="localhost", gateway_url="http://127.0.0.1:5050/"): | ||
"""Create node.json and return gateways.""" | ||
with open(NODE_FILENAME, "w") as f: | ||
gateway = {network: gateway_url} | ||
f.write(json.dumps(gateway)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no more There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
return gateway | ||
|
||
|
||
GATEWAYS = get_gateways() | ||
|
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.
The name is a bit confusing being possible to have multiple custom gateways in the
node.json
file.