[Fix #710] Use network_id to identify faucet_url #715

ckeshava · 2024-06-12T23:52:51Z

High Level Overview of Change

use network_id to disambiguate between different XRPL chains
move the utility function _get_network_id_and_build_version into client.py file for convinience

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Did you update CHANGELOG.md?

Yes
No, this change does not impact library users
This change should not modify the experience for external developers

Test Plan

Tests have been modified to accomodate the new API for grabbing the faucet_url

- move the utility function _get_network_id_and_build_version into client.py file for convinience

mvadari · 2024-06-25T14:12:02Z

xrpl/asyncio/clients/client.py

@@ -51,3 +54,24 @@ async def _request_impl(
        :meta private:
        """
        pass
+
+
+async def _get_network_id_and_build_version(client: Client) -> None:


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

mvadari · 2024-07-01T15:11:41Z

xrpl/asyncio/transaction/main.py

 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


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

fixed in ab918dc

xrpl/asyncio/wallet/wallet_generation.py

Co-authored-by: Mayukha Vadari <mvadari@gmail.com>

mvadari · 2024-07-03T14:48:08Z

xrpl/asyncio/wallet/wallet_generation.py

+        elif network_id < 0:
+            raise XRPLFaucetException("network_id cannot be negative.")


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

mvadari · 2024-07-03T14:50:02Z

xrpl/asyncio/wallet/wallet_generation.py

    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."
    )


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.

updated sometime before f06e5c1

mvadari · 2024-07-03T18:23:26Z

xrpl/asyncio/wallet/wallet_generation.py

    if faucet_host is not None:
        return process_faucet_host_url(faucet_host)


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

ckeshava · 2024-07-03T20:20:18Z

tests/unit/asyn/wallet/test_wallet.py

-        self.assertEqual(get_faucet_url(json_client_url, custom_host), expected_faucet)
-        self.assertEqual(get_faucet_url(ws_client_url, custom_host), expected_faucet)
-
-    def test_get_faucet_wallet_test(self):


these test cases are already covered in the below class TestProcessFaucetHostURL

mvadari · 2024-07-05T20:48:53Z

xrpl/asyncio/wallet/wallet_generation.py

+        raise XRPLFaucetException("Unable to parse the specified network_id.")
+
+    # this line is unreachable. Custom devnets must specify a faucet_host input
+    return ""


You should raise an error here.

fixed in f06e5c1

mvadari · 2024-07-05T20:50:22Z

xrpl/asyncio/wallet/wallet_generation.py

-    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."
-        )


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

thanks. fixed in f06e5c1

mvadari · 2024-07-05T20:50:49Z

xrpl/asyncio/wallet/wallet_generation.py

-        return process_faucet_host_url(faucet_host)
-    if "altnet" in url or "testnet" in url:  # testnet
+    if network_id == 1:


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

updated in f06e5c1

CHANGELOG.md

mvadari · 2024-07-08T19:47:03Z

xrpl/asyncio/wallet/wallet_generation.py

-    if "altnet" in url or "testnet" in url:  # testnet
-        return _TEST_FAUCET_URL
-    if "sidechain-net2" in url:  # sidechain issuing chain devnet
+    map_network_id_to_url: Dict[int, str] = {1: _TEST_FAUCET_URL, 2: _DEV_FAUCET_URL}


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

xrpl/asyncio/wallet/wallet_generation.py

Co-authored-by: Mayukha Vadari <mvadari@gmail.com>

mvadari · 2024-07-08T21:36:56Z

xrpl/asyncio/wallet/wallet_generation.py

@@ -17,6 +17,7 @@
 _DEV_FAUCET_URL: Final[str] = "https://faucet.devnet.rippletest.net/accounts"

 _TIMEOUT_SECONDS: Final[int] = 40
+_MAP_NETWORK_ID_TO_URL: Dict[int, str] = {1: _TEST_FAUCET_URL, 2: _DEV_FAUCET_URL}


nit:

Suggested change

_MAP_NETWORK_ID_TO_URL: Dict[int, str] = {1: _TEST_FAUCET_URL, 2: _DEV_FAUCET_URL}

_NETWORK_ID_URL_MAP: Dict[int, str] = {1: _TEST_FAUCET_URL, 2: _DEV_FAUCET_URL}

[Fix XRPLF#710] Use network_id to identify faucet_url

ea6365a

- move the utility function _get_network_id_and_build_version into client.py file for convinience

ckeshava requested review from mvadari and justinr1234 June 12, 2024 23:52

mvadari reviewed Jun 25, 2024

View reviewed changes

additional check to prevent wasteful ServerInfo() requests

23d3eb3

mvadari reviewed Jul 1, 2024

View reviewed changes

xrpl/asyncio/wallet/wallet_generation.py Outdated Show resolved Hide resolved

mvadari reviewed Jul 1, 2024

View reviewed changes

xrpl/asyncio/wallet/wallet_generation.py Outdated Show resolved Hide resolved

mvadari reviewed Jul 1, 2024

View reviewed changes

xrpl/asyncio/wallet/wallet_generation.py Outdated Show resolved Hide resolved

ckeshava and others added 2 commits July 1, 2024 14:36

Update xrpl/asyncio/wallet/wallet_generation.py

56430f3

Co-authored-by: Mayukha Vadari <mvadari@gmail.com>

address PR comments

ab918dc

mvadari reviewed Jul 3, 2024

View reviewed changes

refactor the generate faucet wallet logic

afcb4d3

ckeshava commented Jul 3, 2024

View reviewed changes

remove extra checks on the existence of client.network_id

7f93914

ckeshava requested a review from mvadari July 3, 2024 20:24

Merge branch 'main' into faucet_url_logic

cf3d8e5

mvadari reviewed Jul 5, 2024

View reviewed changes

address PR comments

f06e5c1

mvadari reviewed Jul 8, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mvadari reviewed Jul 8, 2024

View reviewed changes

xrpl/asyncio/wallet/wallet_generation.py Outdated Show resolved Hide resolved

ckeshava and others added 3 commits July 8, 2024 13:41

Update CHANGELOG.md

bb62029

Co-authored-by: Mayukha Vadari <mvadari@gmail.com>

Update xrpl/asyncio/wallet/wallet_generation.py

3881c7b

Co-authored-by: Mayukha Vadari <mvadari@gmail.com>

Use a global dict map between network_id and URL

0839082

mvadari reviewed Jul 8, 2024

View reviewed changes

mvadari approved these changes Jul 8, 2024

View reviewed changes

address PR comments

fe616ec

ckeshava requested a review from khancode July 17, 2024 20:55

Merge branch 'main' into faucet_url_logic

e346219

anissa-ripple approved these changes Aug 23, 2024

View reviewed changes

Merge branch 'main' into faucet_url_logic

c30f772

mvadari linked an issue Sep 17, 2024 that may be closed by this pull request

Use Network ID to determine the correct faucet to use #710

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #710] Use network_id to identify faucet_url #715

[Fix #710] Use network_id to identify faucet_url #715

ckeshava commented Jun 12, 2024

mvadari Jun 25, 2024

ckeshava Jun 28, 2024

mvadari Jun 28, 2024 •

edited

Loading

ckeshava Jun 28, 2024

mvadari Jul 1, 2024

ckeshava Jul 1, 2024

mvadari Jul 3, 2024 •

edited

Loading

ckeshava Jul 3, 2024

mvadari Jul 3, 2024

ckeshava Jul 8, 2024

mvadari Jul 3, 2024 •

edited

Loading

ckeshava Jul 3, 2024

ckeshava Jul 3, 2024

mvadari Jul 3, 2024

ckeshava Jul 3, 2024

mvadari Jul 3, 2024

ckeshava Jul 3, 2024

ckeshava Jul 8, 2024

mvadari Jul 3, 2024 •

edited

Loading

ckeshava Jul 3, 2024

ckeshava Jul 8, 2024

ckeshava Jul 3, 2024

mvadari Jul 5, 2024

ckeshava Jul 8, 2024

mvadari Jul 5, 2024

ckeshava Jul 8, 2024

mvadari Jul 5, 2024

ckeshava Jul 8, 2024

mvadari Jul 8, 2024

mvadari Jul 8, 2024

		elif network_id < 0:
		raise XRPLFaucetException("network_id cannot be negative.")

		if faucet_host is not None:
		return process_faucet_host_url(faucet_host)

	_MAP_NETWORK_ID_TO_URL: Dict[int, str] = {1: _TEST_FAUCET_URL, 2: _DEV_FAUCET_URL}
	_NETWORK_ID_URL_MAP: Dict[int, str] = {1: _TEST_FAUCET_URL, 2: _DEV_FAUCET_URL}

[Fix #710] Use network_id to identify faucet_url #715

Are you sure you want to change the base?

[Fix #710] Use network_id to identify faucet_url #715

Conversation

ckeshava commented Jun 12, 2024

High Level Overview of Change

Context of Change

Type of Change

Did you update CHANGELOG.md?

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari Jun 28, 2024 •

edited

Loading

mvadari Jul 3, 2024 •

edited

Loading

mvadari Jul 3, 2024 •

edited

Loading

mvadari Jul 3, 2024 •

edited

Loading