XRPLF / xrpl-py

A Python library to interact with the XRP Ledger (XRPL) blockchain
ISC License
150 stars 84 forks source link

Improve the processing of faucet_host URL input (Draft PR) #676

Closed ckeshava closed 6 months ago

ckeshava commented 8 months ago

High Level Overview of Change

This PR seeks to address https://github.com/XRPLF/xrpl-py/issues/364

Context of Change

This is a slight modification in the handling of the faucet_host URL. The developer can have the flexibility to specify the resource paths (like /accounts) and internet protocols ('https://') in the input.

Type of Change

Did you update CHANGELOG.md?

Test Plan

How can I test the use of custom faucet-host? I used faucet-nft.ripple.com, but I'm getting Connection Timeout errors. The existing integration tests use the Testnet, Devnet and hooks faucet to validate the code, but not any custom faucets.

justinr1234 commented 7 months ago

@ckeshava

from urllib.parse import urlparse, urlunparse

def process_faucet_host_url(input_url: str) -> str:
    """
    Construct a URL from the given input string.

    Args:
    - input_url (str): The input string that may or may not include a protocol,
                       and may or may not have a path.

    Returns:
    - str: The constructed URL with https as the default protocol and /accounts as the default path.
    """

    # Parse the input URL to identify its components.
    parsed_url = urlparse(input_url)

    # If the input string includes a protocol (e.g., "https://"), urlparse will correctly parse it.
    # Scheme refers to the protocol (e.g., "https", "http").
    scheme = parsed_url.scheme if parsed_url.scheme else 'https'

    # Netloc is the network location part, which usually includes the domain name.
    # For input "https://abcd.com", netloc is "abcd.com".
    # If no protocol is provided, the domain might be parsed as the path.
    # Consider the input string "abcd.com". If you were to parse this string using urlparse
    # without manually prepending a protocol (like http:// or https://), the parsing logic would
    # interpret "abcd.com" not as the network location part (or domain) of the URL, but rather
    # as the path component. This is because urlparse expects a scheme (protocol) to correctly
    # identify the parts of the URL.
    # Hence, we check if netloc is present; if not, assume the path is actually the netloc.
    netloc = parsed_url.netloc if parsed_url.netloc else parsed_url.path
    path = parsed_url.path if parsed_url.netloc else ''

    # If no specific path is provided, append '/accounts' to the URL.
    # For input "abcd.com", the constructed path will be "/accounts".
    if not path:
        path = '/accounts'

    # Construct the final URL by reassembling its components.
    final_url = urlunparse((scheme, netloc, path, '', '', ''))

    return final_url

Tests

from unittest import TestCase
from utils import process_faucet_host_url

class TestProcessFaucetHostURL(TestCase):
    """Test process_faucet_host_url."""

    def test_process_faucet_host_url_no_protocol(self) -> None:
        """Test with domain only, no protocol. Lacks a protocol and path, so it defaults to https://abcd.com/accounts"""
        input_url = "faucet.devnet.rippletest.net"
        expected_url = "https://faucet.devnet.rippletest.net/accounts"
        self.assertEqual(process_faucet_host_url(input_url), expected_url)

    def test_process_faucet_host_url_with_https(self) -> None:
        """Test with HTTPS protocol specified. Has a complete URL but lacks a path, thus "/accounts" is appended."""
        input_url = "https://faucet.devnet.rippletest.net"
        expected_url = "https://faucet.devnet.rippletest.net/accounts"
        self.assertEqual(process_faucet_host_url(input_url), expected_url)

    def test_process_faucet_host_url_with_path(self) -> None:
        """Test with domain and path, no protocol. Lacks a protocol, defaults to "https://", and already specifies a path."""
        input_url = "faucet.devnet.rippletest.net/accounts"
        expected_url = "https://faucet.devnet.rippletest.net/accounts"
        self.assertEqual(process_faucet_host_url(input_url), expected_url)

    def test_process_faucet_host_url_full(self) -> None:
        """Test with full URL."""
        input_url = "https://faucet.devnet.rippletest.net/accounts"
        expected_url = "https://faucet.devnet.rippletest.net/accounts"
        self.assertEqual(process_faucet_host_url(input_url), expected_url)

     def test_process_faucet_host_url_different_protocol(self) -> None:
        """Test with a non-HTTP protocol specified."""
        input_url = "ftp://faucet.devnet.rippletest.net"
        expected_url = "ftp://faucet.devnet.rippletest.net/accounts"
        self.assertEqual(process_faucet_host_url(input_url), expected_url)
