Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
Adds Smart Escrows support to xrpl-py by introducing new fields in escrow transactions, updating serialization definitions, extending fee logic, and reorganizing tests.
- Introduces
finish_functionanddatainEscrowCreate, andcomputation_allowanceinEscrowFinish - Updates binary codec definitions for new fields and WASM-related error codes
- Extends async fee calculation to include gas pricing and finish function costs
- Reorganizes escrow integration tests into a consolidated file and updates wallet faucet logic
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| xrpl/models/transactions/escrow_create.py | Add finish_function & data fields and validation logic |
| xrpl/models/transactions/escrow_finish.py | Add computation_allowance field |
| xrpl/core/binarycodec/definitions/definitions.json | Register new serialization definitions & WASM errors |
| xrpl/asyncio/wallet/wallet_generation.py | Update faucet URLs, map new network, add request timeout |
| xrpl/asyncio/transaction/main.py | Incorporate gas price and finish_function into fee calc |
| tests/unit/models/transactions/test_escrow_create.py | Add unit tests for new finish_function logic |
| tests/unit/asyn/wallet/test_wallet.py | Update unit tests for new faucet mappings |
| tests/integration/transactions/test_escrow.py | Consolidate escrow integration tests |
| tests/integration/it_utils.py | Add wait parameter to ledger acceptance helpers |
| pyproject.toml | Bump version to beta |
Comments suppressed due to low confidence (2)
tests/unit/models/transactions/test_escrow_create.py:8
- Add unit tests for the new
datafield inEscrowCreate, covering both serialization and validation scenarios whendatais provided or omitted.
def test_all_fields_valid(self):
xrpl/asyncio/transaction/main.py:610
ServerStateis not imported in this module, causing aNameError. Addfrom xrpl.models.requests import ServerState(or the correct import) at the top.
server_state = await client._request_impl(ServerState())
|
|
||
| finish_function: Optional[str] = None | ||
|
|
||
| data: Optional[str] = None |
There was a problem hiding this comment.
The new data field lacks a corresponding entry in definitions.json, so it won't be serialized. Add a "Data" definition under the escrow fields section with the appropriate isSerialized, nth, and type settings.
| if usage_context is not None: | ||
| json_body["usageContext"] = usage_context | ||
| response = await http_client.post(url=url, json=json_body) | ||
| response = await http_client.post(url=url, json=json_body, timeout=10) |
There was a problem hiding this comment.
Hard-coding timeout=10 duplicates the existing _TIMEOUT_SECONDS constant. Use timeout=_TIMEOUT_SECONDS to keep timeout behavior consistent.
| response = await http_client.post(url=url, json=json_body, timeout=10) | |
| response = await http_client.post(url=url, json=json_body, timeout=_TIMEOUT_SECONDS) |
| if transaction.transaction_type == TransactionType.ESCROW_CREATE: | ||
| escrow_create = cast(EscrowCreate, transaction) | ||
| if escrow_create.finish_function is not None: | ||
| base_fee += 1000 |
There was a problem hiding this comment.
[nitpick] The 1000 finish-function surcharge is a magic number. Extract it into a named constant (e.g., FINISH_FUNCTION_FEE) and document its origin.
| base_fee += 1000 | |
| base_fee += FINISH_FUNCTION_FEE |
| """Credentials associated with sender of this transaction. The credentials included | ||
| must not be expired.""" | ||
|
|
||
| computation_allowance: Optional[int] = None |
There was a problem hiding this comment.
[nitpick] Add validation in _get_errors to ensure that computation_allowance is only set when a finish_function is provided, otherwise reject or warn.
High Level Overview of Change
Adds xrpl-py support for Smart Escrows.
Also reorganizes the Escrow integration tests into one file.
Context of Change
XRPLF/XRPL-Standards#270
Type of Change
Did you update CHANGELOG.md?
Test Plan
Added some basic tests
Integration tests expected to fail right now, since this is a new feature.