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

Condition and fulfillment in escrow #698

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ckeshava
Copy link
Collaborator

@ckeshava ckeshava commented Mar 22, 2024

High Level Overview of Change

Provide utility functions to generate cryptographic condition and fulfillment for conditional escrows.

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

Included unit test and snippet test to validate the working of generate_crypto_conditions function.

This work was inspired by the PR by author and also the XRPL tutorials

Many thanks to both these sources.

Copy link
Contributor

coderabbitai bot commented Jan 2, 2025

Walkthrough

This pull request introduces functionality for creating conditional escrows in the XRPL Python library. The changes include adding a new utility function generate_escrow_cryptoconditions to generate cryptographic conditions and fulfillments for escrow transactions. A new dependency cryptoconditions is added to the project, and the escrow-related models and snippets are updated to support conditional escrows with additional parameters like condition and fulfillment. New tests are also implemented to validate these changes.

Changes

File Change Summary
CHANGELOG.md Added entry for new generate_escrow_cryptoconditions utility function
pyproject.toml Added cryptoconditions = "0.8.1" dependency
snippets/send_escrow.py Updated escrow creation to include cryptographic conditions and cancellation parameters
xrpl/models/transactions/escrow_create.py Added CryptoConditions class and generate_escrow_cryptoconditions function
tests/unit/models/transactions/test_escrow_create.py Added new test method test_escrow_condition_and_fulfillment

Possibly related PRs

Suggested reviewers

  • anissa-ripple
  • justinr1234

Poem

🐰 A Rabbit's Escrow Ballad 🔒
Crypto conditions, oh so neat,
Escrows now dance to a cryptic beat
Random bytes, a magical key
Unlocking funds with security
XRPL hops with joy today! 🚀


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (6)
snippets/send_escrow.py (1)

101-104: Grammar note on comment.
“of a conditional escrows” can be streamlined to “of conditional escrows” or “of a conditional escrow.”

-# The fees for EscrowFinish transaction of a conditional escrows are higher.
+# The fees for EscrowFinish transactions of conditional escrows are higher.
tests/unit/models/transactions/test_escrow_create.py (2)

8-9: Maintain naming consistency for test globals.
_OFFER_SEQUENCE and _OWNER are well-named, but consider grouping them in a test constants file if used widely.


33-72: Coverage for escrow condition and fulfillment path looks solid.

  • The test ensures that attempting to finalize an escrow without providing the fulfillment raises an exception.
  • Minor spelling correction in the comment: “fullfillment” → “fulfillment.”
-# EscrowFinish without the fullfillment must throw an error
+# EscrowFinish without the fulfillment must throw an error
xrpl/models/transactions/escrow_create.py (2)

8-9: cryptoconditions import is correct but note type ignore usage.
If you want mypy to work with cryptoconditions, you could add custom stubs or reference types.

Would you like a quick stub generation snippet or further guidance?


117-120: Remove commented-out code for clarity.
If the commented lines are no longer needed, removing them helps maintain readability.

CHANGELOG.md (1)

10-12: Grammar refinement in the changelog entry.
Replace “Add a utility function ... to generate ...” with a slightly more polished phrasing.

-### Added
-- Add a utility function `generate_escrow_cryptoconditions` to generate cryptographic condition and fulfillment for conditional escrows
+
+### Added
+- Added a utility function `generate_escrow_cryptoconditions` to generate a cryptographic condition and fulfillment for conditional escrows
🧰 Tools
🪛 LanguageTool

[grammar] ~10-1010: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...c/v2.0.0.html). ## [[Unreleased]] ### Added - Add a utility function `generate_escrow_cry...

(REPEATED_VERBS)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c6b489 and 93fc134.

⛔ Files ignored due to path filters (1)
  • poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • CHANGELOG.md (1 hunks)
  • pyproject.toml (1 hunks)
  • snippets/send_escrow.py (2 hunks)
  • tests/unit/models/transactions/test_escrow_create.py (2 hunks)
  • xrpl/models/transactions/escrow_create.py (2 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md

[grammar] ~10-1010: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...c/v2.0.0.html). ## [[Unreleased]] ### Added - Add a utility function `generate_escrow_cry...

(REPEATED_VERBS)

🔇 Additional comments (11)
snippets/send_escrow.py (7)

4-4: Use of os.urandom for cryptographic seeds looks appropriate.
This line correctly imports a secure randomness source for generating secrets. No concerns here.


70-73: Conditional escrow setup is clear.
These lines correctly introduce the conditional escrow snippet.


74-80: Ensure alignment of time-based parameters.
You are using cancel_after but not using finish_after. This may be intentional, but confirm if you want a strict finish time (like in the earlier, non-conditional escrow) in addition to a cancellation time.


82-84: Printing the escrow response is helpful for debugging.
No issues here.


85-91: AccountObjects request usage is consistent.
Fetching account objects to confirm the escrow’s existence is straightforward.


