feat: add rippled API v2 support and use as default #720

ckeshava · 2024-07-01T18:47:46Z

High Level Overview of Change

All the requests are updated to use the newer rippled v2 API responses

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

Test Plan

Existing integration tests have been modified to use the newer API version responses.

* add nfts_by_issuer * update Changelog * added nfts_by_issuer to Request's get_method * reformat changelog and add required comment for nfts_by_issuer.py --------- Co-authored-by: Kassaking <kassaking7@gmail.com>

* BaseModel.from_xrpl method should not accept snake_case keys in dictionary input * Fix integration test: Update regex to accomodate Amount2 AMM field --------- Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

use the kw_only=True parameter in the dataclass module. Ensure backwards compatibility for older versions of Python, this feature is enabled in Python v3.10 or later versions --------- Co-authored-by: Jackson Mills <jmills@ripple.com> Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

…st rippled validator

mvadari · 2024-07-01T20:31:56Z

xrpl/models/requests/generic_request.py

@@ -62,7 +63,16 @@ def from_dict(cls: Type[GenericRequest], value: Dict[str, Any]) -> GenericReques

        elif "method" in value:  # JSON RPC formatting
            if "params" in value:  # actual JSON RPC formatting
-                value = {"method": value["method"], **value["params"]}
+                if "api_version" in value:


Why does the API version need to be specified in generic requests specifically?

Certain requests like tx_history are implemented through GenericRequest. But this request ahs been deprecated since api version 2. This PR intends to make API v2 as the default response in xrpl-py.

An api_version < 2 needs to be specified for certain generic-requests, otherwise they fail with an invalid_API_version error.

Look at test_generic_request for an example

Seems fine to leave that as existing behavior then - default to v2. For example, GenericRequest is also used for ledger_accept, which doesn't differ between V1 and V2.

Yes, but api_version: 2 is specified in all requests, by default (including ledger_accept).

Without this change, tx_history requests are transmitted with api_version: 2 and they fail. Since we need to support API v1, how do you propose to handle tx_history requests?

It should error if api_version isn't included, since the default is 2. If api_version: 1 is included, it will work. I assume that's what would still happen.

Note: we don't have to specifically support tx_history, that was just something to use for the test since the RPC doesn't have a model.

You don't need the api_version if check for that. I tried running just this code in the test and it worked:

if "params" in value: # actual JSON RPC formatting value = { "api_version": value["api_version"], "method": value["method"], **value["params"], }

what if a user submits a raw JSON input, but forgets to specify the api_version?

Since the user is not using the GenericRequest/Request class, the api_version field will need to be specified manually

The user can't submit raw JSON input in xrpl-py, you have to go through a model.

Even if you could, it would then default to whatever the rippled instance has as default (probably v1).

I added a test case at the end of this file: tests/integration/reqs/test_generic_request.py

Users can submit raw-JSON inputs through the from_dict methods. This request does not reach rippled due to processing errors in the from_dict function.

We will need to document that api_version is a mandatory field, if raw-JSON input is provided. If GenericRequest model is used, then the default api_version is provided automatically.

It's not a mandatory field if a default option is provided. The same is true of any other API method - if you don't specify an api_version field, you'll get the default version. We don't need to document anything specific for GenericRequest that's different than any other request model.

mvadari · 2024-07-01T20:32:25Z

xrpl/models/response.py

 from xrpl.models.base_model import BaseModel
 from xrpl.models.required import REQUIRED
 from xrpl.models.transactions import PaymentFlag
 from xrpl.models.transactions.types import TransactionType
 from xrpl.models.utils import KW_ONLY_DATACLASS, require_kwargs_on_init

+DEFAULT_API_VERSION: Final[int] = 2


Why is this defined in this file, where it's not used, instead of request.py?

I was looking for a global constants file, but I couldn't find it.
I've updated the location to requests.py file 👍

.ci-config/rippled.cfg

CHANGELOG.md

khancode · 2024-07-02T12:23:38Z

Type of Change should be set to "Breaking change" since response data has changed. Also can you change PR title to: "feat: add rippled API v2 support and use as default"?

Co-authored-by: Omar Khan <khancodegt@gmail.com>

…pi_version field

mvadari · 2024-07-05T15:07:31Z

tests/integration/sugar/test_network_id.py

-
-
-# TODO: move to test_transaction and use the standard integration test setup
-class TestNetworkID(TestCase):


Why is this file removed?

I have moved this test into test_transaction.py file

We're no longer testing both scenarios (network ID < 1024 vs network ID >= 1024), then.

For the cases where network_ID <= 1024, we don't need to populate network_id field inside transactions.

Hence, we don't need to test this function for those networks

It will error if NetworkID is specified, in those cases. So that scenario does still need to be tested.

mvadari · 2024-07-05T15:09:23Z

This PR should be merged into a different branch, called something like 3.0, since this is going to be in a major version bump. That allows us to still merge/deploy other PRs in minor/patch bumps without waiting to be ready for a major version bump.

