-
Notifications
You must be signed in to change notification settings - Fork 105
[Fix #710] Use network_id to identify faucet_url #715
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
Changes from 1 commit
ea6365a
23d3eb3
56430f3
ab918dc
afcb4d3
7f93914
cf3d8e5
f06e5c1
bb62029
3881c7b
0839082
fe616ec
e346219
c30f772
2c91e53
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,11 +1,14 @@ | ||
"""Interface for all network clients to follow.""" | ||
|
||
from __future__ import annotations | ||
|
||
from abc import ABC, abstractmethod | ||
from typing import Optional | ||
|
||
from typing_extensions import Final | ||
|
||
from xrpl.asyncio.clients.exceptions import XRPLRequestFailureException | ||
from xrpl.models.requests import ServerInfo | ||
from xrpl.models.requests.request import Request | ||
from xrpl.models.response import Response | ||
|
||
|
@@ -51,3 +54,24 @@ async def _request_impl( | |
:meta private: | ||
""" | ||
pass | ||
|
||
|
||
async def _get_network_id_and_build_version(client: Client) -> None: | ||
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 not automatically run this on every client upon instantiation, instead of having it be a helper function? Then it only needs to be run once, instead of every time a function that uses this is called. 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 are not duplicating invocations of this function. Here are two examples where the But I agree, this could be done in a cleaner way. let me try it out 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'm open to the option of doing it lazily - i.e. only if and as needed, but only once. All that would require is checking if the network ID has been instantiated before running the request. 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. okay, I've added a check: 23d3eb3 |
||
""" | ||
Get the network id and build version of the connected server. | ||
|
||
Args: | ||
client: The network client to use to send the request. | ||
|
||
Raises: | ||
XRPLRequestFailureException: if the rippled API call fails. | ||
""" | ||
response = await client._request_impl(ServerInfo()) | ||
if response.is_successful(): | ||
if "network_id" in response.result["info"]: | ||
client.network_id = response.result["info"]["network_id"] | ||
if not client.build_version and "build_version" in response.result["info"]: | ||
client.build_version = response.result["info"]["build_version"] | ||
return | ||
|
||
raise XRPLRequestFailureException(response.result) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,19 @@ | ||
"""High-level transaction methods with XRPL transactions.""" | ||
|
||
import math | ||
from typing import Any, Dict, Optional, cast | ||
|
||
from typing_extensions import Final | ||
|
||
from xrpl.asyncio.account import get_next_valid_seq_number | ||
from xrpl.asyncio.clients import Client, XRPLRequestFailureException | ||
from xrpl.asyncio.clients.client import _get_network_id_and_build_version | ||
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. Any function that gets imported elsewhere shouldn't start with a 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. fixed in ab918dc |
||
from xrpl.asyncio.ledger import get_fee, get_latest_validated_ledger_sequence | ||
from xrpl.constants import XRPLException | ||
from xrpl.core.addresscodec import is_valid_xaddress, xaddress_to_classic_address | ||
from xrpl.core.binarycodec import encode, encode_for_multisigning, encode_for_signing | ||
from xrpl.core.keypairs.main import sign as keypairs_sign | ||
from xrpl.models.requests import ServerInfo, ServerState, SubmitOnly | ||
from xrpl.models.requests import ServerState, SubmitOnly | ||
from xrpl.models.response import Response | ||
from xrpl.models.transactions import EscrowFinish | ||
from xrpl.models.transactions.transaction import Signer, Transaction | ||
|
@@ -247,27 +249,6 @@ async def autofill( | |
return Transaction.from_dict(transaction_json) | ||
|
||
|
||
async def _get_network_id_and_build_version(client: Client) -> None: | ||
""" | ||
Get the network id and build version of the connected server. | ||
|
||
Args: | ||
client: The network client to use to send the request. | ||
|
||
Raises: | ||
XRPLRequestFailureException: if the rippled API call fails. | ||
""" | ||
response = await client._request_impl(ServerInfo()) | ||
if response.is_successful(): | ||
if "network_id" in response.result["info"]: | ||
client.network_id = response.result["info"]["network_id"] | ||
if not client.build_version and "build_version" in response.result["info"]: | ||
client.build_version = response.result["info"]["build_version"] | ||
return | ||
|
||
raise XRPLRequestFailureException(response.result) | ||
|
||
|
||
def _tx_needs_networkID(client: Client) -> bool: | ||
""" | ||
Determines whether the transactions required network ID to be valid. | ||
|
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) | ||
ckeshava marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 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 |
||
|
||
if faucet_host is not None: | ||
return process_faucet_host_url(faucet_host) | ||
mvadari marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 | ||
|
||
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