feat: add support for the simulate RPC (XLS-69d) #2867
Conversation
WalkthroughThis pull request updates the XRPL library by replacing the deprecated transaction flag function with a new utility ( Changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ 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: 0
🧹 Nitpick comments (8)
packages/xrpl/src/models/methods/simulate.ts (1)
67-88: Consider adding JSDoc comments for the generic type parameter.The
SimulateJsonResponseinterface looks good, but it would be helpful to document the generic type parameterT.Add JSDoc comments to explain the purpose of the generic type parameter:
+/** + * Response for a JSON simulate request. + * + * @template T - The type of transaction, defaults to BaseTransaction. + */ export interface SimulateJsonResponse<T extends BaseTransaction = Transaction> extends BaseResponse { // ... }packages/xrpl/test/integration/requests/simulate.test.ts (4)
12-14: Consider using a constant for the timeout value.Extract the timeout value into a named constant at the module level for better maintainability.
+const SIMULATE_TEST_TIMEOUT = 20000 -const TIMEOUT = 20000
23-43: Add test cases for error scenarios.The JSON request test looks good but only covers the success case. Consider adding test cases for error scenarios such as:
- Invalid transaction format
- Missing required fields
- Network errors
45-65: Add test cases for binary response validation.The binary request test looks good but could be enhanced by validating the binary response format more thoroughly:
- Verify the binary encoding format
- Check for specific binary fields
67-84: Add test cases for the sugar method with different transaction types.The sugar method test looks good but only tests
AccountSet. Consider adding test cases for other transaction types to ensure the sugar method works correctly with all transaction types.packages/xrpl/src/client/index.ts (1)
772-805: Consider adding error handling and validation.The
simulatemethod implementation looks good but could be enhanced with:
- Input validation for the transaction parameter
- Error handling for network issues
- Validation of the binary flag value
Example implementation:
public async simulate<Binary extends boolean = false>( transaction: SubmittableTransaction | string, opts?: { binary?: Binary }, ): Promise< Binary extends true ? SimulateBinaryResponse : SimulateJsonResponse > { + if (!transaction) { + throw new ValidationError('Transaction is required') + } + + if (opts?.binary !== undefined && typeof opts.binary !== 'boolean') { + throw new ValidationError('Binary flag must be a boolean') + } + // send request const binary = opts?.binary ?? false const request: SimulateRequest = typeof transaction === 'string' ? { command: 'simulate', tx_blob: transaction, binary } : { command: 'simulate', tx_json: transaction, binary } - return this.request(request) + try { + return await this.request(request) + } catch (error) { + throw new RippledError(`Failed to simulate transaction: ${error.message}`) + } }packages/xrpl/HISTORY.md (2)
13-15: New Feature Addition: Support for thesimulateRPC
The release history now documents the support for the newsimulateRPC. For consistency with the other bullet points under "### Added", consider revising the bullet text from:* Support for the
simulateRPCto:
* Adds support for the
simulateRPCThis small change aligns the wording with the earlier bullet that uses “Adds utility function …”.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
13-13: Multiple headings with the same content
null(MD024, no-duplicate-heading)
13-13: Duplicate Heading Observation
Static analysis detected that the "### Added" heading appears more than once in this section. If the intent is to separately list different additions (e.g. one for the utility function and one for RPC support), this may be acceptable. Otherwise, consider consolidating the headings to improve clarity.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
13-13: Multiple headings with the same content
null(MD024, no-duplicate-heading)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/client/index.ts(2 hunks)packages/xrpl/src/models/methods/index.ts(5 hunks)packages/xrpl/src/models/methods/ledger.ts(1 hunks)packages/xrpl/src/models/methods/simulate.ts(1 hunks)packages/xrpl/src/sugar/submit.ts(2 hunks)packages/xrpl/test/integration/requests/simulate.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/xrpl/src/sugar/submit.ts
- packages/xrpl/src/models/methods/ledger.ts
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/xrpl/HISTORY.md
13-13: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
🔇 Additional comments (8)
packages/xrpl/src/models/methods/simulate.ts (3)
15-28: LGTM! The request type definition looks good.The
SimulateRequesttype correctly enforces mutual exclusivity betweentx_blobandtx_jsonusing a discriminated union type.
30-36: LGTM! The binary and JSON request types look good.The specialized request types correctly extend the base
SimulateRequesttype and handle thebinaryflag appropriately.
45-65: LGTM! The binary response type looks good.The
SimulateBinaryResponseinterface correctly defines the structure for binary responses, including all necessary fields.packages/xrpl/src/models/methods/index.ts (4)
151-158: LGTM! The imports look good.The simulate-related types are correctly imported.
214-214: LGTM! The Request union type is correctly updated.The
SimulateRequesttype is correctly added to the Request union.
273-274: LGTM! The Response union type is correctly updated.The
SimulateResponsetype is correctly added to the Response union.
411-417: LGTM! The RequestResponseMap is correctly updated.The simulate-related types are correctly mapped in the RequestResponseMap.
packages/xrpl/src/client/index.ts (1)
43-44: LGTM! The imports look good.The simulate-related types are correctly imported.
Also applies to: 47-49
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