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.

We are not duplicating invocations of this function. Here are two examples where the network_id information persists in the client object.

But I agree, this could be done in a cleaner way.
I can only invoke _get_network_id_and_build_version function after a client has opened the socket-connection. So, I'll need to invoke this in this open() method of all the child classes of Client

let me try it out

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.

okay, I've added a check: 23d3eb3

Any function that gets imported elsewhere shouldn't start with a _.

fixed in ab918dc

Does rippled even allow this scenario?

the config file does not allow negative numbers as input for network_id. But it accepts really large numbers (for eg: 4294967295 which could be negative numbers, if typecast into smaller signed-types)

also, rippled unit tests allow negative numbers - XRPLF/rippled@develop...ckeshava:rippled:negNetworkID

I don't think xrpl-py needs any specific checks for this, "we don't understand this network ID" is fine.

updated in f06e5c1

Thoughts on throwing this error in the main generate_faucet_wallet function instead of here? e.g.

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.

I don't object to that idea, but how is that different from the current implementation?

At line 189 (the last line of the get_faucet_url method), we have established that faucet_host == None. You are correct, an unknown devnet might have a networkID, but how can I determine the host_url from a networkID (say 1345 ?)

if network_id is None and faucet_host is None:
  raise XRPLFaucetException(...)

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

The difference comes from typing and failing fast. IMO it makes more sense for get_faucet_url to not accept None as a valid network ID.

You can't determine the host URL from an unknown Devnet, but the error message should be different.

we cannot expect all networks to have a valid NetworkID isn't it?
If we force get_faucet_url to accept valid networkIDs only, then some of the processing logic shifts into generate_faucet_wallet method.

def get_faucet_url(network_id: int) -> str:
    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.")

    # unreachable code
    return ""

async def generate_faucet_wallet(
    client: Client,
    wallet: Optional[Wallet] = None,
    debug: bool = False,
    faucet_host: Optional[str] = None,
    usage_context: Optional[str] = None,
    user_agent: Optional[str] = "xrpl-py",
) -> Wallet:
    if not client.network_id:
        await get_network_id_and_build_version(client)

    if client.network_id is None and faucet_host is None:
        raise XRPLFaucetException(
            "Cannot create faucet URL without network_id or the faucet_host information"
        )

    if faucet_host is not None:
        faucet_url = process_faucet_host_url(faucet_host)
    else:
        faucet_url = get_faucet_url(client.network_id)
        
    # remaining code in the function ...

That flow makes more sense to me, personally.

alrighty

updated sometime before f06e5c1

Upon further thought, I think it might make more sense to bring this code out of this function and have get_faucet_url be entirely based on the network_id.

yeah, I put down a snippet in a comment below

updated somewhere prior to f06e5c1

these test cases are already covered in the below class TestProcessFaucetHostURL

You should raise an error here.

fixed in f06e5c1

This piece still needs to be included. The network ID is 262.

thanks. fixed in f06e5c1

Can this if-else be replaced with a dictionary? It'll be easier to maintain that way.

updated in f06e5c1

This object should be outside of the function, since it's not dependent on the data inside. And the name should be in uppercase.

nit: