Rename ticket properties in ledgerEntry model#3237
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe ChangesLedgerEntry request type and test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xrpl/src/models/methods/ledgerEntry.ts (1)
184-196:⚠️ Potential issue | 🟡 MinorJSDoc comment is inconsistent with the new field names.
The documentation on lines 186-187 still references the old field names
ownerandticket_sequence, but the actual fields are nowaccountandticket_seq. Update the JSDoc to match.📝 Proposed fix for documentation
/** * The Ticket object to retrieve. If a string, must be the object ID of the - * Ticket, as hexadecimal. If an object, the `owner` and `ticket_sequence` + * Ticket, as hexadecimal. If an object, the `account` and `ticket_seq` * sub-fields are required to uniquely specify the Ticket entry. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/methods/ledgerEntry.ts` around lines 184 - 196, Update the JSDoc for the ticket field to reflect the current property names: replace references to `owner` and `ticket_sequence` with `account` and `ticket_seq` so the comment matches the union type definition for ticket; ensure the descriptive sentence(s) around the `ticket` property and its sub-fields (`account`, `ticket_seq`) correctly describe them in the block above the ticket?: { account: string; ticket_seq: number } | string declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/models/methods/ledgerEntry.ts`:
- Around line 192-194: Update documentation and release notes to clearly
describe the breaking rename of the ledger entry lookup fields: the request
fields in packages/xrpl/src/models/methods/ledgerEntry.ts have changed from
owner -> account and ticket_sequence -> ticket_seq; add a migration section
showing example payloads before and after, and mention versions where the change
takes effect. Also add a short compatibility shim in the SDK surface (e.g.,
input parsing for the ledgerEntry request handler) that accepts the old fields
('owner' and 'ticket_sequence') and maps them to the new properties ('account'
and 'ticket_seq') with a deprecation warning to guide users toward updating
their code. Ensure the changelog entry references the exact field names and the
file/class (ledgerEntry) so consumers can find and update usages.
---
Outside diff comments:
In `@packages/xrpl/src/models/methods/ledgerEntry.ts`:
- Around line 184-196: Update the JSDoc for the ticket field to reflect the
current property names: replace references to `owner` and `ticket_sequence` with
`account` and `ticket_seq` so the comment matches the union type definition for
ticket; ensure the descriptive sentence(s) around the `ticket` property and its
sub-fields (`account`, `ticket_seq`) correctly describe them in the block above
the ticket?: { account: string; ticket_seq: number } | string declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3013724a-fe00-410a-86a0-55c7cd8c1229
📒 Files selected for processing (1)
packages/xrpl/src/models/methods/ledgerEntry.ts
| account: string | ||
| /** The Ticket Sequence number of the Ticket entry to retrieve. */ | ||
| ticket_sequence: number | ||
| ticket_seq: number |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
XRPL ledger_entry API ticket object field names account ticket_seq
💡 Result:
In the XRPL ledger_entry API method, to retrieve a Ticket object, specify the "ticket" field as an object with subfields "account" (string - the account address owning the ticket) and "ticket_seq" (number - the ticket sequence number). Alternatively, use "ticket" as a string for the hex ledger entry ID. The Ticket ledger entry object itself includes these fields (plus common ledger entry fields like index, LedgerEntryType, Flags): - Account: String - Address of the owning account - TicketSequence: Number - The reserved sequence number (note the camelCase in JSON) Example request: { "ticket": { "account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn", "ticket_seq": 389 }, "ledger_index": "validated" }
Citations:
- 1: https://xrpl.org/docs/references/http-websocket-apis/public-api-methods/ledger-methods/ledger_entry
- 2: https://xrpl.org/docs/references/protocol/ledger-data/ledger-entry-types/ticket
- 3: https://js.xrpl.org/interfaces/LedgerEntryRequest.html
These field names (account and ticket_seq) are correct and match the official XRPL ledger_entry API specification for ticket lookups.
This is a confirmed breaking change from the previous field names (owner and ticket_sequence). While the new names align with the official XRPL API, this will require migration guidance for existing consumers. Ensure release notes and documentation clearly communicate the rename with migration instructions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/models/methods/ledgerEntry.ts` around lines 192 - 194,
Update documentation and release notes to clearly describe the breaking rename
of the ledger entry lookup fields: the request fields in
packages/xrpl/src/models/methods/ledgerEntry.ts have changed from owner ->
account and ticket_sequence -> ticket_seq; add a migration section showing
example payloads before and after, and mention versions where the change takes
effect. Also add a short compatibility shim in the SDK surface (e.g., input
parsing for the ledgerEntry request handler) that accepts the old fields
('owner' and 'ticket_sequence') and maps them to the new properties ('account'
and 'ticket_seq') with a deprecation warning to guide users toward updating
their code. Ensure the changelog entry references the exact field names and the
file/class (ledgerEntry) so consumers can find and update usages.
| @@ -189,9 +189,9 @@ export interface LedgerEntryRequest extends BaseRequest, LookupByLedgerRequest { | |||
| ticket?: | |||
There was a problem hiding this comment.
This is a breaking change. Can you update
- HISTORY.md
- PR description where it currently claims that this isn't a breaking change that will impact library users
There was a problem hiding this comment.
@kuan121 Do we need to do a major-version upgrade owing to this change? Clearly other users have not used ledger_entry RPC to fetch Ticket objects, hence this bug is being discovered for the first time.
There was a problem hiding this comment.
Doesn't this seem more like a bug fix rather than a spec change? I don't think we need a major version update for this.
There was a problem hiding this comment.
I think you yourself should decide what to do next. I have done everything that depends on me.
ckeshava
left a comment
ckeshava
left a comment
There was a problem hiding this comment.
@Platonenkov Thank you for this work. Can you also include an integration test under the ledger_entry RPC tests that verifies that Ticket objects are fetched correctly? This will ensure that future code changes will not cause this regression.
packages/xrpl/test/integration/requests/ledgerEntry.test.ts would be an appropriate file this type of test
| const accountInfoResponse = await testContext.client.request({ | ||
| command: 'account_info', | ||
| account: testContext.wallet.classicAddress, | ||
| }) | ||
| const accSeq = accountInfoResponse.result.account_data.Sequence |
There was a problem hiding this comment.
FYI: For future work, you do not need to manually fill the account-sequence value, that can be populated by "autofill" methods in xrpl-py
There was a problem hiding this comment.
I understand this, but in this case it was necessary to calculate ticketSeq
|
@Platonenkov please resolve the merge conflict and rebase |
fix bug for tickets objects request
… XRPL API specification
done ✅ |
4 participants
fix bug for ticket objects request
High Level Overview of Change
This PR fixes the
ledger_entryrequest model forticketobjects inxrpl.js.Changes made:
ticket.ownertoticket.accountticket.ticket_sequencetoticket.ticket_seqThis aligns the request payload with the XRPL
ledger_entryAPI specification for Ticket objects, where the object form ofticketrequiresaccountandticket_seq.https://xrpl.org/docs/references/http-websocket-apis/public-api-methods/ledger-methods/ledger_entry#get-ticket-entry
Context of Change
The current model uses incorrect field names for ticket lookup requests:
ownerticket_sequenceAccording to the official XRPL documentation, the correct fields are:
accountticket_seqBecause of this mismatch, requests built using the current typed object shape do not match the API contract and fail when trying to retrieve Ticket ledger objects through
ledger_entry. The documented Ticket request example also usesaccountandticket_seq.This change updates the TypeScript model so the library matches the official API and users can construct valid Ticket object requests.
Type of Change
Did you update HISTORY.md?
Test Plan
Verified the change by reviewing the XRPL
ledger_entryTicket request specification and matching the request model to the documented parameter names:ticket.accountticket.ticket_seqTo reproduce:
ledger_entryrequest for a Ticket using object form.{ "command": "ledger_entry", "ticket": { "account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn", "ticket_seq": 389 }, "ledger_index": "validated" }