Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verifiable Credentials #759

Open
wants to merge 78 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
c7e1a15
Update definitions.json -- generated from the tip of rippled codebase…
ckeshava Oct 15, 2024
9d549c1
Specify the CredentialCreate transaction
ckeshava Oct 16, 2024
8375c42
Files relevant to the CredentialAccept transaction
ckeshava Oct 17, 2024
8d810fc
Files pertaining to the CredentialDelete transaction
ckeshava Oct 17, 2024
0a33121
Files pertaining to DepositPreauth transaction are included in this c…
ckeshava Oct 18, 2024
fd8157d
FIX: Update deposit_preauth integration tests to validate the transac…
ckeshava Oct 18, 2024
1d85bbb
Include account_objects RPC call to verify that cred ledger-object is…
ckeshava Oct 21, 2024
8ea12c4
Updates to DepositAuthorized RPC
ckeshava Oct 21, 2024
e4293be
address PR comments
ckeshava Oct 21, 2024
60b7663
FIX: Use sum() method for error validation in DepositPreauth transact…
ckeshava Oct 22, 2024
509ecd1
Update the definitions.json file without changing the format of the d…
ckeshava Oct 22, 2024
c0151c2
Update base_model.py -- Use mypy with Python 3.8 suggestions
ckeshava Oct 22, 2024
48b8abc
include fixAMM1_1 in the rippled.cfg file
ckeshava Oct 24, 2024
f476a51
rearrange the LedgerEntryTypes in definitions.json
ckeshava Oct 24, 2024
8b82c3d
include amendments which are not in majority consensus yet
ckeshava Oct 25, 2024
a24ca5b
Update tests/unit/models/transactions/test_account_delete.py
ckeshava Oct 30, 2024
4402f6d
Update tests/unit/models/transactions/test_credential_accept.py
ckeshava Oct 30, 2024
22de7a3
refactor DepositPreauth transaction validity checks
ckeshava Oct 30, 2024
f848bbd
refactor suggestions from code-rabbit
ckeshava Oct 30, 2024
cb63c6d
Merge branch 'main' into cred
ckeshava Oct 30, 2024
ffcf1bd
Update xrpl/models/transactions/deposit_preauth.py
ckeshava Oct 31, 2024
e498721
Update xrpl/models/transactions/payment_channel_claim.py
ckeshava Oct 31, 2024
e5a0779
Update xrpl/models/transactions/payment_channel_claim.py
ckeshava Oct 31, 2024
ad4331a
remove unnecessary comments
ckeshava Oct 31, 2024
1a974fb
update error msg in CredentialDelete txn
ckeshava Oct 31, 2024
880a670
Update xrpl/models/transactions/credential_accept.py
ckeshava Oct 31, 2024
af67d78
Update xrpl/models/transactions/credential_accept.py
ckeshava Oct 31, 2024
c50d611
Update xrpl/models/transactions/credential_accept.py
ckeshava Oct 31, 2024
2c887aa
Update xrpl/models/transactions/credential_create.py
ckeshava Oct 31, 2024
8468fcd
Update xrpl/models/transactions/credential_create.py
ckeshava Oct 31, 2024
bba054d
Update xrpl/models/transactions/credential_create.py
ckeshava Oct 31, 2024
6c0b958
Update xrpl/models/transactions/credential_create.py
ckeshava Oct 31, 2024
7f68440
Update xrpl/models/transactions/credential_create.py
ckeshava Oct 31, 2024
1485ae9
Update xrpl/models/transactions/credential_create.py
ckeshava Oct 31, 2024
27f8719
Update xrpl/models/transactions/credential_accept.py
ckeshava Oct 31, 2024
485d392
modified the structure of error collection
ckeshava Oct 31, 2024
562aa49
modified error msg in DepositPreauth txn
ckeshava Oct 31, 2024
d83f0d7
use _MAX_URI_LENGTH variable
ckeshava Oct 31, 2024
a188c1a
Update all the unit tests as per the changes in model files
ckeshava Oct 31, 2024
b97d6f3
Use List instead of Set to store sequence of credentials
ckeshava Oct 31, 2024
379620a
simplify DepositPreauth unit tests
ckeshava Oct 31, 2024
e63b49d
unify the regex used in DIDSet and Credentials
ckeshava Oct 31, 2024
5425b37
remove the use of lambda in collecting errors
ckeshava Oct 31, 2024
3e8b825
Update xrpl/models/transactions/account_delete.py
ckeshava Nov 1, 2024
4106236
Use variables instead of magic numbers, credential length checks
ckeshava Nov 1, 2024
6e42798
remove magic number in credential_delete txn model
ckeshava Nov 1, 2024
0719107
use filter construct for deposit_preauth validity check
ckeshava Nov 1, 2024
f1d2b4e
remove doc-strings alluding to de-duplication
ckeshava Nov 1, 2024
bf5114a
refactor credential_ids check into utils file
ckeshava Nov 1, 2024
6d8f1be
Update xrpl/models/transactions/escrow_finish.py
ckeshava Nov 4, 2024
8e496e1
address PR comments from Mayukha
ckeshava Nov 4, 2024
5656080
Update tests/integration/transactions/test_credential.py
ckeshava Nov 4, 2024
b43aa12
Update xrpl/models/utils.py
ckeshava Nov 4, 2024
6392351
Update xrpl/models/transactions/credential_create.py
ckeshava Nov 4, 2024
4ad028b
Update xrpl/models/transactions/credential_delete.py
ckeshava Nov 4, 2024
1f25b01
Update xrpl/models/transactions/deposit_preauth.py
ckeshava Nov 4, 2024
a870f2d
Merge branch 'main' into cred
ckeshava Nov 4, 2024
a6ac0f4
address PR comments; check for duplicates in the credential list
ckeshava Nov 7, 2024
111e12a
Update xrpl/models/transactions/credential_create.py
ckeshava Nov 7, 2024
8593111
Update xrpl/models/transactions/credential_delete.py
ckeshava Nov 7, 2024
7fa47e1
Update xrpl/models/transactions/credential_create.py
ckeshava Nov 7, 2024
3d66a36
address PR comments
ckeshava Nov 7, 2024
2d86ea2
address PR comments
ckeshava Nov 8, 2024
f58dc2c
update definitions.json to match rippled develop branch
ckeshava Nov 9, 2024
0f7f745
use short-circuit in an if-condition
ckeshava Nov 9, 2024
b02f902
[WIP] Fix CICD integration tests
ckeshava Nov 12, 2024
b70ac84
Merge branch 'main' into cred
ckeshava Nov 12, 2024
7f79596
CI/CD Fix: Update docker-stop command
ckeshava Nov 12, 2024
3814a94
LedgerEntry RPC: Introduce Credential functionality
ckeshava Nov 12, 2024
c721a87
Update Contributing guidelines
ckeshava Nov 12, 2024
c76b755
fix black linter errors
ckeshava Nov 12, 2024
9b7d17c
fix mypy errors
ckeshava Nov 12, 2024
5068266
re-introduce docker health checks with server_info RPC
ckeshava Nov 12, 2024
c7a6e2e
remove extraneous comments
ckeshava Nov 15, 2024
344bb49
[WIP] use single docker run command
ckeshava Nov 15, 2024
ee01fc9
[WIP] Revert some changes to the CICD integration tests
ckeshava Nov 15, 2024
09b7d18
[refactor] address Ashray PR suggestion
ckeshava Nov 15, 2024
87d2af7
update release notes in CHANGELOG
ckeshava Nov 15, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .ci-config/rippled.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,13 @@ PriceOracle
fixEmptyDID
fixXChainRewardRounding
fixPreviousTxnID
fixAMMv1_1
Credentials
mvadari marked this conversation as resolved.
Show resolved Hide resolved
NFTokenMintOffer
fixNFTokenPageLinks
fixInnerObjTemplate2
fixEnforceNFTokenTrustline
fixReducedOffersV2