105-107: Balance checks after escrow finish are well-placed.
No issues found.


92-99: Consider consistency in referencing “Sequence”.
Earlier, you used create_escrow_response.result["tx_json"]["Sequence"] for offer_sequence. Here, you use create_escrow_response.result["Sequence"]. Confirm that the top-level key "Sequence" always exists.

tests/unit/models/transactions/test_escrow_create.py (2)

1-1: Use of os.urandom in tests is acceptable for cryptographic randomness.


5-6: Testing additional module imports.
Including both EscrowFinish and generate_escrow_cryptoconditions broadens the test coverage.

xrpl/models/transactions/escrow_create.py (1)

90-98: Well-structured TypedDict.
The CryptoConditions class is concise and descriptive for conditional escrow data.

pyproject.toml (1)

35-35: Validate cryptoconditions 0.8.1.
Ensure no known vulnerabilities or updated versions exist.

✅ Verification successful

cryptoconditions 0.8.1 is the current stable version with no known vulnerabilities

The package verification shows:

  • Version 0.8.1 is the latest available version on PyPI
  • No known vulnerabilities are reported
  • Package provides multi-algorithm, multi-level, multi-signature format for ILP conditions
  • No direct Python package dependencies are listed
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if cryptoconditions 0.8.1 is the latest stable version
curl -s https://pypi.org/pypi/cryptoconditions/json | jq '.info.version'

Length of output: 80


Script:

#!/bin/bash
# Check for known vulnerabilities in cryptoconditions package
curl -s https://pypi.org/pypi/cryptoconditions/0.8.1/json | jq '.vulnerabilities'

# Get release date and other metadata
curl -s https://pypi.org/pypi/cryptoconditions/0.8.1/json | jq '{upload_time: .releases["0.8.1"][0].upload_time, requires_python: .info.requires_python, summary: .info.summary}'

# Check dependencies
curl -s https://pypi.org/pypi/cryptoconditions/0.8.1/json | jq '.info.requires_dist'

Length of output: 562

from time import sleep

from xrpl.account import get_balance
from xrpl.clients import JsonRpcClient
from xrpl.models import AccountObjects, EscrowCreate, EscrowFinish
from xrpl.models.transactions.escrow_create import generate_escrow_cryptoconditions
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 2, 2025

Choose a reason for hiding this comment

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

💡 Codebase verification

Import is correctly structured, but function needs to be exposed

Based on the verification:

  • The generate_escrow_cryptoconditions function is defined in xrpl/models/transactions/escrow_create.py
  • The function is newly added according to the CHANGELOG.md
  • The function is not currently exposed in the package's public API through xrpl/models/transactions/__init__.py

To make this function properly accessible for external usage, it should be:

  1. Imported in xrpl/models/transactions/__init__.py
  2. Added to the __all__ list in the same file
🔗 Analysis chain

Verify import accessibility.
Ensure generate_escrow_cryptoconditions is properly exposed in the package’s __init__.py if needed for external usage.


Let me gather more information about the package structure and imports.


Let me check one more thing to verify if this is a newly added function.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if generate_escrow_cryptoconditions is imported in __init__.py or directly in other modules
rg --heading --context 5 "generate_escrow_cryptoconditions"

Length of output: 2311


Script:

#!/bin/bash
# Check if the function is exposed in __init__.py files
fd "__init__.py" | xargs rg "generate_escrow_cryptoconditions"

# Check the module structure
fd . --type f --extension py | grep -v "test" | grep "xrpl"

Length of output: 8835


Script:

#!/bin/bash
# Check the content of the models/transactions/__init__.py to see if the function is exposed
cat xrpl/models/transactions/__init__.py

Length of output: 6651

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Developers can use the new function as from xrpl.models.transactions.escrow_create import generate_escrow_cryptoconditions.

Is it really necessary to expose this function through the __init__.py file? Would that have any costs in terms of the size of the xrpl-py python package?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ckeshava, exposing functions in __init__.py does not significantly increase package size unless you're re-exporting many large modules. Typically, it’s more about how you want users to discover and import functionality. If you feel comfortable directing developers to escrow_create, you can certainly leave it out of __init__.py. However, adding a single function there will have a negligible impact on package size; it mainly offers discoverability and an implicit statement that this function is part of the “official” public API.

xrpl/models/transactions/escrow_create.py Outdated Show resolved Hide resolved
ckeshava and others added 2 commits January 2, 2025 14:21
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
snippets/send_escrow.py (3)

103-104: Enhance the fee documentation.

The comment about fees could be more specific to help users better understand the cost implications.

Consider updating the comment to:

-# The fees for EscrowFinish transaction of a conditional escrows are higher.
-# Additionally, the fees scale with the reference load on the server
+# Note on fees:
+# 1. Conditional escrow finish transactions have higher base fees than regular escrows
+# 2. The actual fee scales with the server load and condition complexity
+# 3. Recommended to check the fee before submitting using fee_margin parameter

