Improve generate_faucet_wallet interface with custom faucet_host

[mvadari]

The current usage of faucet_host is mildly confusing.

For example, the NFT faucet is hosted at https://faucet-nft.ripple.com/accounts. You have to input only faucet-nft.ripple.com in faucet_host. If you leave the https or the accounts part in, the method fails, with a pretty ambiguous error.

A couple of potential fixes:

• There's probably a library that enables you to add parts of a URL together, similar to os.path.join. Using that would fix the inclusion of the https.
• There should be a better error if you leave the /accounts in, or it should still work.
• There should be much better documentation for the generate_faucet_wallet method, to emphasize what to use as the faucet_host.

[wojake]

What if you just removed anything after .com, in this case, it would be /accounts
And if it included https/http, just get the address after ://.
Faucet: https://faucet-nft.ripple.com/accounts

1. https://faucet-nft.ripple.com
2. faucet-nft.ripple.com

Example code, don't use it since most faucet addresses don't end with /accounts but it'll give you an idea of what I'm trying to say, remove anything after .com and get the address after the https://:

address = "https://faucet-nft.ripple.com/accounts"
address_len = len(address)
if address[-9:] == "/accounts":
    address = address[0:(address_len-9)]
    print(address)
    if address[0:8] == "https://":
        address_len = len(address)
        address = address[8:address_len]
        print(address)
    elif address[0:7] == "http://":
        address_len = len(address)
        address = address[7:address_len]
        print(address)

[ckeshava]

Hello,
I considered using https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urljoin to construct the URL, but it has some idiosyncrasies pertaining to the trailing versus leading / character. This Stackoverflow thread explains the non-intuitive nature of the library: https://stackoverflow.com/questions/1793261/how-to-join-components-of-a-path-when-you-are-constructing-a-url-in-python

I agree that we need better docs and error messages. I believe extraneous /accounts should produce an error, otherwise it leads to more confusion.

@wojake Your solution works if every faucet has a prefix of https:// and a suffix of /accounts. Otherwise, I can't think of an elegant solution to handle all the inputs.

I came up with this draft to improve the error message, but I'm not entirely happy with it. Any suggestions/improvements are most welcome. https://github.com/ckeshava/xrpl-py/tree/issue364

[jonathanlei]

Hello, I considered using https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urljoin to construct the URL, but it has some idiosyncrasies pertaining to the trailing versus leading / character. This Stackoverflow thread explains the non-intuitive nature of the library: https://stackoverflow.com/questions/1793261/how-to-join-components-of-a-path-when-you-are-constructing-a-url-in-python

I agree that we need better docs and error messages. I believe extraneous /accounts should produce an error, otherwise it leads to more confusion.

@wojake Your solution works if every faucet has a prefix of https:// and a suffix of /accounts. Otherwise, I can't think of an elegant solution to handle all the inputs.

I came up with this draft to improve the error message, but I'm not entirely happy with it. Any suggestions/improvements are most welcome. https://github.com/ckeshava/xrpl-py/tree/issue364

could you put this in a draft PR and link it? thanks

[mvadari]

I considered using https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urljoin to construct the URL, but it has some idiosyncrasies pertaining to the trailing versus leading / character. This Stackoverflow thread explains the non-intuitive nature of the library: https://stackoverflow.com/questions/1793261/how-to-join-components-of-a-path-when-you-are-constructing-a-url-in-python

For this purpose, those idiosyncrasies don't matter, because we're building the URL and there's only one variable that's a part of it (the main URL). Also, supporting other endpoints that aren't /accounts would be useful for people who set up testnets on their own, external to how Ripple does it.

It also should be pretty easy to check for http[s] at the beginning with a simple startswith check, and only add it if needed.

[ckeshava]

Okay. Is it possible to set up faucet hosts on non-http/https protocols? Would it be a hindrance if we pre-suppose and prepend https:// ?

[mvadari]

Is it possible to set up faucet hosts on non-http/https protocols?

It's certainly possible, because you can set up a faucet however you want. But I don't know of anyone that does it like that, or why they would.

Would it be a hindrance if we pre-suppose and prepend https:// ?

I don't quite understand what you mean. Do you mean assume it already starts with https:// or prepend it regardless, or something else?

[ckeshava]

Yeah, suppose a developer provides abcd.com as the input. What is the expected behavior?

Should I return https://abcd.com/accounts or https://abcd.com/ or abcd.com/accounts ? I feel either of these options would perplex the developer.

If we prepend https://, then how can developers use other internet protocols? (wss://, file servers, etc)
Similarly, appending /accounts is not intuitive either.

[mvadari]

No URL is being returned to the developer, it's all internal to the generate_faucet_wallet function.

IMO the dev should be able to provide any of:

• abcd.com
• https://abcd.com (then they can also provide other internet protocols, though I don't see a need for that right now and that support can always be added later)
• abcd.com/accounts (then they can also provide other sub-URLs)
• https://abcd.com/accounts

[ckeshava]

@mvadari
yes, I understand that it's all internal to the generate_faucet_wallet function.

Here is a draft of my idea -- #676
I need some ideas on how I can test this code (with custom faucet hosts).

Is this what you had in mind?