pdp2121 · 2024-07-08T19:34:19Z

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ### Added
 - Add support for the DeliverMax field in Payment transactions
+- Use rippled API v2 as default in requests


I think this should be at the top (or highlighted some how) since this is a major change. Other than that, LGTM.

It should be in a BREAKING CHANGE section.

fixed in 4179077

mvadari · 2024-07-08T19:36:11Z

.ci-config/rippled.cfg

@@ -105,6 +105,10 @@ validators.txt
 # Note: The version of rippled you use this config with must have an implementation for the amendments you attempt to enable or it will crash.
 # If you need the version of rippled to be more up to date, you may need to make a comment on this repo: https://github.com/WietseWind/docker-rippled

+# network_id is required otherwise it's read as None
+[network_id]
+63456


Why do we need a network ID?

This is related to the integration test: test_autofill_populate_networkid. Originally, this test was using the hooks-testnet for sending transactions. But it was decided that client libraries are not supporting (or) using hooks test-nets.

The docker container for the integration test uses this config file for startup.

The autofill method's control flow is contingent on network_id > 1025 for this particular test.

This change is not directly related to the API v2 updates

But it was decided that client libraries are not supporting (or) using hooks test-nets.

We can still use it for a test.

This change is not directly related to the API v2 updates

Why is this change included in this PR, then?

i believe the integration tests are better off without the dependency on hooks-test-network -- this particular function can be tested without making network calls to the hooks-network

I can remove this change from this PR

It'll be easier if that discussion can happen separately.

I have reverted the changes to the main branch. But it appears that the hooks testnet is using rippledv1.10 : https://github.com/XRPL-Labs/xrpld-hooks/blob/b13e7db18d1376d40cdbeeea849702b568324279/src/ripple/protocol/impl/BuildInfo.cpp#L36

Here is the Request and Response from hooks testnet validator

# request ServerInfo(method=<RequestMethod.SERVER_INFO: 'server_info'>, id=None, api_version=2) # response Response(status=<ResponseStatus.ERROR: 'error'>, result={'api_version': 2, 'error': 'invalid_API_version', 'id': 'RequestMethod.SERVER_INFO_174181', 'request': {'api_version': 2, 'command': 'server_info', 'id': 'RequestMethod.SERVER_INFO_174181'}, 'status': 'error', 'type': 'response'}, id='RequestMethod.SERVER_INFO_174181', type=<ResponseType.RESPONSE: 'response'>)

I'll try to find a fix tomorrow

mvadari · 2024-07-08T19:36:58Z

snippets/multisign.py

@@ -57,10 +58,10 @@
 if multisigned_tx_response.result["validated"]:
    print("The multisigned transaction was accepted by the ledger:")
    print(multisigned_tx_response)
-    if multisigned_tx_response.result["Signers"]:
+    if multisigned_tx_response.result["tx_json"]["Signers"]:


nit: this would be easier to read with a helper variable for multisigned_tx_response.result["tx_json"]["Signers"]

fixed in 4179077

tests/integration/reqs/test_generic_request.py

mvadari · 2024-07-08T19:39:05Z

xrpl/models/requests/request.py


 import xrpl.models.requests  # bare import to get around circular dependency
 from xrpl.models.base_model import BaseModel
 from xrpl.models.exceptions import XRPLModelException
 from xrpl.models.required import REQUIRED
 from xrpl.models.utils import KW_ONLY_DATACLASS, require_kwargs_on_init

+DEFAULT_API_VERSION: Final[int] = 2


Suggested change

DEFAULT_API_VERSION: Final[int] = 2

_DEFAULT_API_VERSION: Final[int] = 2

It's a helper variable that's only used here (other than in tests).

fixed in 4179077

mvadari · 2024-07-08T19:40:32Z

xrpl/models/requests/request.py

@@ -106,6 +108,13 @@ class Request(BaseModel):

    id: Optional[Union[str, int]] = None

+    api_version: int = DEFAULT_API_VERSION


There should be an additional test for another request that makes sure that the api_version field is handled properly - perhaps account_info since there is a significant difference in the shape of the output.

Agree with this

what exactly do you want to test about the api_version field?

Is this what you had in mind, something like test_populate_api_version_field ?

There should be an integration test that ensures that including an api_version param in a request type that is not GenericRequest still works correctly and properly returns the correct API version.

Doesn't a unit test suffice?

It is rippled's responsibility to return the correct api_version response, why should the client libraries test the response?

Even in the case of GenericRequest, I'm only forwarding the api_version input specified by the user. The integration test pertaining to tx_history tests the older versions of rippled. I have not tested the validity of responses of rippled

Doesn't a unit test suffice?

In this case, I'd like to ensure that it matches rippled compatibility, so an integration test is needed.

It is rippled's responsibility to return the correct api_version response, why should the client libraries test the response?

I'm not suggesting a test that actually fully tests the rippled response, but a test that ensures that when I put in api_version: 1 in my request model, it actually gives me the rippled API v1 response and not the rippled API v2 response (in which case there's something wrong with the implementation of api_version in the xrpl-py model). I'm not sure if there's a field that returns the API version of the response (EDIT: there only appears to be one if the api_version is specified, which IMO is not sufficient), but if there is, just checking that would also be sufficient.

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

… file

ckeshava and others added 8 commits July 1, 2024 11:44

use api_version:2 in all requests

5ee6232

Add nfts_by_issuer data type (XRPLF#707)

a5c6c95

* add nfts_by_issuer * update Changelog * added nfts_by_issuer to Request's get_method * reformat changelog and add required comment for nfts_by_issuer.py --------- Co-authored-by: Kassaking <kassaking7@gmail.com>

from_xrpl should not accept snake_case dict keys (XRPLF#694)

ad7d2ff

* BaseModel.from_xrpl method should not accept snake_case keys in dictionary input * Fix integration test: Update regex to accomodate Amount2 AMM field --------- Co-authored-by: Mayukha Vadari <mvadari@ripple.com>

update CL

41110fc

Merge branch 'main' into apiVersion

281d6f8

fix snippets tests

d8a3660

refactor test_networkID: Use a specific network_id for integration te…

9760d74

…st rippled validator

ckeshava requested review from khancode and pdp2121 July 1, 2024 20:19

mvadari reviewed Jul 1, 2024

View reviewed changes

remove extreneous comment

c31cb03

khancode reviewed Jul 2, 2024

View reviewed changes

.ci-config/rippled.cfg Outdated Show resolved Hide resolved

khancode reviewed Jul 2, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ckeshava and others added 2 commits July 2, 2024 10:54

Update CHANGELOG.md

57971ae

Co-authored-by: Omar Khan <khancodegt@gmail.com>

Update .ci-config/rippled.cfg

f74109b

Co-authored-by: Omar Khan <khancodegt@gmail.com>

ckeshava changed the title ~~use api_version:2 in all requests~~ feat: add rippled API v2 support and use as default Jul 2, 2024

address PR comments

bf27c3a

ckeshava requested review from mvadari and khancode July 2, 2024 18:18

ckeshava and others added 3 commits July 2, 2024 11:31

Merge branch 'main' into apiVersion

91089bd

refactor GenericRequest.from_dict: remove the check on existence of a…

d559b69

…pi_version field

Merge branch 'main' into apiVersion

9bf0ea9

mvadari reviewed Jul 5, 2024

View reviewed changes

fix the linter error

6f1d848

ckeshava changed the base branch from main to xrpl-pyv3.0 July 5, 2024 17:43

ckeshava changed the base branch from xrpl-pyv3.0 to v3.0 July 5, 2024 17:45

ckeshava requested a review from mvadari July 5, 2024 17:46

khancode approved these changes Jul 8, 2024

View reviewed changes

pdp2121 reviewed Jul 8, 2024

View reviewed changes

pdp2121 approved these changes Jul 8, 2024

View reviewed changes

mvadari reviewed Jul 8, 2024

View reviewed changes

tests/integration/reqs/test_generic_request.py Outdated Show resolved Hide resolved

mvadari reviewed Jul 8, 2024

View reviewed changes

pdp2121 self-requested a review July 8, 2024 19:51

ckeshava and others added 4 commits July 8, 2024 14:21

address PR comments

43a5a8b

use a leading "_" for DEFAULT_API_VERSION

4179077

Update tests/integration/reqs/test_generic_request.py

7594eb3

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

revert the changes pertaining to introducing network ID in the config…

f136db7

… file



		# TODO: move to test_transaction and use the standard integration test setup
		class TestNetworkID(TestCase):

	DEFAULT_API_VERSION: Final[int] = 2
	_DEFAULT_API_VERSION: Final[int] = 2

		@@ -106,6 +108,13 @@ class Request(BaseModel):

		id: Optional[Union[str, int]] = None

		api_version: int = DEFAULT_API_VERSION

feat: add rippled API v2 support and use as default #720

Are you sure you want to change the base?

feat: add rippled API v2 support and use as default #720

Conversation

ckeshava commented Jul 1, 2024 • edited Loading

High Level Overview 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

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 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khancode commented Jul 2, 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

mvadari commented Jul 5, 2024

Choose a reason for hiding this comment

mvadari Jul 8, 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

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 8, 2024 • edited Loading

Choose a reason for hiding this comment

ckeshava commented Jul 1, 2024 •

edited

Loading

mvadari Jul 3, 2024 •

edited

Loading

mvadari Jul 5, 2024 •

edited

Loading

khancode commented Jul 2, 2024 •

edited

Loading

mvadari Jul 8, 2024 •

edited

Loading

mvadari Jul 8, 2024 •

edited

Loading