93-101: Add validation before finishing the escrow.

The code should validate the escrow's existence and state before attempting to finish it.

Consider adding validation:

+# Verify the escrow exists and is in the correct state
+account_objects = client.request(AccountObjects(
+    account=wallet1.address,
+    type="escrow"
+)).result["account_objects"]
+
+escrow = next((obj for obj in account_objects 
+    if obj["Sequence"] == create_escrow_response.result["tx_json"]["Sequence"]), None)
+
+if not escrow:
+    raise ValueError("Escrow not found")
+
 finish_tx = EscrowFinish(
     account=wallet1.address,
     owner=wallet1.address,
     offer_sequence=create_escrow_response.result["tx_json"]["Sequence"],
     fulfillment=cryptoCondition["fulfillment"],
     condition=cryptoCondition["condition"],
 )

-submit_and_wait(finish_tx, client, wallet1)
+try:
+    finish_response = submit_and_wait(finish_tx, client, wallet1)
+    print(f"Escrow finished successfully: {finish_response.result['hash']}")
+except Exception as e:
+    print(f"Failed to finish escrow: {e}")
+    raise

71-107: Consider enhancing the educational value of this example.

While this example effectively demonstrates the basic usage of conditional escrows, consider these architectural improvements:

  1. Add examples of different condition types (e.g., time-based, hash-locked)
  2. Demonstrate the escrow cancellation flow
  3. Show how to handle common error cases
  4. Include examples of checking escrow status
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93fc134 and 1255d5d.

⛔ Files ignored due to path filters (1)
  • poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • snippets/send_escrow.py (2 hunks)
🔇 Additional comments (2)
snippets/send_escrow.py (2)

4-4: Verify the import accessibility of generate_escrow_cryptoconditions.

The function needs to be properly exposed in the package's public API through xrpl/models/transactions/__init__.py for external usage.

Also applies to: 10-10


72-72: Verify the randomness of generated crypto conditions.

The use of urandom(32) for generating crypto conditions has been observed to produce predictable patterns (always starting with A0258020 and ending with 810120).

#!/bin/bash
# Check if there are any tests validating the randomness of crypto conditions
rg -l "test.*crypto.*condition" | xargs rg "assert|expect.*A0258020|810120"

snippets/send_escrow.py Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
xrpl/models/transactions/escrow_create.py (1)

100-110: Enhance security documentation and input validation

While the function is well-documented functionally, it would benefit from:

  1. Security considerations in the docstring (e.g., requirements for secret entropy)
  2. Input validation for the secret parameter

Consider adding:

 def generate_escrow_cryptoconditions(secret: bytes) -> CryptoConditions:
     """Generate a condition and fulfillment for escrows
 
     Args:
         secret: Cryptographic source of randomness used to generate the condition and
             fulfillment
 
     Returns:
         A pair of condition and fulfillment is returned
 
+    Security:
+        - The secret must have sufficient entropy (recommended: at least 32 bytes of randomness)
+        - The secret should be securely generated and stored
+        - Never reuse the same secret for different escrows
     """
+    if len(secret) < 32:
+        raise ValueError("Secret must be at least 32 bytes for sufficient security")
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1255d5d and 0c4115e.

📒 Files selected for processing (1)
  • xrpl/models/transactions/escrow_create.py (2 hunks)
🔇 Additional comments (3)
xrpl/models/transactions/escrow_create.py (3)

8-8: Document the type_ignore rationale

The # type: ignore comment is necessary as the cryptoconditions library lacks type stubs. Consider adding a more descriptive comment explaining why the type checking is disabled.

-from cryptoconditions import PreimageSha256  # type: ignore
+from cryptoconditions import PreimageSha256  # type: ignore  # cryptoconditions lacks type stubs - see https://github.com/Legrandin/pycryptodome/issues/209

90-98: Well-structured type definition!

The CryptoConditions TypedDict is well-documented and properly typed, providing clear structure for the escrow condition/fulfillment pairs.


111-116: Implementation looks good!

The cryptographic condition generation is correctly implemented:

  • Proper use of PreimageSha256
  • Correct hex encoding with uppercase for XRPL compatibility
  • Type-safe return value using CryptoConditions

Let's verify the usage of this function in the codebase:

✅ Verification successful

Cryptographic condition generation is correctly used across the codebase

The verification shows that generate_escrow_cryptoconditions is used appropriately:

  • In snippets/send_escrow.py, it's used with urandom(32) for secure random secret generation
  • In tests, it's also properly used with urandom(32) for test cases
  • The function is documented in the changelog, indicating it's a supported public API
  • No instances of insecure secret generation were found
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of generate_escrow_cryptoconditions to ensure proper secret generation
rg "generate_escrow_cryptoconditions" -A 5

Length of output: 2577

@ckeshava
Copy link
Collaborator Author

ckeshava commented Jan 2, 2025

@khancode Please review this PR at your convinience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant