-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix #710] Use network_id to identify faucet_url #715
base: main
Are you sure you want to change the base?
Changes from 4 commits
ea6365a
23d3eb3
56430f3
ab918dc
afcb4d3
7f93914
cf3d8e5
f06e5c1
bb62029
3881c7b
0839082
fe616ec
e346219
c30f772
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 |
---|---|---|
@@ -1,4 +1,5 @@ | ||
"""Handles wallet generation from a faucet.""" | ||
|
||
import asyncio | ||
from typing import Optional | ||
from urllib.parse import urlparse, urlunparse | ||
|
@@ -8,6 +9,7 @@ | |
|
||
from xrpl.asyncio.account import get_balance, get_next_valid_seq_number | ||
from xrpl.asyncio.clients import Client, XRPLRequestFailureException | ||
from xrpl.asyncio.clients.client import get_network_id_and_build_version | ||
from xrpl.constants import XRPLException | ||
from xrpl.wallet.main import Wallet | ||
|
||
|
@@ -57,7 +59,9 @@ async def generate_faucet_wallet( | |
|
||
.. # noqa: DAR402 exception raised in private method | ||
""" | ||
faucet_url = get_faucet_url(client.url, faucet_host) | ||
if not client.network_id: | ||
await get_network_id_and_build_version(client) | ||
faucet_url = get_faucet_url(client.network_id, faucet_host) | ||
|
||
if wallet is None: | ||
wallet = Wallet.create() | ||
|
@@ -150,34 +154,40 @@ def process_faucet_host_url(input_url: str) -> str: | |
return final_url | ||
|
||
|
||
def get_faucet_url(url: str, faucet_host: Optional[str] = None) -> str: | ||
def get_faucet_url(network_id: Optional[int], faucet_host: Optional[str] = None) -> str: | ||
""" | ||
Returns the URL of the faucet that should be used, based on whether the URL is from | ||
a testnet or devnet client. | ||
Returns the URL of the faucet that should be used, based on network_id and | ||
faucet_host | ||
|
||
Args: | ||
url: The URL that the client is using to access the ledger. | ||
faucet_host: A custom host to use for funding a wallet. | ||
network_id: The network_id corresponding to the XRPL network. This is parsed | ||
from a ServerInfo() rippled response | ||
faucet_host: A custom host to use for funding a wallet. This is useful because | ||
network_id of sidechains might not be well-known. If faucet_url cannot be | ||
gleaned from network_id alone, faucet_host is used | ||
|
||
Returns: | ||
The URL of the matching faucet. | ||
|
||
Raises: | ||
XRPLFaucetException: if the provided URL is not for the testnet or devnet. | ||
XRPLFaucetException: if the provided network_id does not correspond to testnet | ||
or devnet. | ||
""" | ||
if faucet_host is not None: | ||
return process_faucet_host_url(faucet_host) | ||
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. Upon further thought, I think it might make more sense to bring this code out of this function and have 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. yeah, I put down a snippet in a comment below 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. updated somewhere prior to f06e5c1 |
||
if "altnet" in url or "testnet" in url: # testnet | ||
return _TEST_FAUCET_URL | ||
if "sidechain-net2" in url: # sidechain issuing chain devnet | ||
raise XRPLFaucetException( | ||
"Cannot fund an account on an issuing chain. Accounts must be created via " | ||
"the bridge." | ||
) | ||
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 piece still needs to be included. The network ID is 262. 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. thanks. fixed in f06e5c1 |
||
if "devnet" in url: # devnet | ||
return _DEV_FAUCET_URL | ||
|
||
if network_id is not None: | ||
mvadari marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if network_id == 1: | ||
return _TEST_FAUCET_URL | ||
elif network_id == 2: | ||
return _DEV_FAUCET_URL | ||
elif network_id == 0: | ||
raise XRPLFaucetException("Cannot create faucet with a client on mainnet.") | ||
elif network_id < 0: | ||
raise XRPLFaucetException("network_id cannot be negative.") | ||
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. Does rippled even allow this scenario? 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 config file does not allow negative numbers as input for network_id. But it accepts really large numbers (for eg: also, rippled unit tests allow negative numbers - XRPLF/rippled@develop...ckeshava:rippled:negNetworkID 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. I don't think xrpl-py needs any specific checks for this, "we don't understand this network ID" is fine. 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. updated in f06e5c1 |
||
|
||
raise XRPLFaucetException( | ||
"Cannot fund an account with a client that is not on the testnet or devnet." | ||
"Cannot create faucet URL without network_id or the faucet_host information." | ||
) | ||
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. Thoughts on throwing this error in the main if network_id is None and faucet_host is None:
raise XRPLFaucetException(...) Otherwise, this error currently executes on an unknown devnet as well, which may have a network ID. 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. I don't object to that idea, but how is that different from the current implementation? At line 189 (the last line of the 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 said devnet will have a networkID, hence this condition needs to be: if faucet_host is None and networkID >= 1025:
raise XRPLFaucetException(...) The current implementation is equivalent to the above if-condition 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 difference comes from typing and failing fast. IMO it makes more sense for You can't determine the host URL from an unknown Devnet, but the error message should be different. 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 cannot expect all networks to have a valid
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. That flow makes more sense to me, personally. 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. alrighty 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. updated sometime before f06e5c1 |
||
|
||
|
||
|
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.
these test cases are already covered in the below class
TestProcessFaucetHostURL