fix: incorrect SField definitions for Lending Protocols and DynamicMPTs#918
Merged
Conversation
Contributor
WalkthroughUpdated a single binary codec definitions JSON: added MutableFlags and StartDate fields, renamed PreviousPaymentDate to PreviousPaymentDueDate, reorganized LoanScale entries across blocks, added terNO_DELEGATE_PERMISSION error, and extended/reordered transaction type mappings (new loan-related types). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xrpl/core/binarycodec/definitions/definitions.json (1)
695-710: Document generator provenance for thesedefinitions.jsonupdates.Please include which upstream source/generation step produced the Line 695–Line 710 changes so this generated artifact remains reproducible.
Based on learnings: The
definitions.jsonfile in XRPL is a generated file and should not be directly edited.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xrpl/core/binarycodec/definitions/definitions.json` around lines 695 - 710, The changes to definitions.json (entries around the "UInt32" nth:53 and "StartDate" nth:54) are generated and must record their provenance rather than being edited in-place; revert any manual edits to definitions.json and instead update the generation pipeline: document the upstream source/commit/tag and generation step that produced these entries (e.g., XRPL definitions repo/commit and the generator script), add a short provenance metadata block (source repo + commit SHA + generator command) to the build/generation artifacts or README used by the generator, and always regenerate definitions.json via the documented generator (referencing the "StartDate" and the adjacent "UInt32" field entries to validate the regenerated output).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@xrpl/core/binarycodec/definitions/definitions.json`:
- Around line 695-710: The changes to definitions.json (entries around the
"UInt32" nth:53 and "StartDate" nth:54) are generated and must record their
provenance rather than being edited in-place; revert any manual edits to
definitions.json and instead update the generation pipeline: document the
upstream source/commit/tag and generation step that produced these entries
(e.g., XRPL definitions repo/commit and the generator script), add a short
provenance metadata block (source repo + commit SHA + generator command) to the
build/generation artifacts or README used by the generator, and always
regenerate definitions.json via the documented generator (referencing the
"StartDate" and the adjacent "UInt32" field entries to validate the regenerated
output).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
14-17: Improve phrasing and consider expanding the changelog entry.Two observations:
The phrasing "Fixed correct mapping" is redundant. Consider: "Corrected mapping of..." or "Fixed incorrect mapping of..."
The AI summary indicates this PR includes additional changes beyond the three fields mentioned (e.g., LoanScale reorganization, terNO_DELEGATE_PERMISSION error code, transaction type mappings). Verify whether those changes should also be documented in the changelog for completeness.
📝 Suggested phrasing improvement
### Fixed -- Fixed correct mapping of `sfMutableFlags`, `sfStartDate`, and `sfPreviousPaymentDueDate` fields in the binary codec `definitions.json`. +- Corrected mapping of `sfMutableFlags`, `sfStartDate`, and `sfPreviousPaymentDueDate` fields in the binary codec `definitions.json`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 14 - 17, Reword the changelog entry to remove redundant phrasing and add any other notable changes from this PR: change "Fixed correct mapping of `sfMutableFlags`, `sfStartDate`, and `sfPreviousPaymentDueDate` fields in the binary codec `definitions.json`." to something like "Corrected mapping of `sfMutableFlags`, `sfStartDate`, and `sfPreviousPaymentDueDate` in `definitions.json`." and, if the PR also includes the LoanScale reorganization, the new terNO_DELEGATE_PERMISSION error code, or updates to transaction type mappings, append short bullet lines mentioning those (e.g., "Reorganized LoanScale", "Added terNO_DELEGATE_PERMISSION error code", "Updated transaction type mappings") so the changelog reflects all relevant changes from this change set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 14-17: Reword the changelog entry to remove redundant phrasing and
add any other notable changes from this PR: change "Fixed correct mapping of
`sfMutableFlags`, `sfStartDate`, and `sfPreviousPaymentDueDate` fields in the
binary codec `definitions.json`." to something like "Corrected mapping of
`sfMutableFlags`, `sfStartDate`, and `sfPreviousPaymentDueDate` in
`definitions.json`." and, if the PR also includes the LoanScale reorganization,
the new terNO_DELEGATE_PERMISSION error code, or updates to transaction type
mappings, append short bullet lines mentioning those (e.g., "Reorganized
LoanScale", "Added terNO_DELEGATE_PERMISSION error code", "Updated transaction
type mappings") so the changelog reflects all relevant changes from this change
set.
Patel-Raj11
approved these changes
Mar 2, 2026
ckeshava
approved these changes
Mar 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Fix incorrect entries to binary-codec
definitions.json:sfMutableFlags(DynamicMPT): Defined as nth=54 instead of the correct nth=53. This causes xrpl-py to serializeMutableFlagswith the field code that rippled assigns tosfStartDate, corrupting transactions on the write path.sfStartDate(Lending Protocol): Entirely missing from xrpl-py's definitions. UInt32 nth=54 is incorrectly occupied byMutableFlags. Any xrpl-py application building Lending transactions (LoanSet) cannot include thesfStartDatefield, and when decoding binary containingsfStartDate, the field is misidentified as "MutableFlags".sfPreviousPaymentDueDate(Lending Protocol): Misspelled asPreviousPaymentDate. Code using the authoritative rippled field name will fail with field-not-found errors.Type of Change
Did you update CHANGELOG.md?