Add missing field types to lending protocol related objects and support Int32 serialized type#3198
Add missing field types to lending protocol related objects and support Int32 serialized type#3198Patel-Raj11 merged 9 commits intomainfrom
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:
WalkthroughAdds lending-protocol fields and validations across ledger models and transactions, introduces Vault Scale and DestinationTag handling, registers LoanPay flags with runtime validation, adds Int/Int32 codec types, and expands related unit and integration tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/xrpl/src/models/transactions/vaultCreate.ts`:
- Around line 167-187: The Scale validation must enforce integer values for IOU
assets: in the VaultCreate validation block (referencing tx.Scale, isIOU, isXRP,
isMPT, MAX_SCALE, and ValidationError) remove the redundant typeof check and add
an integer check (e.g., Number.isInteger(tx.Scale)) when isIOU is true, keeping
the existing 0..MAX_SCALE bounds; ensure the thrown ValidationError message
reflects the integer requirement (e.g., "Scale must be an integer between 0 and
MAX_SCALE inclusive for IOU assets").
🧹 Nitpick comments (2)
packages/xrpl/src/models/transactions/vaultCreate.ts (1)
161-165: Asset type detection looks correct but is narrowly scoped.The
assettype detection (XRP / MPT / IOU) at lines 161–165 is only used inside theif (tx.Scale !== undefined)block. If future validation rules also need asset-type awareness, consider hoisting this detection above the Scale block. Not blocking for now since it only serves the Scale check.packages/xrpl/test/integration/transactions/lendingProtocol.test.ts (1)
217-222: Consider usingBigIntinstead ofparseIntfor comparing ledger amounts.
parseIntwould silently truncate values in scientific notation (e.g.,"5e6"→5). While current values are plain integer strings, usingBigInt(paidLoan.PrincipalOutstanding)would be more robust and consistent with BigInt usage elsewhere in this file (e.g., lines 529–531).Proposed fix
assert.isTrue( - parseInt(paidLoan.PrincipalOutstanding, 10) < - parseInt(loanObject.PrincipalOutstanding, 10), + BigInt(paidLoan.PrincipalOutstanding) < + BigInt(loanObject.PrincipalOutstanding), 'Principal should decrease after payment', )And similarly at line 232:
assert.isTrue( - parseInt(paidLoan.PrincipalOutstanding, 10) > 0, + BigInt(paidLoan.PrincipalOutstanding) > BigInt(0), 'Principal should not be zero', )
ckeshava
left a comment
ckeshava
left a comment
There was a problem hiding this comment.
Where can I find the binary codec for the Int32 type? There is a new field titled LoanScale introduced in the Lending Protocol.
Patel-Raj11
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/ripple-binary-codec/HISTORY.md`:
- Around line 5-6: The changelog entry "* Add `Int32` serialized type." is
incorrectly placed under the "### Fixed" section; move that bullet to the "###
Added" section (or create an "Added" heading if missing) so the new Int32
serialized type is recorded as a feature addition rather than a fix, keeping the
exact bullet text and formatting intact.
In `@packages/ripple-binary-codec/src/types/int-32.ts`:
- Around line 43-51: The string branch in Int32.from uses Number.parseInt which
truncates decimals (e.g., "1.5" -> 1); update the logic in the block handling
typeof val === 'string' to parse via Number(val) (or parseFloat) and then
validate with Number.isFinite(num) and Number.isInteger(num) before calling
Int32.checkIntRange and writeInt32BE; ensure you throw the same Error message
for non-numeric or non-integer strings and return new Int32(buf) on success
(references: Int32, Int32.checkIntRange, writeInt32BE).
In `@packages/ripple-binary-codec/src/types/int.ts`:
- Around line 59-69: The static method checkIntRange uses this.constructor.name
which resolves to "Function"; update the error message to use the class
constructor's name via this.name instead. Locate the static method checkIntRange
and change the error construction so it references this.name (e.g., `Invalid
${this.name}: ...`) so the thrown Error reports the correct class name like
Int32.
In `@packages/ripple-binary-codec/test/int.test.ts`:
- Around line 176-181: Remove the debug console.log call in the test "can encode
and decode Loan with negative LoanScale": delete the line console.log(decoded)
from the it block that encodes with encode(...) and decodes with decode(...),
leaving only the expect(decoded).toEqual(loanWithScale) assertion so the test no
longer emits noisy output; reference symbols: encode, decode, loanWithScale, and
the it block name.
I completely missed this, thanks for highlighting. Added |
3 participants
High Level Overview of Change
Add newly added fields to lending protocol related ledger objects and transactions:
ManagementFeeOutstandingandLoanScaletoLoanobject.ManagementFeeRateandDatatoLoanBrokerobject.ShareMPTIDandScaletoVaultobject.AssetsMaximum,DataandScaletovault_inforesponse.DestinationTagtoLoanBrokerCoverWithdrawandVaultWithdrawtransactions.LoanPayFlags.ScaletoVaultCreatetransaction.Int32serialized type inripple-binary-codec. Needed forLoanScalefield ofLoanobject.Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan
Added unit and updated integration tests.