# This section can be used to simulate various FeeSettings scenarios for rippled node in standalone mode
[voting]
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/integration_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: Integration test

env:
POETRY_VERSION: 1.8.3
RIPPLED_DOCKER_IMAGE: rippleci/rippled:2.2.0-b3
RIPPLED_DOCKER_IMAGE: rippleci/rippled:2.3.0-rc1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Use a stable rippled version (2.3.0) instead of a release candidate

The workflow is currently using a release candidate version (2.3.0-rc1). It's recommended to use the stable 2.3.0 release to ensure stability in CI.

🔗 Analysis chain

Verify the rippled version requirement

The workflow now uses a release candidate version (2.3.0-rc1). While this might be necessary for the new Verifiable Credentials feature, using release candidates in CI can potentially introduce instability.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the rippled version is explicitly required by searching for version-specific dependencies or features

# Search for version requirements in documentation and configuration
rg -g '!.git' -g '*.{md,rst,txt,json,yml,yaml}' '2\.3\.0|rippled.*version'

# Check Dockerfile references
fd Dockerfile | xargs rg 'rippled.*version|2\.3\.0'

Length of output: 600


on:
push:
Expand Down Expand Up @@ -32,7 +32,7 @@ jobs:

- name: Run docker in background
run: |
docker run --detach --rm --name rippled-service -p 5005:5005 -p 6006:6006 --volume "${{ github.workspace }}/.ci-config/":"/opt/ripple/etc/" --health-cmd="wget localhost:6006 || exit 1" --health-interval=5s --health-retries=10 --health-timeout=2s --env GITHUB_ACTIONS=true --env CI=true ${{ env.RIPPLED_DOCKER_IMAGE }} /opt/ripple/bin/rippled -a --conf /opt/ripple/etc/rippled.cfg
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why all the changes here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I have updated the docker image to use the latest tip from develop branch
  • I'm given to understand that there are some updates to the entrypoint.sh bash file in the docker image. In consultation with the @micheal legleux, I've updated the docker run command accordingly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lastly, I've updated the mount point of the docker container. We don't need to explicitly specify the conf file anymore, its already placed into the default location that is searched by rippled