ckeshava commented 7 months ago

Hey @justinr1234 , Thank you very much for writing this code. It is very thorough and clear. However, I'm facing trouble with two test cases:

    # failing test cases
    # third test case in your code
    def test_process_faucet_host_url_with_path(self) -> None:
        """Test with domain and path, no protocol. Lacks a protocol, defaults to "https://", and already specifies a path."""
        input_url = "faucet.devnet.rippletest.net/accounts"
        expected_url = "https://faucet.devnet.rippletest.net/accounts"
        self.assertEqual(process_faucet_host_url(input_url), expected_url)

    def test_process_faucet_host_url_trailing_slash(self) -> None:
        """Test with a trailing slash character."""
        input_url = "https://faucet.devnet.rippletest.net/"
        expected_url = "https://faucet.devnet.rippletest.net/accounts"
        self.assertEqual(process_faucet_host_url(input_url), expected_url)

    def test_process_faucet_host_url_custom_path(self) -> None:
        """Test with a custom path specified."""
        input_url = "https://faucet.devnet.rippletest.net/america/"
        expected_url = "https://faucet.devnet.rippletest.net/america"
        self.assertEqual(process_faucet_host_url(input_url), expected_url)

The test cases are failing when the path is specified, and/or there is a trailing slash / in the URL.

Here is the stacktrace:

  xrpl-py git:(justinURL) ✗ poetry run poe test tests/unit/asyn/wallet/test_wallet.py
Poe => python3 -m unittest tests/unit/asyn/wallet/test_wallet.py
F...F.F.....
======================================================================
FAIL: test_process_faucet_host_url_custom_path (tests.unit.asyn.wallet.test_wallet.TestProcessFaucetHostURL)
Test with a non-HTTP protocol specified.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ckeshavabs/xrpl-py/tests/unit/asyn/wallet/test_wallet.py", line 102, in test_process_faucet_host_url_custom_path
    self.assertEqual(process_faucet_host_url(input_url), expected_url)
AssertionError: 'https://faucet.devnet.rippletest.net/america/' != 'https://faucet.devnet.rippletest.net/america'
- https://faucet.devnet.rippletest.net/america/
?                                             -
+ https://faucet.devnet.rippletest.net/america

======================================================================
FAIL: test_process_faucet_host_url_trailing_slash (tests.unit.asyn.wallet.test_wallet.TestProcessFaucetHostURL)
Test with a non-HTTP protocol specified.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ckeshavabs/xrpl-py/tests/unit/asyn/wallet/test_wallet.py", line 96, in test_process_faucet_host_url_trailing_slash
    self.assertEqual(process_faucet_host_url(input_url), expected_url)
AssertionError: 'https://faucet.devnet.rippletest.net/' != 'https://faucet.devnet.rippletest.net/accounts'
- https://faucet.devnet.rippletest.net/
+ https://faucet.devnet.rippletest.net/accounts
?                                      ++++++++

======================================================================
FAIL: test_process_faucet_host_url_with_path (tests.unit.asyn.wallet.test_wallet.TestProcessFaucetHostURL)
Test with domain and path, no protocol. Lacks a protocol, defaults to "https://", and already specifies a path.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ckeshavabs/xrpl-py/tests/unit/asyn/wallet/test_wallet.py", line 77, in test_process_faucet_host_url_with_path
    self.assertEqual(process_faucet_host_url(input_url), expected_url)
AssertionError: 'https://faucet.devnet.rippletest.net/accounts/accounts' != 'https://faucet.devnet.rippletest.net/accounts'
- https://faucet.devnet.rippletest.net/accounts/accounts
?                                              ---------
+ https://faucet.devnet.rippletest.net/accounts

----------------------------------------------------------------------
Ran 12 tests in 0.015s

FAILED (failures=3)
➜  xrpl-py git:(justinURL) ✗ 

This is a known behavior of the urllib in Python. Are we expecting our users to not specify a trailing / in their inputs? I have noted these behavioral differences in this comment: https://github.com/XRPLF/xrpl-py/blob/50ae888e83db8ea3dc1673b15290e71931e2ad71/xrpl/asyncio/wallet/wallet_generation.py#L114

justinr1234 commented 7 months ago

@ckeshava we should be able to handle stripping trailing slashes I’d say

ckeshava commented 6 months ago

This issue is being solved by https://github.com/XRPLF/xrpl-py/pull/690, hence closing this PR