feat: add support for the simulate RPC (XLS-69d)
#793
Conversation
WalkthroughThe pull request introduces support for the Changes
Sequence DiagramsequenceDiagram
participant Client
participant TransactionModule
participant XRPLNetwork
Client->>TransactionModule: simulate(transaction)
TransactionModule->>TransactionModule: Autofill network ID
TransactionModule->>TransactionModule: Encode transaction
TransactionModule->>XRPLNetwork: Send Simulate request
XRPLNetwork-->>TransactionModule: Return simulation response
TransactionModule-->>Client: Return response
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
xrpl/models/requests/sign_and_submit.py (1)
91-98: Consider adding error handling for Transaction.from_xrpl.While the implementation is correct, consider adding error handling for the
Transaction.from_xrplcall to gracefully handle malformed transaction data.if "tx_json" in value: + try: fixed_value = { **value, "transaction": Transaction.from_xrpl(value["tx_json"]), } del fixed_value["tx_json"] + except Exception as e: + raise ValueError(f"Invalid transaction data: {str(e)}") else: fixed_value = value return super().from_dict(fixed_value)CHANGELOG.md (2)
10-12: Enhance the changelog entry with more details about the simulate RPC.The changelog entry should provide more context about this significant feature. Consider expanding it to:
### Added -Support for the `simulate` RPC +Support for the `simulate` RPC (XLS-69d): +- New method to simulate transactions without submitting them to the network +- Available in `xrpl/asyncio/transaction/main.py` via the `simulate` method +- Uses the new `Simulate` request model from `xrpl/models/requests/simulate.py`
13-15: Clarify the WebSocket client compatibility fix.The entry should explain what specifically was fixed. Consider expanding it to:
### Fixed -`Sign` `SignFor`, and `SignAndSubmit` now work with Websocket clients +`Sign`, `SignFor`, and `SignAndSubmit` methods now properly handle transactions with WebSocket clients: +- The `transaction` field is now populated as a `Transaction` object instead of a raw JSON object +- Affects implementations in `xrpl/models/requests/sign.py`, `xrpl/models/requests/sign_for.py`, and `xrpl/models/requests/sign_and_submit.py`
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(1 hunks)tests/integration/reqs/test_simulate.py(1 hunks)tests/unit/models/test_base_model.py(2 hunks)xrpl/asyncio/transaction/main.py(3 hunks)xrpl/models/nested_model.py(1 hunks)xrpl/models/requests/__init__.py(2 hunks)xrpl/models/requests/request.py(1 hunks)xrpl/models/requests/sign.py(1 hunks)xrpl/models/requests/sign_and_submit.py(1 hunks)xrpl/models/requests/sign_for.py(1 hunks)xrpl/models/requests/simulate.py(1 hunks)xrpl/models/requests/submit_multisigned.py(1 hunks)xrpl/models/transactions/transaction.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- xrpl/models/transactions/transaction.py
🧰 Additional context used
🪛 GitHub Actions: Integration test
tests/integration/reqs/test_simulate.py
[error] 15-15: KeyError: 'meta' - Attempting to access non-existent 'meta' key in response.result dictionary in test_basic_functionality
🔇 Additional comments (15)
xrpl/models/requests/simulate.py (4)
1-2: Great use of a clear module docstring.This docstring quickly explains the intention behind the
Simulaterequest, which helps maintainers and end users get started with the feature easily.
31-37: Validation logic is solid.Enforcing exactly one of
tx_blobortransactionensures that the simulation has consistent inputs and prevents ambiguous or invalid states. This careful approach helps maintain robust request models.
39-59: Good design for thefrom_dictconstructor.Converting
tx_jsoninto aTransactionobject by callingTransaction.from_xrplcentralizes transaction parsing logic. This makes the API cleaner and avoids scattered self-managed conversions.
60-71: Clean serialization into_dict.Removing the
transactionfield in favor oftx_jsonis an elegant way to keep JSON output consistent with XRPL’s expected format. This approach simplifies the round-trip process for transaction objects.xrpl/models/requests/submit_multisigned.py (1)
61-61: Modernized super() usage.Using
super().from_dict(...)is more maintainable than referencing the class name directly, especially in complex inheritance scenarios—no issues identified here.xrpl/models/requests/sign_for.py (1)
73-80: LGTM! Improved type safety in transaction handling.The conversion of
tx_jsonto aTransactionobject ensures type safety and consistent transaction handling across request models.xrpl/models/requests/__init__.py (2)
46-46: LGTM! Added import for new Simulate request model.The import statement is correctly placed alphabetically in the imports section.
103-103: LGTM! Added Simulate to public API.The
Simulateclass is properly exposed in the__all__list, maintaining alphabetical order.xrpl/models/requests/sign.py (1)
82-88: LGTM! Consistent transaction handling implementation.The conversion of
tx_jsonto aTransactionobject aligns with the pattern used in other request classes.xrpl/models/requests/request.py (1)
41-41: LGTM!The addition of the
SIMULATEvalue to theRequestMethodenum is well-placed and follows the established naming convention.xrpl/asyncio/transaction/main.py (3)
16-24: LGTM!The imports are well-organized and the addition of the
Simulatemodel is properly placed within the grouped imports.
160-162: LGTM!The docstring case changes improve consistency with the rest of the codebase.
182-219: LGTM! Well-structured implementation of the simulate method.The implementation:
- Properly handles network ID autofill
- Includes appropriate error handling
- Returns consistent Response type
- Provides flexibility with the binary parameter
However, consider adding unit tests to verify the behavior of this new method.
Run the following script to check for test coverage:
tests/unit/models/test_base_model.py (1)
127-127: LGTM! Appropriate update to useto_xrpl().The change to use
to_xrpl()instead ofto_dict()is more appropriate for transaction serialization and maintains consistency with the codebase's conventions.Also applies to: 143-143
xrpl/models/nested_model.py (1)
62-63: LGTM! Simplified super() call.The change improves code readability while maintaining the same functionality.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| network. | ||
| """ | ||
|
|
||
| tx_blob: Optional[str] = None |
There was a problem hiding this comment.
Could you add docstring descriptions for these variables?
|
Should we check if the transaction is unsigned either in the |
Good idea - fixed |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/models/requests/test_simulate.py (1)
58-73: Add test coverage for the binary flag.While the success cases are well tested, consider adding test cases to validate the behavior when the
binaryflag is set toTrue.def test_simulate_with_binary_flag(self): simulate = Simulate(transaction=_TRANSACTION, binary=True) self.assertTrue(simulate.binary) self.assertEqual(simulate.method, "simulate") self.assertTrue(simulate.is_valid())xrpl/models/requests/simulate.py (1)
67-99: Add error handling for tx_json conversion.Consider adding error handling for the tx_json conversion to handle malformed input gracefully.
@classmethod def from_dict(cls: Type[Self], value: Dict[str, Any]) -> Self: if "tx_json" in value: + try: fixed_value = { **value, "transaction": Transaction.from_xrpl(value["tx_json"]), } del fixed_value["tx_json"] + except XRPLModelException as e: + raise XRPLModelException(f"Invalid tx_json format: {str(e)}") else: fixed_value = value return super().from_dict(fixed_value)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.vscode/settings.json(1 hunks)tests/unit/models/requests/test_simulate.py(1 hunks)xrpl/models/requests/simulate.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (5)
tests/unit/models/requests/test_simulate.py (2)
1-10: LGTM! Well-structured test setup.The test setup follows best practices with appropriate imports and test data initialization.
12-56: LGTM! Comprehensive error case coverage.The test suite thoroughly validates error scenarios, including:
- Input validation for tx_blob and transaction fields
- Handling of signed transactions
- Invalid transaction blob validation
This aligns well with the PR discussion about handling unsigned transactions.
.vscode/settings.json (1)
30-35: LGTM! IDE settings aligned with test framework.The changes correctly configure VSCode to use unittest and maintain consistent code formatting.
xrpl/models/requests/simulate.py (2)
1-44: LGTM! Well-documented class with proper design.The class is well-structured with:
- Comprehensive docstrings for class and fields
- Proper use of dataclass decorators
- Immutability through frozen=True
47-66: LGTM! Robust error handling for transaction validation.The error handling effectively:
- Validates input field requirements
- Checks for signed transactions
- Handles invalid transaction blobs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
xrpl/models/requests/simulate.py (3)
16-28: Class & Attribute DocumentationThe class and its attribute docstrings are descriptive and provide useful guidance. However, having two consecutive triple-quoted blocks (one for the class overview and another for the
tx_blobfield) may be slightly unconventional. Consider consolidating or clearly marking attribute-level documentation if alignment with your project’s documentation style is desired.
54-57: Refactor Nested If Condition in_get_errorsThe nested
ifon lines 54–57 can be combined into a single condition to improve readability. For example, consider refactoring as follows:- if self.transaction is not None: - if self.transaction.is_signed(): - errors["transaction"] = "Cannot simulate a signed transaction." + if self.transaction is not None and self.transaction.is_signed(): + errors["transaction"] = "Cannot simulate a signed transaction."This change simplifies the control flow without affecting functionality.
🧰 Tools
🪛 Ruff (0.8.2)
54-55: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
75-83: Enhance Exception Chaining infrom_dictIn the
from_dictmethod, when handling the exception during the conversion oftx_jsonto aTransaction, it is recommended to chain exceptions to preserve the original traceback. Update the exception block as follows:- except XRPLModelException as e: - raise XRPLModelException(f"Invalid tx_json format: {str(e)}") + except XRPLModelException as e: + raise XRPLModelException(f"Invalid tx_json format: {str(e)}") from eThis aligns with best practices for exception handling.
🧰 Tools
🪛 Ruff (0.8.2)
82-82: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/models/requests/test_simulate.py(1 hunks)xrpl/models/requests/simulate.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/requests/test_simulate.py
🧰 Additional context used
🪛 Ruff (0.8.2)
xrpl/models/requests/simulate.py
54-55: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (4)
xrpl/models/requests/simulate.py (4)
1-2: File Docstring ClarityThe module-level docstring clearly states the purpose of this request.
29-43: Field DeclarationsThe optional fields (
tx_blob,transaction, andbinary) are well declared with appropriate types and defaults. The inline documentation is clear.
46-59: Error Validation in_get_errorsThe validation logic correctly ensures that exactly one of
tx_blobandtransactionis provided and that a signed transaction is not simulated. The use of the expression(self.tx_blob is None) == (self.transaction is None)is clever but may be non-intuitive for new contributors. A brief inline comment explaining this condition could improve clarity.🧰 Tools
🪛 Ruff (0.8.2)
54-55: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
88-99: Dictionary Serialization into_dictThe
to_dictimplementation correctly handles the conversion of the internaltransactionfield into the expectedtx_jsonformat while removing the originaltransactionkey. The method is straightforward and appropriately documented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
xrpl/transaction/__init__.py (1)
Line range hint
1-1: Consider enhancing the module docstring.Given the addition of the
simulateRPC support, consider updating the module docstring to mention this key functionality. This would help users understand the full capabilities of the module.Apply this diff to enhance the documentation:
-"""Methods for working with transactions on the XRP Ledger.""" +"""Methods for working with transactions on the XRP Ledger. + +This module provides functionality for creating, signing, and submitting transactions +to the XRP Ledger. It also includes support for simulating transactions without +submitting them to the network, which is useful for testing and validation purposes. +"""tests/integration/sugar/test_transaction.py (1)
273-306: Consider improving the fee verification test.The test relies on parsing a config file which could be fragile. Consider:
- Using a mock or fixture for the expected fee
- Adding error handling for file operations
- Making the config file path configurable
- expected_fee = "" - with open(".ci-config/rippled.cfg", "r", encoding="utf-8") as file: - lines = file.readlines() # Read all lines into a list - - for value in lines: - kv_pairs = value.split() - # This step assumes that no non-`voting` section in the config file - # uses the reference_fee key-value pair. - if "reference_fee" in kv_pairs: - expected_fee = kv_pairs[2] - break + CONFIG_PATH = os.getenv("RIPPLED_CONFIG_PATH", ".ci-config/rippled.cfg") + try: + with open(CONFIG_PATH, "r", encoding="utf-8") as file: + for line in file: + if "reference_fee" in line: + expected_fee = line.split()[2] + break + else: + self.fail("reference_fee not found in config") + except FileNotFoundError: + self.fail(f"Config file not found at {CONFIG_PATH}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)tests/integration/it_utils.py(3 hunks)tests/integration/sugar/test_transaction.py(2 hunks)xrpl/asyncio/transaction/__init__.py(2 hunks)xrpl/asyncio/transaction/main.py(3 hunks)xrpl/models/requests/simulate.py(1 hunks)xrpl/transaction/__init__.py(2 hunks)xrpl/transaction/main.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- xrpl/models/requests/simulate.py
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Integration test (3.13)
- GitHub Check: Snippet test (3.13)
- GitHub Check: Snippet test (3.12)
- GitHub Check: Integration test (3.12)
- GitHub Check: Snippet test (3.11)
- GitHub Check: Integration test (3.11)
- GitHub Check: Snippet test (3.10)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Snippet test (3.9)
- GitHub Check: Snippet test (3.8)
- GitHub Check: Integration test (3.8)
🔇 Additional comments (9)
xrpl/transaction/__init__.py (2)
12-12: LGTM!The import of the
simulatefunction is correctly placed and aligns with the PR objectives of adding support for thesimulateRPC.
21-21: LGTM!The changes to
__all__correctly reflect the module's public API:
- Re-adding
multisignmaintains backward compatibility- Adding
simulatemakes the new RPC functionality publicly accessibleAlso applies to: 24-24
xrpl/asyncio/transaction/__init__.py (1)
9-9: LGTM!The
simulatefunction is correctly imported and exported, following the module's established pattern.Also applies to: 23-23
tests/integration/it_utils.py (1)
Line range hint
221-250: LGTM!The
async_onlyparameter is well implemented:
- Maintains backward compatibility with default value
False- Correctly controls the execution of sync tests
- Properly handles module imports based on the parameter value
Also applies to: 297-306
tests/integration/sugar/test_transaction.py (2)
256-272: LGTM!The test case for
sign_and_submitis well structured:
- Tests a basic payment transaction
- Verifies successful submission
308-328: LGTM!The test case for
simulateis comprehensive:
- Verifies successful response
- Checks response type and structure
- Validates engine result and applied status
xrpl/transaction/main.py (1)
136-162: LGTM!The
simulatefunction is well implemented:
- Clear documentation with proper parameter descriptions
- Correct error handling with appropriate exception
- Proper handling of optional binary parameter
xrpl/asyncio/transaction/main.py (2)
16-24: New Import for Simulate RPC Support
The updated import block now includes the newSimulatemodel along with other transaction-related models. This grouping is clear and consistent with the rest of the file, and it correctly supports the new simulation functionality.
182-217: Review of the NewsimulateMethod
The newly introduced asynchronoussimulatemethod correctly follows the patterns established by other transaction methods in the file. Key points include:
- Network ID Autofill: Lines 204–207 extract the transaction data and conditionally add the network ID using
_tx_needs_networkID(client), which is a consistent strategy compared to other autofill functions.- Request Execution and Error Handling: Lines 211–215 send the
Simulaterequest viaclient._request_impland correctly check if the response is successful; otherwise, anXRPLRequestFailureExceptionis raised.- Docstring Consistency: The docstring accurately reflects the method’s parameters and behavior.
One suggestion would be to consider adding an inline comment (or even logging, if appropriate for your codebase) in the error-handling block to facilitate debugging in case of simulation failures. Additionally, please verify that the simulation endpoint accepts unsigned transactions—as discussed in the PR comments—and, if needed, add a clarification either in the code or its documentation.
High Level Overview of Change
This PR adds support to xrpl-py for the new
simulateRPC.Context of Change
Spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0069d-simulate
rippled PR: XRPLF/rippled#5069
Type of Change
Did you update CHANGELOG.md?
Test Plan
TODO