docker run --detach --rm -p 5005:5005 -p 6006:6006 --volume "${{ github.workspace }}/.ci-config/":/etc/opt/ripple/ --name rippled-service --health-cmd="rippled server_info || exit 1" --health-interval=5s --health-retries=10 --health-timeout=2s --env GITHUB_ACTIONS=true --env CI=true --entrypoint bash ${{ env.RIPPLED_DOCKER_IMAGE }} -c "rippled -a"

- name: Install poetry
if: steps.cache-poetry.outputs.cache-hit != 'true'
Expand Down
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added
- Add `include_deleted` to ledger_entry request
- Add support for XLS-70d Verifiable Credentials in xrpl-py library.
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance the changelog entry with more details about XLS-70d implementation.

The current entry could be more detailed given the significance of this feature. Consider expanding it to include:

  • New transaction types (CredentialCreate, CredentialAccept, CredentialDelete)
  • New Credential ledger object
  • Modifications to DepositPreauth ledger object
-Add support for XLS-70d Verifiable Credentials in xrpl-py library.
+Add support for XLS-70d Verifiable Credentials in xrpl-py library:
+  - Add new transaction types: CredentialCreate, CredentialAccept, CredentialDelete
+  - Add new Credential ledger object
+  - Update DepositPreauth ledger object with credential support
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- Add support for XLS-70d Verifiable Credentials in xrpl-py library.
Add support for XLS-70d Verifiable Credentials in xrpl-py library:
- Add new transaction types: CredentialCreate, CredentialAccept, CredentialDelete
- Add new Credential ledger object
- Update DepositPreauth ledger object with credential support

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 12 needs to begin with a - character to represent a bullet point in the markdown file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckeshava, thank you for catching that. You're absolutely right—the line should start with a - to represent a bullet point in the markdown file. Here's the updated changelog entry:

-Add support for XLS-70d Verifiable Credentials in xrpl-py library.
+- Add support for XLS-70d Verifiable Credentials in xrpl-py library:
+  - Add new transaction types: CredentialCreate, CredentialAccept, CredentialDelete
+  - Add new Credential ledger object
+  - Update DepositPreauth ledger object with credential support

Let me know if there's anything else I can assist with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rabbit can you present a "Commitable Suggestion" with this updated change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


### BREAKING CHANGE:
- Remove Python 3.7 support to fix dependency installation and use 3.8 as new default.
Expand Down Expand Up @@ -91,7 +92,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [2.0.0] - 2023-07-05
### BREAKING CHANGE
- The default signing algorithm in the `Wallet` was changed from secp256k1 to ed25519
-
-
### Added:
- Wallet support for regular key compatibility
- Added new ways of wallet generation: `from_seed`, `from_secret`, `from_entropy`, `from_secret_numbers`
Expand Down
10 changes: 5 additions & 5 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,17 @@ poetry run poe test_unit
To run integration tests, you'll need a standalone rippled node running with WS port `6006` and JSON RPC port `5005`. You can run a docker container for this:

```bash
docker run -p 5005:5005 -p 6006:6006 --interactive -t --volume $PWD/.ci-config:/opt/ripple/etc/ --platform linux/amd64 rippleci/rippled:2.2.0-b3 /opt/ripple/bin/rippled -a --conf /opt/ripple/etc/rippled.cfg
docker run -dit -p 5005:5005 -p 6006:6006 --volume $PWD/.ci-config/:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:develop -c 'rippled -a'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance Docker command robustness and security

The current Docker command could benefit from additional security and reliability improvements:

  1. Add user specification to avoid running as root
  2. Include health check to ensure rippled is ready
  3. Add resource limits to prevent container from consuming excessive resources
  4. Add automatic cleanup with --rm flag

Consider this improved version:

-docker run -dit -p 5005:5005 -p 6006:6006 --volume $PWD/.ci-config/:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:develop -c 'rippled -a'
+# Start the rippled container with improved security and reliability
+docker run -dit \
+    --rm \
+    --user "$(id -u):$(id -g)" \
+    --memory="4g" \
+    --memory-swap="4g" \
+    --cpu-shares=1024 \
+    -p 5005:5005 -p 6006:6006 \
+    --volume "$PWD/.ci-config/":/etc/opt/ripple/:ro \
+    --health-cmd="rippled server_info" \
+    --health-interval=5s \
+    --health-retries=3 \
+    rippleci/rippled:develop \
+    sh -c 'rippled -a && while true; do sleep 30; done'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
docker run -dit -p 5005:5005 -p 6006:6006 --volume $PWD/.ci-config/:/etc/opt/ripple/ --entrypoint bash rippleci/rippled:develop -c 'rippled -a'
# Start the rippled container with improved security and reliability
docker run -dit \
--rm \
--user "$(id -u):$(id -g)" \
--memory="4g" \
--memory-swap="4g" \
--cpu-shares=1024 \
-p 5005:5005 -p 6006:6006 \
--volume "$PWD/.ci-config/":/etc/opt/ripple/:ro \
--health-cmd="rippled server_info" \
--health-interval=5s \
--health-retries=3 \
rippleci/rippled:develop \
sh -c 'rippled -a && while true; do sleep 30; done'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This no longer matches above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is an intentional change. please check my comment on the integration_test.yml CI/CD file

```

Breaking down the command:
* `docker run -p 5005:5005 -p 6006:6006` starts a Docker container with an open port for admin JsonRPC and WebSocket requests.
* `--interactive` allows you to interact with the container.
* `-it` allows you to interact with the container.
* `-d` runs the docker container in detached mode. The container will run in the background and developer gets back control of the terminal
* `-t` starts a terminal in the container for you to send commands to.
* `--volume $PWD/.ci-config:/config/` identifies the `rippled.cfg` and `validators.txt` to import. It must be an absolute path, so we use `$PWD` instead of `./`.
* `xrpllabsofficial/xrpld:1.12.0` is an image that is regularly updated with the latest `rippled` releases and can be found here: https://github.com/WietseWind/docker-rippled
* `--volume $PWD/.ci-config:/etc/opt/ripple/` mounts the directories as indicated. It must be an absolute path, so we use `$PWD` instead of `./`. `rippled` software searches the location `/etc/opt/ripple/` (default behavior) for the config files. Hence there is no need to explicitly specify the config-file path.
* `rippleci/rippled:develop` is an image that is regularly updated with the latest build of the `develop` branch of `rippled`.
* `-a` starts `rippled` in standalone mode
* `--start` signals to start `rippled` with the specified amendments in `rippled.cfg` enabled immediately instead of voting for 2 weeks on them.

Then to actually run the tests, run the command:

Expand Down
121 changes: 121 additions & 0 deletions tests/integration/transactions/test_credential.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
from tests.integration.integration_test_case import IntegrationTestCase
from tests.integration.it_utils import (
sign_and_reliable_submission_async,
test_async_and_sync,
)
from tests.integration.reusable_values import DESTINATION, WALLET
from xrpl.models import AccountObjects, AccountObjectType
from xrpl.models.requests.ledger_entry import Credential, LedgerEntry
from xrpl.models.response import ResponseStatus
from xrpl.models.transactions.credential_accept import CredentialAccept
from xrpl.models.transactions.credential_create import CredentialCreate
from xrpl.models.transactions.credential_delete import CredentialDelete
from xrpl.utils import str_to_hex

_URI = "www.my-id.com/username"


def is_cred_object_present(
result: dict, issuer: str, subject: str, cred_type: str
) -> bool:
"""
Args:
result: JSON response from account_objects RPC
issuer: Address of the credential issuer
subject: Address of the credential subject
cred_type: Type of the credential

Returns:
bool: True if credential exists, False otherwise
"""

for val in result["account_objects"]:
if (
val["Issuer"] == issuer
and val["Subject"] == subject
and val["CredentialType"] == cred_type
):
return True
mvadari marked this conversation as resolved.
Show resolved Hide resolved

return False


class TestCredentials(IntegrationTestCase):
@test_async_and_sync(globals())
async def test_valid(self, client):
# Define entities helpful for Credential lifecycle
_ISSUER = WALLET.address
_SUBJECT = DESTINATION.address

# Disambiguate the sync/async, json/websocket tests with different
# credential type values -- this avoids tecDUPLICATE error
# self.value is defined inside the above decorator
cred_type = str_to_hex("Passport_" + str(self.value))

tx = CredentialCreate(
account=_ISSUER,
subject=_SUBJECT,
credential_type=cred_type,
uri=str_to_hex(_URI),
)
response = await sign_and_reliable_submission_async(tx, WALLET, client)
self.assertEqual(response.status, ResponseStatus.SUCCESS)
self.assertEqual(response.result["engine_result"], "tesSUCCESS")

mvadari marked this conversation as resolved.
Show resolved Hide resolved
# Use the LedgerEntry RPC to validate the creation of the credential object
ledger_entry_response = await client.request(
LedgerEntry(
credential=Credential(
subject=_SUBJECT, issuer=_ISSUER, credential_type=cred_type
)
)
)

self.assertEqual(ledger_entry_response.status, ResponseStatus.SUCCESS)

# Execute the CredentialAccept transaction on the above Credential ledger object
tx = CredentialAccept(
issuer=_ISSUER, account=_SUBJECT, credential_type=cred_type
)
# CredentialAccept transaction is submitted by SUBJECT
response = await sign_and_reliable_submission_async(tx, DESTINATION, client)
self.assertEqual(response.status, ResponseStatus.SUCCESS)
self.assertEqual(response.result["engine_result"], "tesSUCCESS")

