-
Notifications
You must be signed in to change notification settings - Fork 94
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
base: main
Are you sure you want to change the base?
Conversation
… into conditionalEscrow
WalkthroughThis pull request introduces functionality for creating conditional escrows in the XRPL Python library. The changes include adding a new utility function Changes
Possibly related PRs
Suggested reviewers
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this 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 errorxrpl/models/transactions/escrow_create.py (2)
8-9
: cryptoconditions import is correct but note type ignore usage.
If you wantmypy
to work withcryptoconditions
, 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
⛔ 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 usingcancel_after
but not usingfinish_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 usedcreate_escrow_response.result["tx_json"]["Sequence"]
foroffer_sequence
. Here, you usecreate_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 bothEscrowFinish
andgenerate_escrow_cryptoconditions
broadens the test coverage.xrpl/models/transactions/escrow_create.py (1)
90-98
: Well-structured TypedDict.
TheCryptoConditions
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 |
There was a problem hiding this comment.
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 inxrpl/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:
- Imported in
xrpl/models/transactions/__init__.py
- 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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this 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:
- Add examples of different condition types (e.g., time-based, hash-locked)
- Demonstrate the escrow cancellation flow
- Show how to handle common error cases
- Include examples of checking escrow status
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ 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 ofgenerate_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 withA0258020
and ending with810120
).#!/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"
There was a problem hiding this 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 validationWhile the function is well-documented functionally, it would benefit from:
- Security considerations in the docstring (e.g., requirements for secret entropy)
- 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
📒 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 rationaleThe
# type: ignore
comment is necessary as thecryptoconditions
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 withurandom(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 5Length of output: 2577
@khancode Please review this PR at your convinience |
High Level Overview of Change
Provide utility functions to generate cryptographic condition and fulfillment for conditional escrows.
Type of Change
Did you update CHANGELOG.md?
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.