# Execute the CredentialDelete transaction
# Subject initiates the deletion of the Credential ledger object
tx = CredentialDelete(
issuer=_ISSUER, account=_SUBJECT, credential_type=cred_type
)

response = await sign_and_reliable_submission_async(tx, DESTINATION, client)
self.assertEqual(response.status, ResponseStatus.SUCCESS)
self.assertEqual(response.result["engine_result"], "tesSUCCESS")

# The credential ledger object must be deleted from both the Issuer and Subject
# account's directory pages
account_objects_response = await client.request(
AccountObjects(account=_ISSUER, type=AccountObjectType.CREDENTIAL)
)
self.assertFalse(
is_cred_object_present(
account_objects_response.result,
issuer=_ISSUER,
subject=_SUBJECT,
cred_type=cred_type,
)
)

# Verify that the Credential object has been deleted from the Subject's
# directory page as well
account_objects_response = await client.request(
AccountObjects(account=_SUBJECT, type=AccountObjectType.CREDENTIAL)
)
self.assertFalse(
is_cred_object_present(
account_objects_response.result,
issuer=_ISSUER,
subject=_SUBJECT,
cred_type=cred_type,
)
)
Comment on lines +95 to +121
Copy link
Collaborator

@mvadari mvadari Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is unnecessary. We're testing the library, not rippled. Checking just one is more than enough. You could even just check for whether the object exists with ledger_entry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The choice between ledger_entry and account_objects is inconsequential, because both of them refer to the same ledger object.

There is no harm in additional check for the existence/deletion of ledger objects. It serves to explain the behavior of credentials better.

44 changes: 38 additions & 6 deletions tests/integration/transactions/test_deposit_preauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,65 @@
sign_and_reliable_submission_async,
test_async_and_sync,
)
from tests.integration.reusable_values import WALLET
from tests.integration.reusable_values import DESTINATION, WALLET
from xrpl.models.response import ResponseStatus
from xrpl.models.transactions import DepositPreauth
from xrpl.models.transactions.deposit_preauth import Credential
from xrpl.utils import str_to_hex

ACCOUNT = WALLET.address
ADDRESS = "rEhxGqkqPPSxQ3P25J66ft5TwpzV14k2de"


class TestDepositPreauth(IntegrationTestCase):
@test_async_and_sync(globals())
async def test_authorize(self, client):
async def test_authorize_unauthorize_fields(self, client):
deposit_preauth = DepositPreauth(
account=ACCOUNT,
authorize=ADDRESS,
authorize=DESTINATION.address,
)
response = await sign_and_reliable_submission_async(
deposit_preauth, WALLET, client
)
self.assertEqual(response.status, ResponseStatus.SUCCESS)
self.assertEqual(response.result["engine_result"], "tesSUCCESS")

# validate the un-authorization of the same address as above
deposit_preauth = DepositPreauth(
account=ACCOUNT,
unauthorize=DESTINATION.address,
)
response = await sign_and_reliable_submission_async(
deposit_preauth, WALLET, client
)
self.assertEqual(response.status, ResponseStatus.SUCCESS)
self.assertEqual(response.result["engine_result"], "tesSUCCESS")

@test_async_and_sync(globals())
async def test_unauthorize(self, client):
async def test_credentials_array_input_fields(self, client):
sample_credentials = [
Credential(
issuer=DESTINATION.address, credential_type=str_to_hex("SampleCredType")
)
]

# Test the authorize_credentials input field
deposit_preauth = DepositPreauth(
account=ACCOUNT,
authorize_credentials=sample_credentials,
)
response = await sign_and_reliable_submission_async(
deposit_preauth, WALLET, client
)
self.assertEqual(response.status, ResponseStatus.SUCCESS)
self.assertEqual(response.result["engine_result"], "tesSUCCESS")

# Test the unauthorize_credentials input field
deposit_preauth = DepositPreauth(
account=ACCOUNT,
unauthorize=ADDRESS,
unauthorize_credentials=sample_credentials,
)
response = await sign_and_reliable_submission_async(
deposit_preauth, WALLET, client
)
self.assertEqual(response.status, ResponseStatus.SUCCESS)
self.assertEqual(response.result["engine_result"], "tesSUCCESS")
mvadari marked this conversation as resolved.
Show resolved Hide resolved
15 changes: 15 additions & 0 deletions tests/unit/models/requests/test_deposit_authorized.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
from unittest import TestCase

from xrpl.models import DepositAuthorized


class TestDepositAuthorized(TestCase):
def test_valid(self):
req = DepositAuthorized(
source_account="srcAccount",
destination_account="dstAccount",
credentials=[
"EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A"
],
)
self.assertTrue(req.is_valid())
mvadari marked this conversation as resolved.
Show resolved Hide resolved
19 changes: 18 additions & 1 deletion tests/unit/models/requests/test_ledger_entry.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from xrpl.models import XRP, LedgerEntry, XChainBridge
from xrpl.models.exceptions import XRPLModelException
from xrpl.models.requests.ledger_entry import Oracle, RippleState
from xrpl.models.requests.ledger_entry import Credential, Oracle, RippleState


class TestLedgerEntry(TestCase):
Expand All @@ -18,6 +18,23 @@ def test_has_only_account_root_is_valid(self):
)
self.assertTrue(req.is_valid())

def test_query_credential_object_id(self):
self.assertTrue(
LedgerEntry(
credential="EA85602C1B41F6F1F5E83C0E6B87142FB8957B"
"D209469E4CC347BA2D0C26F66A"
).is_valid()
)

def test_query_credential_by_object_params(self):
self.assertTrue(
LedgerEntry(
credential=Credential(
subject="rSubject", issuer="rIssuer", credential_type="ABCDE"
)
).is_valid()
)
Comment on lines +29 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Strengthen credential object parameter testing

The test should be more comprehensive to ensure robust credential validation:

  1. Add docstring documenting parameter requirements
  2. Verify all object properties after construction
  3. Add negative test cases for invalid parameters
  4. Use realistic test data that matches the XLS-0070d standard

Suggested implementation:

 def test_query_credential_by_object_params(self):
+    """
+    Test LedgerEntry initialization with a Credential object.
+    
+    Requirements:
+    - subject: valid XRPL account address
+    - issuer: valid XRPL account address
+    - credential_type: string matching XLS-0070d standard types
+    """
+    subject = "rN7n7otQDd6FczFgLdSqtcsAUxDkw6fzRH"
+    issuer = "rPMh7Pi9ct699iZUTWaytJUoHcJ7cgyziK"
+    credential_type = "KYC_CREDENTIAL"
+    
+    ledger_entry = LedgerEntry(
+        credential=Credential(
+            subject=subject,
+            issuer=issuer,
+            credential_type=credential_type
+        )
+    )
+    
+    # Verify valid case
+    self.assertTrue(ledger_entry.is_valid())
+    self.assertEqual(ledger_entry.credential.subject, subject)
+    self.assertEqual(ledger_entry.credential.issuer, issuer)
+    self.assertEqual(ledger_entry.credential.credential_type, credential_type)
+    
+    # Negative test cases
+    with self.assertRaises(XRPLModelException):
+        LedgerEntry(
+            credential=Credential(
+                subject="invalid_address",
+                issuer=issuer,
+                credential_type=credential_type
+            )
+        )
+    
+    with self.assertRaises(XRPLModelException):
+        LedgerEntry(
+            credential=Credential(
+                subject=subject,
+                issuer="invalid_address",
+                credential_type=credential_type
+            )
+        )

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +21 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Additional credential test scenarios needed

The suggested credential scenarios are not fully covered in existing tests. Consider adding the following tests in tests/unit/models/requests/test_ledger_entry.py:

  • Test combining credential with other LedgerEntry fields (should be invalid)
  • Test credential field mutations after object creation
  • Test serialization/deserialization of credential objects
🔗 Analysis chain

Consider additional test scenarios

While the basic credential functionality is tested, consider adding these scenarios:

  1. Test combining credential with other LedgerEntry fields (should be invalid)
  2. Test credential field mutations after object creation
  3. Test serialization/deserialization of credential objects

Let's check if these scenarios are covered elsewhere:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other credential-related tests
rg -l "credential.*test" tests/

Length of output: 33


Script:

#!/bin/bash
# Comprehensive search for any credential-related tests
rg "credential" tests/

Length of output: 12591


def test_has_only_directory_is_valid(self):
req = LedgerEntry(
directory="hello",
Expand Down
60 changes: 60 additions & 0 deletions tests/unit/models/transactions/test_account_delete.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
from unittest import TestCase

from xrpl.models.exceptions import XRPLModelException
from xrpl.models.transactions import AccountDelete

_ACCOUNT = "r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ"
_DESTINATION = "rf7HPydP4ihkFkSRHWFq34b4SXRc7GvPCR"


class TestAccountDelete(TestCase):
def test_creds_list_too_long(self):
"""Test that AccountDelete raises exception when credential_ids exceeds max
length."""
with self.assertRaises(XRPLModelException) as err:
AccountDelete(
account=_ACCOUNT,
destination=_DESTINATION,
credential_ids=["credential_index_" + str(i) for i in range(9)],
)

self.assertEqual(
err.exception.args[0],
"{'credential_ids': 'CredentialIDs list cannot have more than 8 elements."
+ "'}",
)

def test_creds_list_empty(self):
with self.assertRaises(XRPLModelException) as err:
AccountDelete(
account=_ACCOUNT,
destination=_DESTINATION,
credential_ids=[],
)
self.assertEqual(
err.exception.args[0],
"{'credential_ids': 'CredentialIDs list cannot be empty.'}",
)

def test_creds_list_duplicates(self):
with self.assertRaises(XRPLModelException) as err:
AccountDelete(
account=_ACCOUNT,
destination=_DESTINATION,
credential_ids=["credential_index" for _ in range(5)],
)
self.assertEqual(
err.exception.args[0],
"{'credential_ids_duplicates': 'CredentialIDs list cannot contain duplicate"
+ " values.'}",
)

def test_valid_account_delete_txn(self):
tx = AccountDelete(
account=_ACCOUNT,
destination=_DESTINATION,
credential_ids=[
"EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A"
],
)
self.assertTrue(tx.is_valid())
Comment on lines +52 to +60
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for credential ID format.

The test verifies basic functionality but should also validate the credential ID format:

     def test_valid_account_delete_txn(self):
+        """Test valid AccountDelete transaction with properly formatted credential ID.
+        
+        The credential ID should be a 32-byte (64 character) hexadecimal string.
+        """
         tx = AccountDelete(
             account=_ACCOUNT,
             destination=_DESTINATION,
             credential_ids=[
                 "EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A"
             ],
         )
         self.assertTrue(tx.is_valid())
+        self.assertEqual(len(tx.credential_ids[0]), 64)  # 32 bytes in hex
+        self.assertTrue(all(c in '0123456789ABCDEFabcdef' for c in tx.credential_ids[0]))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_valid_account_delete_txn(self):
tx = AccountDelete(
account=_ACCOUNT,
destination=_DESTINATION,
credential_ids=[
"EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A"
],
)
self.assertTrue(tx.is_valid())
def test_valid_account_delete_txn(self):
"""Test valid AccountDelete transaction with properly formatted credential ID.
The credential ID should be a 32-byte (64 character) hexadecimal string.
"""
tx = AccountDelete(
account=_ACCOUNT,
destination=_DESTINATION,
credential_ids=[
"EA85602C1B41F6F1F5E83C0E6B87142FB8957BD209469E4CC347BA2D0C26F66A"
],
)
self.assertTrue(tx.is_valid())
self.assertEqual(len(tx.credential_ids[0]), 64) # 32 bytes in hex
self.assertTrue(all(c in '0123456789ABCDEFabcdef' for c in tx.credential_ids[0]))

Loading