Conversation
* update definitions.json * fix contract datatype * use definitions.json names for datatype
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces comprehensive smart contract support to the XRPL JavaScript library, replacing Hook infrastructure with WASM-based contract execution. It adds new transaction types, ledger entry types, complex serialization types for data handling, and contract-related transaction models and validators. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds initial support for XRPL “Contract*” transactions and related ledger/method models, plus binary-codec updates to serialize/deserialize new Data/DataType/Json primitives and ContractCall fixtures.
Changes:
- Add new
Contract*transaction model types, exports, andvalidate()switch handling inxrpl. - Add
ComputationAllowanceautofill viasimulate(gas estimate) and extend transaction metadata withGasUsed. - Extend
ripple-binary-codecwith new serialized types (Data, DataType, Json), type-ID plumbing, and ContractCall binary fixtures/tests.
Reviewed changes
Copilot reviewed 57 out of 58 changed files in this pull request and generated 35 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/xrpl/tools/generateModels.js | Updates model generation parsing/mapping for new transaction/type definitions. |
| packages/xrpl/test/models/contractUserDelete.test.ts | Adds ContractUserDelete validation tests (currently placeholder). |
| packages/xrpl/test/models/contractModify.test.ts | Adds ContractModify validation tests (currently placeholder). |
| packages/xrpl/test/models/contractDelete.test.ts | Adds ContractDelete validation tests (currently placeholder). |
| packages/xrpl/test/models/contractCreate.test.ts | Adds ContractCreate validation tests (currently placeholder). |
| packages/xrpl/test/models/contractClawback.test.ts | Adds ContractClawback validation tests (currently placeholder). |
| packages/xrpl/test/models/contractCall.test.ts | Adds ContractCall validation tests (currently placeholder). |
| packages/xrpl/src/sugar/getFeeXrp.ts | Adds getGasEstimate() helper using simulate. |
| packages/xrpl/src/sugar/autofill.ts | Adds getComputationAllowance() autofill helper using binary encoding + gas estimate. |
| packages/xrpl/src/models/transactions/transaction.ts | Extends SubmittableTransaction and validate() switch for Contract* types. |
| packages/xrpl/src/models/transactions/metadata.ts | Extends transaction metadata with optional GasUsed. |
| packages/xrpl/src/models/transactions/index.ts | Exports new Contract* transaction model types. |
| packages/xrpl/src/models/transactions/contractUserDelete.ts | Introduces ContractUserDelete transaction model + validator stub. |
| packages/xrpl/src/models/transactions/contractModify.ts | Introduces ContractModify transaction model + validator stub. |
| packages/xrpl/src/models/transactions/contractDelete.ts | Introduces ContractDelete transaction model + validator stub. |
| packages/xrpl/src/models/transactions/contractCreate.ts | Introduces ContractCreate transaction model/flags + validator stub. |
| packages/xrpl/src/models/transactions/contractClawback.ts | Introduces ContractClawback transaction model + validator stub. |
| packages/xrpl/src/models/transactions/contractCall.ts | Introduces ContractCall transaction model + validator stub. |
| packages/xrpl/src/models/transactions/common.ts | Adds ContractParameter flag types. |
| packages/xrpl/src/models/methods/contractInfo.ts | Adds contract_info request/response model. |
| packages/xrpl/src/models/ledger/ContractSource.ts | Adds ContractSource ledger entry model. |
| packages/xrpl/src/models/ledger/ContractData.ts | Adds ContractData ledger entry model. |
| packages/xrpl/src/models/ledger/Contract.ts | Adds Contract ledger entry model. |
| packages/xrpl/src/models/ledger/AccountRoot.ts | Adds ContractID to AccountRoot ledger entry. |
| packages/xrpl/src/models/common/index.ts | Adds contract ABI-ish common interfaces (Function/Parameter/etc). |
| packages/xrpl/src/client/index.ts | Adds ContractCall ComputationAllowance autofill hook. |
| packages/xrpl/src/Wallet/utils.ts | Adds signing helper changes (currently duplicates existing function). |
| packages/ripple-binary-codec/tools/generateDefinitions.js | Adds renaming rule for DATATYPE → DataType. |
| packages/ripple-binary-codec/test/st-data.test.ts | Adds Data type encode/decode tests. |
| packages/ripple-binary-codec/test/quality.test.ts | Updates bytesToHex import (currently to a missing dependency). |
| packages/ripple-binary-codec/test/fixtures/contract-call-tx.json | Adds ContractCall JSON fixture for codec roundtrip. |
| packages/ripple-binary-codec/test/fixtures/contract-call-binary.json | Adds ContractCall binary fixture for codec roundtrip. |
| packages/ripple-binary-codec/test/binary-serializer.test.ts | Adds ContractCall encode/decode test using new fixtures. |
| packages/ripple-binary-codec/test/binary-parser.test.ts | Updates bytesToHex import (currently to a missing dependency). |
| packages/ripple-binary-codec/src/types/xchain-bridge.ts | Adds getSType() implementation for XChainBridge. |
| packages/ripple-binary-codec/src/types/vector-256.ts | Adds getSType() implementation for Vector256. |
| packages/ripple-binary-codec/src/types/uint-8.ts | Adds getSType() implementation for UInt8. |
| packages/ripple-binary-codec/src/types/uint-64.ts | Adds getSType() implementation for UInt64. |
| packages/ripple-binary-codec/src/types/uint-32.ts | Adds getSType() implementation for UInt32. |
| packages/ripple-binary-codec/src/types/uint-16.ts | Adds getSType() implementation for UInt16. |
| packages/ripple-binary-codec/src/types/st-object.ts | Hardens associatedType handling and adds getSType() for STObject. |
| packages/ripple-binary-codec/src/types/st-number.ts | Adds getSType() implementation for STNumber. |
| packages/ripple-binary-codec/src/types/st-array.ts | Adds getSType() implementation for STArray. |
| packages/ripple-binary-codec/src/types/serialized-type.ts | Introduces SerializedTypeID and type-ID maps (currently has recursive getSType()). |
| packages/ripple-binary-codec/src/types/path-set.ts | Adds getSType() implementation for PathSet. |
| packages/ripple-binary-codec/src/types/json.ts | Adds STJson serialized type implementation. |
| packages/ripple-binary-codec/src/types/issue.ts | Adds getSType() implementation for Issue. |
| packages/ripple-binary-codec/src/types/index.ts | Registers Data + DataType in coreTypes (Json not yet registered). |
| packages/ripple-binary-codec/src/types/hash-256.ts | Adds getSType() implementation for Hash256. |
| packages/ripple-binary-codec/src/types/hash-192.ts | Adds getSType() implementation for Hash192. |
| packages/ripple-binary-codec/src/types/hash-160.ts | Adds getSType() implementation for Hash160. |
| packages/ripple-binary-codec/src/types/hash-128.ts | Adds getSType() implementation for Hash128. |
| packages/ripple-binary-codec/src/types/dataType.ts | Adds DataType serialized type implementation. |
| packages/ripple-binary-codec/src/types/data.ts | Adds Data serialized type implementation. |
| packages/ripple-binary-codec/src/types/currency.ts | Adds getSType() implementation for Currency. |
| packages/ripple-binary-codec/src/types/blob.ts | Adds getSType() implementation for Blob. |
| packages/ripple-binary-codec/src/types/amount.ts | Adds getSType() implementation for Amount. |
| packages/ripple-binary-codec/src/types/account-id.ts | Adds getSType() implementation for AccountID. |
| packages/ripple-binary-codec/src/serdes/binary-serializer.ts | Makes encodeVariableLength static and updates call sites. |
| packages/ripple-binary-codec/src/enums/definitions.json | Updates definitions for contract-related fields/types/transaction types. |
Comments suppressed due to low confidence (1)
packages/ripple-binary-codec/src/types/index.ts:49
definitions.jsonnow defines aJsontype, andSerializedTypeIDincludesSTI_JSON, butcoreTypesdoes not register/export an implementation for it. Without addingSTJsontocoreTypes(e.g.,Json: STJson), encoding/decoding fields of typeJson(likeContractJson) will fail because the associated type can’t be resolved.
const coreTypes: Record<string, typeof SerializedType> = {
AccountID,
Amount,
Blob,
Currency,
Data,
DataType,
Hash128,
Hash160,
Hash192,
Hash256,
Int32,
Issue,
Number: STNumber,
PathSet,
STArray,
STObject,
UInt8,
UInt16,
UInt32,
UInt64,
Vector256,
XChainBridge,
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
+44
| export function validateContractCall(tx: Record<string, unknown>): void { | ||
| validateBaseTransaction(tx) | ||
|
|
||
| // validateRequiredField(tx, 'ComputationAllowance', isNumber) | ||
|
|
||
| // validateRequiredField(tx, 'ContractAccount', isString) | ||
|
|
||
| // validateRequiredField(tx, 'FunctionName', isString) | ||
|
|
||
| // validateOptionalField(tx, 'Parameters', isany[]) | ||
| } |
There was a problem hiding this comment.
validateContractCall currently only calls validateBaseTransaction; all ContractCall-specific required/optional field validations are commented out. This means malformed ContractCall transactions will pass validation (and tests expecting ContractCall: missing/invalid field ... will fail). Uncomment/implement the required field checks for ContractAccount, FunctionName, ComputationAllowance (if required), and validate Parameters shape.
Comment on lines
+48
to
+66
| /** | ||
| * Estimates the gas required for a transaction by simulating the provided tx blob. | ||
| * | ||
| * @param client - The Client used to connect to the ledger. | ||
| * @param txBlob - The transaction blob to simulate. | ||
| * @returns The estimated gas as a string. | ||
| */ | ||
| export async function getGasEstimate( | ||
| client: Client, | ||
| txBlob: string, | ||
| ): Promise<number> { | ||
| const response = await client.request({ | ||
| command: 'simulate', | ||
| tx_blob: txBlob, | ||
| }) | ||
|
|
||
| if (response.result.engine_result !== 'tesSUCCESS') { | ||
| throw new Error(response.result.engine_result_message) | ||
| } |
There was a problem hiding this comment.
getGasEstimate’s JSDoc says it returns a string, but the function returns a number (Promise<number>). Align the documentation and return type. Also consider throwing XrplError (as getFeeXrp does) instead of a generic Error for non-tesSUCCESS results to keep error handling consistent.
Comment on lines
+128
to
+143
| private static makeValueFromVLWithType(parser: BinaryParser): SerializedType { | ||
| if (parser.size() === 0) { | ||
| throw new Error('Empty data when parsing STJson value') | ||
| } | ||
|
|
||
| const typeId = parser.read(1)[0] | ||
|
|
||
| // Delegate to appropriate type's fromParser | ||
| // This is a placeholder - actual implementation would dispatch to concrete types | ||
| // For now, we create an STJson if type is Object or Array | ||
| if (typeId === STJson.JsonType.Object || typeId === STJson.JsonType.Array) { | ||
| return STJson.fromParser(parser) | ||
| } | ||
|
|
||
| throw new Error(`Unsupported type ID in STJson: ${typeId}`) | ||
| } |
There was a problem hiding this comment.
STJson.makeValueFromVLWithType reads the first byte as an SType marker (written by addVLValue via value.getSType()), but then compares it to STJson.JsonType.Object/Array (0x00/0x01). This will never match for normal STypes (including STJson itself, which would be SerializedTypeID.STI_JSON), so decoding will always throw Unsupported type ID. Dispatch should be based on SerializedTypeID (or TYPE_NUMBER_TO_ID) and construct the correct concrete type from the remaining bytes.
Comment on lines
+14
to
+21
| beforeEach(function () { | ||
| tx = { | ||
| /* TODO: add sample transaction */ | ||
| } as any | ||
| }) | ||
|
|
||
| it('verifies valid ContractCreate', function () { | ||
| assert.doesNotThrow(() => validateContractCreate(tx)) |
There was a problem hiding this comment.
tx is initialized as an empty object, but validateBaseTransaction requires at least TransactionType and Account. As written, the "verifies valid ContractCreate" test will throw. Initialize tx with a minimal valid ContractCreate transaction shape before asserting it is valid.
Comment on lines
+68
to
+71
| ) | ||
| }) | ||
|
|
||
| it('throws w/ invalid InstanceParameters', function () { |
There was a problem hiding this comment.
This assignment uses /*TODO*/ as the RHS (tx.InstanceParameters = /*TODO*/), which is not valid TypeScript and will not compile. Replace the placeholder with a concrete invalid value to test validation.
Comment on lines
+3
to
+5
| import { validate, ValidationError } from '../../src' | ||
| import { validateContractCreate } from '../../src/models/transactions/contractCreate' | ||
|
|
There was a problem hiding this comment.
The import path casing here does not match the actual transaction model filename (contractCreate.ts). On case-sensitive filesystems this will fail to resolve. Update the import to use the correct path/casing (e.g., .../transactions/contractCreate).
Comment on lines
+83
to
+86
| ) | ||
| }) | ||
|
|
||
| it('throws w/ invalid InstanceParameters', function () { |
There was a problem hiding this comment.
This assignment uses /*TODO*/ as the RHS (tx.InstanceParameters = /*TODO*/), which is not valid TypeScript and will not compile. Replace it with a concrete invalid value.
Comment on lines
+3
to
+5
| import { validate, ValidationError } from '../../src' | ||
| import { validateContractUserDelete } from '../../src/models/transactions/contractUserDelete' | ||
|
|
There was a problem hiding this comment.
The import path casing here does not match the actual transaction model filename (contractUserDelete.ts). On case-sensitive filesystems this will fail to resolve. Update the import to use the correct path/casing (e.g., .../transactions/contractUserDelete).
| private innerType: SerializedTypeID | ||
|
|
||
| /** | ||
| * Default bytes for DataType (STI_NOTPRESENT) |
There was a problem hiding this comment.
DataType.defaultBytes is documented as STI_NOTPRESENT but is initialized to [0x00, 0x01] (which corresponds to SerializedTypeID.STI_UINT16). Either change the default bytes to represent STI_NOTPRESENT (0x0000) or update the comment/behavior so the default is unambiguous.
Suggested change
| * Default bytes for DataType (STI_NOTPRESENT) | |
| * Default bytes for DataType (SerializedTypeID.STI_UINT16 / 0x0001) |
Comment on lines
+14
to
+21
| beforeEach(function () { | ||
| tx = { | ||
| /* TODO: add sample transaction */ | ||
| } as any | ||
| }) | ||
|
|
||
| it('verifies valid ContractUserDelete', function () { | ||
| assert.doesNotThrow(() => validateContractUserDelete(tx)) |
There was a problem hiding this comment.
tx is initialized as an empty object, but validateBaseTransaction requires at least TransactionType and Account. As written, the "verifies valid ContractUserDelete" test will throw. Initialize tx with a minimal valid ContractUserDelete transaction before asserting validity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (10)
packages/ripple-binary-codec/test/binary-parser.test.ts-11-11 (1)
11-11:⚠️ Potential issue | 🟠 MajorChange import back to
@xrplf/isomorphic/utils;@transia/isomorphicis not declared in the monorepo.The package
@transia/isomorphicis not installed or listed in anypackage.json. All other code in ripple-binary-codec imports from@xrplf/isomorphic/utils(the declared dependency). This import change will cause a module resolution failure at test runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-binary-codec/test/binary-parser.test.ts` at line 11, The test imports bytesToHex from the wrong package name; change the require call that currently imports bytesToHex from '@transia/isomorphic/utils' back to the declared monorepo package '@xrplf/isomorphic/utils' so module resolution succeeds (update the require for bytesToHex in the test file).packages/xrpl/src/client/index.ts-697-699 (1)
697-699:⚠️ Potential issue | 🟠 MajorUse a nullish check for
ComputationAllowanceto preserve explicit values.On line 697,
!tx.ComputationAllowancetreats0as "missing" and overwrites caller-provided input during autofill. This is inconsistent with the pattern used on line 694 (tx.DeliverMax != null). Usetx.ComputationAllowance == nullto autofill only when the field is absent.Suggested fix
- if (tx.TransactionType === 'ContractCall' && !tx.ComputationAllowance) { + if (tx.TransactionType === 'ContractCall' && tx.ComputationAllowance == null) { promises.push(getComputationAllowance(this, tx)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/client/index.ts` around lines 697 - 699, The condition that autofills ComputationAllowance treats 0 as missing; update the check in the block where tx.TransactionType === 'ContractCall' (the branch that pushes getComputationAllowance(this, tx)) to use a nullish check (tx.ComputationAllowance == null) instead of !tx.ComputationAllowance so explicit zero values provided by callers are preserved when autofilling.packages/xrpl/test/models/contractDelete.test.ts-14-21 (1)
14-21:⚠️ Potential issue | 🟠 MajorTests are incomplete and will fail.
The sample transaction is an empty object. The "valid ContractDelete" test at lines 18-21 will fail because:
validateBaseTransactionrequiresTransactionTypeandAccountvalidateContractDeletelikely requiresContractAccountWould you like me to help generate a valid sample
ContractDeletetransaction?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/models/contractDelete.test.ts` around lines 14 - 21, The test uses an empty tx so validateContractDelete/validate will fail; update the beforeEach to construct a minimal valid ContractDelete transaction object including required base fields TransactionType and Account (e.g., TransactionType: "ContractDelete", Account: "<valid address>") and the contract-specific field ContractAccount (set to a valid account/address), plus any other required base fields (Sequence/Fee/SigningPubKey or stubbed equivalents) so that validateBaseTransaction and validateContractDelete succeed when called in the 'verifies valid ContractDelete' test; ensure the object matches the expected shape used by validateContractDelete and cast to the same any/type as before.packages/xrpl/test/models/contractUserDelete.test.ts-14-21 (1)
14-21:⚠️ Potential issue | 🟠 MajorBuild these cases from a valid
ContractUserDeletefixture.Line 15 seeds
txwith{}, so the “valid” case on Lines 18-21 is a false positive, and the missing-field cases below depend on whichever base fieldvalidateBaseTransaction()rejects first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/models/contractUserDelete.test.ts` around lines 14 - 21, The test seeds tx with an empty object causing false positives; replace the placeholder in beforeEach so tx is cloned from a known-good ContractUserDelete fixture (use the project fixture helper or a JSON fixture for ContractUserDelete) and then use that valid tx in the “verifies valid ContractUserDelete” case to call validateContractUserDelete(tx) and validate(tx). For the missing-field tests, start from the same cloned valid fixture and remove the specific field under test (rather than starting from {}), so validateContractUserDelete/validateBaseTransaction reject the intended missing-field and not some other base-field first.packages/xrpl/test/models/contractCall.test.ts-14-23 (1)
14-23:⚠️ Potential issue | 🟠 MajorStart these assertions from a valid
ContractCallpayload.Line 15 leaves
txeffectively empty, so the “valid” case on Lines 20-23 only proves the validator is still a no-op, and the missing-field tests below are deleting fields that were never present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/models/contractCall.test.ts` around lines 14 - 23, Replace the placeholder tx in the beforeEach with a complete, valid ContractCall payload so the "valid" assertions exercise the validators (not a no-op); populate tx with the required fields that validateContractCall/validate expect (e.g., TransactionType: 'ContractCall', Account, Fee, Sequence, Contract, Method, Params) so subsequent tests that delete fields are removing real properties; update the beforeEach initialization of tx (used by validateContractCall and validate) to this realistic example.packages/xrpl/test/models/contractClawback.test.ts-14-21 (1)
14-21:⚠️ Potential issue | 🟠 MajorStart from a valid
ContractClawbackfixture.Line 15 initializes
txas{}, so deletingAmounton Line 24 is a no-op and the later exact-message assertions depend on base-transaction validation, not theContractClawbackrules this suite is supposed to cover.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/models/contractClawback.test.ts` around lines 14 - 21, The test uses an empty tx so ContractClawback-specific validations aren’t exercised; initialize tx in beforeEach with a known-valid ContractClawback fixture (or build a valid object) instead of {} so subsequent mutations (e.g., deleting Amount) actually test ContractClawback rules; update the beforeEach fixture setup referenced by tx and keep the asserts calling validateContractClawback(tx) and validate(tx) unchanged so the suite verifies ContractClawback-specific validation logic.packages/xrpl/test/models/contractCall.test.ts-115-128 (1)
115-128:⚠️ Potential issue | 🟠 MajorFix incomplete assignment syntax—test currently assigns function result instead of invalid value.
Line 116 ends with
=, leaving the assignment incomplete. The subsequentassert.throws()call becomes the assigned value fortx.Parameters, meaning the test never sets up an invalid payload. Assign a concrete invalid value to properly validate error handling.🔧 Proposed fix
- tx.Parameters = - /*TODO*/ + tx.Parameters = 123🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/models/contractCall.test.ts` around lines 115 - 128, The test leaves tx.Parameters assignment incomplete so tx.Parameters ends up being the result of assert.throws and the invalid payload is never set; set tx.Parameters to a concrete invalid value (for example a wrong type or malformed structure such as a plain number, string, or an object missing required fields) before calling validateContractCall and validate so both throw ValidationError; update the test to assign the invalid value to tx.Parameters (referencing tx.Parameters) and then call validateContractCall(tx) and validate(tx) expecting ValidationError.packages/xrpl/test/models/contractUserDelete.test.ts-113-125 (1)
113-125:⚠️ Potential issue | 🟠 MajorReplace the
/*TODO*/RHS with a real invalid value.The current code sets
tx.Parameterstoundefined(the comment consumes the assignment's right-hand side), which doesn't actually test validation of an invalidParametersvalue. The test should assign a concrete invalid value to properly verify the validation throws.🔧 Proposed fix
- tx.Parameters = /*TODO*/ + tx.Parameters = 123🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/models/contractUserDelete.test.ts` around lines 113 - 125, Replace the placeholder RHS for tx.Parameters with a concrete invalid value (e.g., a wrong type like 123 or an empty object) so the test actually exercises validation; set tx.Parameters = 123 (or {}) and keep the existing assertions that validateContractUserDelete(tx) and validate(tx) throw ValidationError to confirm the field is rejected. Ensure you update the assignment in the test where tx.Parameters is currently /*TODO*/ and leave the assert.throws lines unchanged.packages/ripple-binary-codec/src/types/data.ts-337-344 (1)
337-344: 🛠️ Refactor suggestion | 🟠 MajorWrap switch case declarations in a block scope.
The
const valueVLdeclaration inside the switch case can be erroneously accessed by other switch clauses. This is flagged by the static analysis tool.♻️ Proposed fix
case SerializedTypeID.STI_VL: - const valueVL = parser.readVariableLength() - dataValue = Blob.from(bytesToHex(valueVL)) - dataBytes = concat([ - BinarySerializer.encodeVariableLength(valueVL.length), - valueVL, - ]) - break + { + const valueVL = parser.readVariableLength() + dataValue = Blob.from(bytesToHex(valueVL)) + dataBytes = concat([ + BinarySerializer.encodeVariableLength(valueVL.length), + valueVL, + ]) + break + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-binary-codec/src/types/data.ts` around lines 337 - 344, The const declared in the SerializedTypeID.STI_VL switch branch (valueVL from parser.readVariableLength()) must be confined to its own block to avoid leaking into other cases; wrap the case body for SerializedTypeID.STI_VL in braces { ... } so valueVL, dataValue, and dataBytes are block-scoped, keep the existing assignments to dataValue (Blob.from(bytesToHex(valueVL))), dataBytes (concat([BinarySerializer.encodeVariableLength(valueVL.length), valueVL])) and the break inside the new block; this prevents cross-case access while preserving behavior of parser.readVariableLength(), BinarySerializer.encodeVariableLength, bytesToHex, Blob.from and concat.packages/ripple-binary-codec/src/types/json.ts-588-593 (1)
588-593:⚠️ Potential issue | 🟠 Major
isEquivalentcompares constructor bytes, not actual data.The method compares
this.byteswhich is set in the constructor, but instances created programmatically (e.g., viasetObjectField) pass emptyUint8Arrayto the constructor while storing actual data inthis.data. Two different STJson instances created this way would incorrectly compare as equivalent.🐛 Proposed fix to serialize before comparison
isEquivalent(t: SerializedType): boolean { if (!(t instanceof STJson)) { return false } - return bytesToHex(this.bytes) === bytesToHex(t.bytes) + // Serialize both to get current state, not constructor bytes + return bytesToHex(this.toBlob()) === bytesToHex(t.toBlob()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-binary-codec/src/types/json.ts` around lines 588 - 593, isEquivalent on STJson incorrectly compares constructor-populated this.bytes instead of the actual JSON payload, so instances created via setObjectField (which store data in this.data) can be falsely equal; update isEquivalent to compare the serialized form of both instances (use the class's serialize/toBytes/getBytes method or fall back to this.data when present) — e.g., obtain bytes for both this and t by calling the STJson serialization accessor and compare those byte arrays (referencing isEquivalent, STJson, this.bytes, this.data, and setObjectField to locate the logic).
🟡 Minor comments (5)
packages/xrpl/src/sugar/getFeeXrp.ts-53-53 (1)
53-53:⚠️ Potential issue | 🟡 MinorJSDoc return description is outdated.
Line 53 says “returns ... as a string”, but
getGasEstimatereturnsPromise<number>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/sugar/getFeeXrp.ts` at line 53, The JSDoc for getGasEstimate in getFeeXrp.ts is incorrect: it says the function "returns the estimated gas as a string" but getGasEstimate actually returns a Promise<number>; update the `@returns` description to accurately state it returns a Promise<number> (or "the estimated gas as a number" / "a Promise resolving to a number") so the documentation matches the function signature and behavior.packages/xrpl/src/models/transactions/contractCall.ts-18-18 (1)
18-18:⚠️ Potential issue | 🟡 MinorTypo in comment: "beacuse" → "because"
- // Optional beacuse autofill will add it if missing + // Optional because autofill will add it if missing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/contractCall.ts` at line 18, Fix the typo in the inline comment in packages/xrpl/src/models/transactions/contractCall.ts: change "beacuse" to "because" in the comment that reads "// Optional beacuse autofill will add it if missing" (update the comment text within the ContractCall/contract call-related code to the corrected spelling).packages/xrpl/src/models/ledger/ContractSource.ts-4-8 (1)
4-8:⚠️ Potential issue | 🟡 MinorEmpty JSDoc — add a description for the ledger entry.
The JSDoc block is missing a description of what
ContractSourcerepresents.📝 Suggested fix
/** - * - * + * Represents a contract source stored on the XRP Ledger. + * Contains the contract code, hash, functions, and configuration flags. * * `@category` Ledger Entries */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/ledger/ContractSource.ts` around lines 4 - 8, The JSDoc for the ledger entry type ContractSource is empty; add a concise description above the ContractSource declaration explaining what this ledger entry represents (e.g., stores on-ledger smart contract bytecode/source and metadata for XRPL contracts), include any relevant usage or important fields it models, and keep the `@category` Ledger Entries tag — update the JSDoc block immediately preceding the ContractSource type/class so readers and docs generators get a meaningful summary.packages/ripple-binary-codec/src/types/dataType.ts-147-150 (1)
147-150:⚠️ Potential issue | 🟡 Minor
setInnerType()should not mutate shared buffer state.The method mutates
this.bytesviawriteUInt16BE(), which is problematic when the buffer is the shared staticDataType.defaultBytes. Instances constructed without an explicit bytes parameter use this shared buffer, so callingsetInnerType()on any such instance would corrupt the state for all other instances relying ondefaultBytes. This creates a latent bug. Consider creating a defensive copy ofdefaultBytesin the constructor or validating thatsetInnerType()is only called on instances with independent buffers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-binary-codec/src/types/dataType.ts` around lines 147 - 150, setInnerType currently mutates this.bytes via writeUInt16BE which corrupts the shared DataType.defaultBytes for other instances; to fix, ensure instances do not share the static buffer by making a defensive copy of DataType.defaultBytes in the constructor when bytes is not provided (or when this.bytes === DataType.defaultBytes), or alternatively have setInnerType perform a copy-on-write (clone this.bytes into a new Buffer before calling writeUInt16BE) so that setInnerType always mutates an instance-local buffer rather than the shared defaultBytes.packages/ripple-binary-codec/src/types/data.ts-428-446 (1)
428-446:⚠️ Potential issue | 🟡 MinorFix switch declaration scoping and error message.
Two issues in this segment:
- Line 429: The
const vlLengthdeclaration should be wrapped in a block scope (same as line 338).- Line 444: The error message uses
typeof innerTypewhich would output "number" or "undefined", not the actual type ID value.🐛 Proposed fix
case SerializedTypeID.STI_VL: - const vlLength = parser.readVariableLengthLength() - return Blob.fromParser(parser, vlLength) + { + const vlLength = parser.readVariableLengthLength() + return Blob.fromParser(parser, vlLength) + } case SerializedTypeID.STI_ACCOUNT: parser.skip(1) return AccountID.fromParser(parser) case SerializedTypeID.STI_AMOUNT: return Amount.fromParser(parser) case SerializedTypeID.STI_ISSUE: return Issue.fromParser(parser) case SerializedTypeID.STI_CURRENCY: return Currency.fromParser(parser) case SerializedTypeID.STI_NUMBER: return STNumber.fromParser(parser) default: throw new Error( - `Data.getValue(): unsupported type ID: ${typeof innerType}`, + `Data.getValue(): unsupported type ID: ${innerType}`, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-binary-codec/src/types/data.ts` around lines 428 - 446, The switch branch for SerializedTypeID.STI_VL declares const vlLength at case scope which can leak—wrap that branch body in braces (e.g., case SerializedTypeID.STI_VL: { const vlLength = parser.readVariableLengthLength(); return Blob.fromParser(parser, vlLength); } ) and update the thrown error in Data.getValue() to include the actual innerType value instead of typeof innerType (e.g., interpolate innerType into the message) so the error reports the real type ID.
🧹 Nitpick comments (11)
packages/xrpl/src/models/common/index.ts (1)
330-335: Prefer a more specific public type name thanFunction.Using
Functionas an exported interface can be confused with the global JS/TSFunctiontype. Consider renaming toContractFunctionto improve clarity. The change would affect 3 files that currently import and use this type:contractCreate.ts,contractModify.ts, andContractSource.ts.Proposed refactor
-export interface Function { +export interface ContractFunction { Function: { FunctionName: string Parameters?: Parameter[] } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/common/index.ts` around lines 330 - 335, The exported interface named Function conflicts with the global JS/TS Function and should be renamed to a clearer public type (e.g., ContractFunction); update the exported declaration (change interface Function { Function: { ... } } to interface ContractFunction { Function: { FunctionName: string; Parameters?: Parameter[] } }) and then update all imports/usages of that type in the three consumers (contractCreate.ts, contractModify.ts, ContractSource.ts) to import and reference ContractFunction instead of Function; ensure any type annotations, export statements, and re-exports are adjusted accordingly so compilation and typings remain correct.packages/xrpl/src/models/ledger/ContractData.ts (1)
3-10: JSDoc comment is empty.The documentation comment block on lines 3-6 is a template placeholder without an actual description for the
ContractDataledger entry type.📝 Suggested improvement
/** - * - * + * The ContractData object type represents data stored by a smart contract + * on behalf of a user account. + * * `@category` Ledger Entries */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/ledger/ContractData.ts` around lines 3 - 10, The JSDoc for the ContractData ledger entry is empty; update the comment block above the ContractData interface to include a concise description of what a ContractData ledger entry represents (e.g., purpose, key fields, and relation to BaseLedgerEntry and HasPreviousTxnID), so consumers and generated docs can understand this type; edit the JSDoc immediately above the ContractData interface declaration to replace the placeholder with a meaningful summary and any relevant tags like `@category` Ledger Entries.packages/xrpl/src/sugar/autofill.ts (2)
426-426: Magic number should be documented or extracted as a constant.The value
100000000000is used as a placeholderComputationAllowancefor gas estimation. Consider extracting this to a named constant with documentation explaining its purpose.📝 Suggested improvement
+// Maximum computation allowance used for gas estimation simulation +const MAX_COMPUTATION_ALLOWANCE_FOR_ESTIMATION = 100000000000 + export async function getComputationAllowance( client: Client, tx: Transaction, ): Promise<void> { const copyTx = { ...tx } - copyTx.ComputationAllowance = 100000000000 + copyTx.ComputationAllowance = MAX_COMPUTATION_ALLOWANCE_FOR_ESTIMATION🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/sugar/autofill.ts` at line 426, Extract the magic number 100000000000 used to set copyTx.ComputationAllowance into a clearly named constant (e.g., DEFAULT_COMPUTATION_ALLOWANCE or PLACEHOLDER_COMPUTATION_ALLOWANCE) with a brief doc comment explaining it's a placeholder for gas estimation and why that specific value was chosen; replace the inline literal in the autofill logic (where copyTx.ComputationAllowance is assigned) with that constant and update any related tests or references to use the new constant name.
421-431: Shallow copy may cause unintended mutation of nested objects.The spread operator
{ ...tx }creates a shallow copy. Iftxcontains nested objects (e.g., arrays,Memos), modifications tocopyTxcould affect the originaltx. While the current code only modifies top-level properties, this could become a subtle bug if the logic expands.Additionally, the JSDoc mentions "zero Fee" but the code doesn't explicitly set
Feeon the copy.♻️ Consider a safer approach
export async function getComputationAllowance( client: Client, tx: Transaction, ): Promise<void> { - const copyTx = { ...tx } + // Deep clone to avoid mutation of nested objects + const copyTx = JSON.parse(JSON.stringify(tx)) as Transaction copyTx.ComputationAllowance = 100000000000 + copyTx.Fee = '0' delete copyTx.SigningPubKey const tx_blob = encode(copyTx)Alternatively, if deep cloning is too expensive, document that this function assumes
txhas no nested objects that would be affected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/sugar/autofill.ts` around lines 421 - 431, getComputationAllowance currently uses a shallow copy (copyTx = { ...tx }) which can mutate nested properties on tx and also doesn't explicitly set Fee to zero as the JSDoc implies; change the function to create a safe copy (either a deep clone of tx or a documented precondition) and explicitly set copyTx.Fee = "0" (or 0 depending on encoding expectations) before setting copyTx.ComputationAllowance and calling encode/getGasEstimate; update references in the function (copyTx, tx, ComputationAllowance, Fee, encode, getGasEstimate) accordingly so nested fields like Memos aren't accidentally modified or document that tx must not contain nested objects if you prefer not to deep clone.packages/xrpl/src/models/methods/contractInfo.ts (2)
24-29: Clean up development comments before merge.The
// IDEAblock (lines 26-29) appears to be a development note. Consider removing it or converting it to a formal TODO/FIXME if this work is planned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/methods/contractInfo.ts` around lines 24 - 29, Remove the informal development note inside the ContractInfoResponse interface: delete the "// IDEA" comment block (the commented potential fields for contract, contract_source, contract_account) or convert it to a proper TODO/FIXME comment if you intend to track it, so the ContractInfoResponse (which extends BaseResponse) no longer contains stray dev comments.
34-34: Consider stronger typing forfunctions.
functions: any[]loses type information. If theFunctiontype from../commonmatches the expected shape, consider usingFunction[]for consistency with other contract models.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/methods/contractInfo.ts` at line 34, The field functions is currently typed as any[] which discards type safety; change its type to the shared Function type from ../common (i.e., replace functions: any[] with functions: Function[] or the exact exported name used in ../common) in the contractInfo model so it matches other contract models and preserves shape information for the contract functions.packages/xrpl/src/models/transactions/contractCall.ts (1)
34-44: Runtime validation is incomplete — all field checks are commented out.The validator only calls
validateBaseTransactionbut does not validateContractAccount,FunctionName, orParameters. WhileComputationAllowanceis optional (autofilled), the required string fields should be validated at runtime to catch malformed transactions early.This pattern appears across all contract transaction validators in this PR. Consider uncommenting these validations or tracking this as a follow-up task.
Would you like me to open an issue to track enabling these validators?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/contractCall.ts` around lines 34 - 44, The validateContractCall function currently only invokes validateBaseTransaction and has all specific field validators commented out; restore the runtime checks by re-enabling the validators in validateContractCall: call validateRequiredField(tx, 'ContractAccount', isString) and validateRequiredField(tx, 'FunctionName', isString), keep ComputationAllowance optional (validateOptionalField(tx, 'ComputationAllowance', isNumber) or leave commented if intentionally autofilled), and validateOptionalField(tx, 'Parameters', isAnyArray) (or the existing isany[] helper) so ContractAccount and FunctionName are enforced as strings and Parameters is validated when present.packages/ripple-binary-codec/test/st-data.test.ts (1)
202-485: Large blocks of commented-out tests.Significant test coverage for AMOUNT, CURRENCY, ISSUE, error handling, equality checks, and
getValue()is commented out. If these features are not yet implemented, consider:
- Removing the commented code and tracking as a follow-up issue
- Using
it.skip()ordescribe.skip()to make pending tests visible in test outputThis helps maintain cleaner code and clearer test status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-binary-codec/test/st-data.test.ts` around lines 202 - 485, The test file contains large blocks of commented-out tests (e.g., describe('AMOUNT'), describe('CURRENCY'), describe('ISSUE'), error handling, Data equality, and Data.getValue checks that reference Data.from, BinaryParser, and getValue) which should be cleaned up: either remove these commented tests and open a follow-up issue to re-add them when features are implemented, or convert them to pending tests using it.skip()/describe.skip() so they remain visible in test output; update the test file to apply one consistent approach (remove or skip) for the blocks named AMOUNT, CURRENCY, ISSUE, Number/Complex roundtrip/Error handling/Data equality/Data getValue to keep the test suite tidy and actionable.packages/ripple-binary-codec/src/types/dataType.ts (1)
44-59: Double cast on line 57 may hide type mismatches.The
readUInt16BE(used, 0) as unknown as SerializedTypeIDdouble-cast bypasses TypeScript's type checking. Consider using a proper conversion or validation that the read value is a validSerializedTypeID.Safer alternative
- this.innerType = readUInt16BE(used, 0) as unknown as SerializedTypeID + const typeId = readUInt16BE(used, 0) + // TYPE_ID_TO_STRING lookup validates the type ID exists + if (TYPE_ID_TO_STRING[typeId] === undefined && typeId !== 0) { + // Allow unknown types but log or handle gracefully + } + this.innerType = typeId as SerializedTypeID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-binary-codec/src/types/dataType.ts` around lines 44 - 59, The constructor in DataType currently bypasses type safety by using readUInt16BE(used, 0) as unknown as SerializedTypeID; instead, read the 16-bit value with readUInt16BE(used, 0), validate that the numeric value is a known/allowed SerializedTypeID (e.g., check against the SerializedTypeID enum or a lookup set), and only then assign it to this.innerType with a single safe cast or direct assignment; if the value is not valid, throw a descriptive Error. Locate this logic in the DataType constructor (constructor, innerType field) and replace the double-cast with explicit validation and a clear error path.packages/ripple-binary-codec/src/types/data.ts (1)
172-181: Missing range validation for UInt32 values.Unlike the UInt8 and UInt16 cases, the UInt32 case doesn't validate that the parsed value is within the valid range (0 to 4294967295). While
parseIntwith radix 10 handles most cases,NaNchecking is missing.🛡️ Proposed fix to add validation
case SerializedTypeID.STI_UINT32: { const val = typeof json.value === 'string' ? parseInt(json.value, 10) : typeof json.value === 'number' ? json.value : Number(json.value) + if ( + typeof val !== 'number' || + Number.isNaN(val) || + val < 0 || + val > 4294967295 + ) { + throw new Error('UINT32 value out of range') + } dataValue = UInt32.from(val) dataBytes = (dataValue as UInt32).toBytes() break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-binary-codec/src/types/data.ts` around lines 172 - 181, The UInt32 branch (case SerializedTypeID.STI_UINT32) is missing validation: after parsing json.value (parseInt/Number) but before calling UInt32.from and toBytes, verify the value is a finite number (not NaN), an integer, and within the allowed range 0..4294967295; if it fails, throw or handle the same error path used in the UInt8/UInt16 cases. Update the logic that produces val (the parsed value) and add the range/NaN checks prior to calling UInt32.from so only valid UInt32 values are passed to UInt32.from and toBytes.packages/ripple-binary-codec/src/types/json.ts (1)
128-143: Value type parsing is limited to nested STJson only.The
makeValueFromVLWithTypemethod only handles nested Object/Array types (0x00, 0x01). The comment mentions this is a "placeholder" - otherSerializedTypevalues will throw an error. This may be intentional for the current scope, but the limitation should be documented or expanded.Consider either:
- Documenting this limitation clearly in the class docstring
- Expanding to support other
SerializedTypevalues by dispatching to appropriatefromParsermethods🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-binary-codec/src/types/json.ts` around lines 128 - 143, The method makeValueFromVLWithType currently only handles STJson.JsonType.Object/Array and throws for all other SerializedType values; update it by either (A) documenting this limitation in the class/docstring for json.ts and improving the thrown error to explicitly state only Object/Array are supported, or (B) implement a proper dispatch: read typeId in makeValueFromVLWithType, map known SerializedType IDs to their concrete classes' fromParser methods (e.g., UInt32.fromParser, Hash160.fromParser, STAmount.fromParser, STJson.fromParser, etc.), and call the appropriate fromParser for supported types (fall back to a clear Unsupported type error listing supported IDs). Ensure you reference and use the makeValueFromVLWithType function and STJson.JsonType symbols when making the change.
🤖 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/ripple-binary-codec/src/types/json.ts`:
- Around line 352-357: The code extends the array by setting array.length =
index + 1 and then calls array.fill(null), which overwrites all existing
elements; fix by preserving the old length and only filling the newly added
slots (e.g. const oldLen = array.length; array.length = index + 1;
array.fill(null, oldLen)) or equivalently use a for-loop to set null for i from
oldLen to array.length-1; apply this change to the block that casts this.data to
(SerializedType | null)[] and modifies array length so only new entries are
initialized to null.
- Around line 329-336: The resize logic in setArrayElement is overwriting
existing elements by calling array.fill(null) after changing array.length;
instead, preserve existing entries: when index >= array.length record the old
length (const oldLen = array.length), then set array.length = index + 1 and only
assign null to the new slots (for i from oldLen to index set array[i] = null),
then set array[index] = value; update the code that manipulates this.data /
array to remove the array.fill(null) call and use this targeted loop to avoid
wiping A, B, C when expanding.
In `@packages/ripple-binary-codec/src/types/serialized-type.ts`:
- Around line 144-145: The base SerializedType.getSType() currently calls
this.getSType() recursively and will overflow if a subclass forgets to override
it; replace the recursive call with a non-recursive failure path: have
SerializedType.getSType() throw a clear error (e.g., "getSType must be
implemented by subclass") or otherwise return a meaningful default, so missing
overrides surface a clear message; update the method in the SerializedType class
(getSType) to stop the recursion and surface the error instead.
In `@packages/xrpl/src/models/transactions/contractClawback.ts`:
- Around line 29-35: Re-enable validation in validateContractClawback by
uncommenting and restoring the checks for the 'Amount' and 'ContractAccount'
fields: call validateRequiredField(tx, 'Amount', isAmount) to enforce Amount is
present and valid, and validateOptionalField(tx, 'ContractAccount', isString) to
validate ContractAccount when provided; these calls should be placed inside the
validateContractClawback function after validateBaseTransaction so client-side
validation matches the model and tests.
In `@packages/xrpl/src/models/transactions/contractDelete.ts`:
- Around line 23-26: The validateContractDelete function currently only calls
validateBaseTransaction and is missing the required field check for
ContractAccount; re-enable the required validation by calling
validateRequiredField(tx, 'ContractAccount', isString) inside
validateContractDelete (uncomment or add that line), and ensure
validateRequiredField and isString are imported or available in this module so
ContractDelete payloads without ContractAccount fail client-side validation.
In `@packages/xrpl/src/sugar/getFeeXrp.ts`:
- Around line 68-76: The current guard in getFeeXrp.ts allows null because
typeof null === 'object', so add an explicit null check before using meta:
verify response.result.meta is not null and is an object (e.g., change the
condition that throws the XrplError to check response.result.meta === null ||
typeof response.result.meta !== 'object'), then proceed to assign const meta =
response.result.meta and keep the existing XrplError throw for missing GasUsed;
this ensures a controlled XrplError is raised instead of a TypeError when meta
is null.
In `@packages/xrpl/src/Wallet/utils.ts`:
- Around line 104-126: There is a duplicate exported function computeSignature
causing a TypeScript compile error; remove the duplicate declaration and keep
the single complete implementation (the one that handles multisign via
isValidXAddress/xAddressToClassicAddress and calls encodeForMultisigning or
encodeForSigning and sign) so only one exported computeSignature remains,
ensuring all referenced helpers (encodeForMultisigning, encodeForSigning, sign,
isValidXAddress, xAddressToClassicAddress) still resolve.
In `@packages/xrpl/test/models/contractCall.test.ts`:
- Line 4: The test imports the module using the wrong case; change the import
path in the test from '../../src/models/transactions/ContractCall' to the exact
lowercase module name '../../src/models/transactions/contractCall' so
validateContractCall resolves on case-sensitive filesystems, and scan for any
other imports of ContractCall to update them similarly.
In `@packages/xrpl/test/models/contractClawback.test.ts`:
- Line 4: The import path in the test imports validateContractClawback from
'../../src/models/transactions/ContractClawback' which mismatches the actual
source file name; update the import to use the exact lowercase filename
'../../src/models/transactions/contractClawback' so validateContractClawback is
resolved on case-sensitive filesystems and CI.
In `@packages/xrpl/test/models/contractCreate.test.ts`:
- Around line 53-66: The test contains an incomplete assignment for tx.Functions
causing a syntax error; replace the TODO with an explicit invalid value (for
example a non-object or malformed value) so the validator fails: set
tx.Functions to a clearly invalid value (e.g. a number or an empty
object/string) before calling validateContractCreate(tx) and validate(tx) to
ensure both validateContractCreate and validate throw ValidationError for the
'ContractCreate: invalid field Functions' case.
- Around line 68-81: The test contains an incomplete assignment that won't
compile: replace the placeholder tx.InstanceParameters = /*TODO*/ with a
concrete invalid value so the validators throw; for example assign a wrong type
(e.g., a string or number) or an object missing required fields to
tx.InstanceParameters so validateContractCreate(tx) and validate(tx) raise
ValidationError. Update the test case in contractCreate.test.ts to set
tx.InstanceParameters to that invalid value and keep the existing assert.throws
checks using validateContractCreate and validate.
- Around line 83-96: The test contains a syntax error because
tx.InstanceParameterValues is left unassigned; replace the placeholder with a
concrete invalid value so the test compiles and triggers validation errors —
e.g., set tx.InstanceParameterValues = 'invalid' (or another wrong type such as
42 or {}), then keep the existing assertions that call
validateContractCreate(tx) and validate(tx) to expect ValidationError for
InstanceParameterValues.
In `@packages/xrpl/test/models/contractModify.test.ts`:
- Around line 15-16: The test is incomplete because the shared fixture tx in
beforeEach is just an empty object and several assertions assign `= /*TODO*/`
with no values; replace that by initializing a concrete baseline transaction
object inside the beforeEach (use the same `tx` variable used by the tests)
containing all required fields for the contract modify scenario, and then in
each specific test replace the TODO placeholders by explicit invalid values
(e.g., numeric values or other wrong types) to exercise validation; locate
references to tx and the beforeEach block and update the test cases that
currently set fields to `/*TODO*/` so they assign explicit invalid values
instead.
In `@packages/xrpl/test/models/contractUserDelete.test.ts`:
- Line 4: The import path casing is wrong: change the import in this test that
currently references '../../src/models/transactions/ContractUserDelete' to the
actual lowercase module '../../src/models/transactions/contractUserDelete' so
the test imports validateContractUserDelete from the correct file name
(validateContractUserDelete / contractUserDelete) and avoids Linux CI import
failures.
---
Major comments:
In `@packages/ripple-binary-codec/src/types/data.ts`:
- Around line 337-344: The const declared in the SerializedTypeID.STI_VL switch
branch (valueVL from parser.readVariableLength()) must be confined to its own
block to avoid leaking into other cases; wrap the case body for
SerializedTypeID.STI_VL in braces { ... } so valueVL, dataValue, and dataBytes
are block-scoped, keep the existing assignments to dataValue
(Blob.from(bytesToHex(valueVL))), dataBytes
(concat([BinarySerializer.encodeVariableLength(valueVL.length), valueVL])) and
the break inside the new block; this prevents cross-case access while preserving
behavior of parser.readVariableLength(), BinarySerializer.encodeVariableLength,
bytesToHex, Blob.from and concat.
In `@packages/ripple-binary-codec/src/types/json.ts`:
- Around line 588-593: isEquivalent on STJson incorrectly compares
constructor-populated this.bytes instead of the actual JSON payload, so
instances created via setObjectField (which store data in this.data) can be
falsely equal; update isEquivalent to compare the serialized form of both
instances (use the class's serialize/toBytes/getBytes method or fall back to
this.data when present) — e.g., obtain bytes for both this and t by calling the
STJson serialization accessor and compare those byte arrays (referencing
isEquivalent, STJson, this.bytes, this.data, and setObjectField to locate the
logic).
In `@packages/ripple-binary-codec/test/binary-parser.test.ts`:
- Line 11: The test imports bytesToHex from the wrong package name; change the
require call that currently imports bytesToHex from '@transia/isomorphic/utils'
back to the declared monorepo package '@xrplf/isomorphic/utils' so module
resolution succeeds (update the require for bytesToHex in the test file).
In `@packages/xrpl/src/client/index.ts`:
- Around line 697-699: The condition that autofills ComputationAllowance treats
0 as missing; update the check in the block where tx.TransactionType ===
'ContractCall' (the branch that pushes getComputationAllowance(this, tx)) to use
a nullish check (tx.ComputationAllowance == null) instead of
!tx.ComputationAllowance so explicit zero values provided by callers are
preserved when autofilling.
In `@packages/xrpl/test/models/contractCall.test.ts`:
- Around line 14-23: Replace the placeholder tx in the beforeEach with a
complete, valid ContractCall payload so the "valid" assertions exercise the
validators (not a no-op); populate tx with the required fields that
validateContractCall/validate expect (e.g., TransactionType: 'ContractCall',
Account, Fee, Sequence, Contract, Method, Params) so subsequent tests that
delete fields are removing real properties; update the beforeEach initialization
of tx (used by validateContractCall and validate) to this realistic example.
- Around line 115-128: The test leaves tx.Parameters assignment incomplete so
tx.Parameters ends up being the result of assert.throws and the invalid payload
is never set; set tx.Parameters to a concrete invalid value (for example a wrong
type or malformed structure such as a plain number, string, or an object missing
required fields) before calling validateContractCall and validate so both throw
ValidationError; update the test to assign the invalid value to tx.Parameters
(referencing tx.Parameters) and then call validateContractCall(tx) and
validate(tx) expecting ValidationError.
In `@packages/xrpl/test/models/contractClawback.test.ts`:
- Around line 14-21: The test uses an empty tx so ContractClawback-specific
validations aren’t exercised; initialize tx in beforeEach with a known-valid
ContractClawback fixture (or build a valid object) instead of {} so subsequent
mutations (e.g., deleting Amount) actually test ContractClawback rules; update
the beforeEach fixture setup referenced by tx and keep the asserts calling
validateContractClawback(tx) and validate(tx) unchanged so the suite verifies
ContractClawback-specific validation logic.
In `@packages/xrpl/test/models/contractDelete.test.ts`:
- Around line 14-21: The test uses an empty tx so
validateContractDelete/validate will fail; update the beforeEach to construct a
minimal valid ContractDelete transaction object including required base fields
TransactionType and Account (e.g., TransactionType: "ContractDelete", Account:
"<valid address>") and the contract-specific field ContractAccount (set to a
valid account/address), plus any other required base fields
(Sequence/Fee/SigningPubKey or stubbed equivalents) so that
validateBaseTransaction and validateContractDelete succeed when called in the
'verifies valid ContractDelete' test; ensure the object matches the expected
shape used by validateContractDelete and cast to the same any/type as before.
In `@packages/xrpl/test/models/contractUserDelete.test.ts`:
- Around line 14-21: The test seeds tx with an empty object causing false
positives; replace the placeholder in beforeEach so tx is cloned from a
known-good ContractUserDelete fixture (use the project fixture helper or a JSON
fixture for ContractUserDelete) and then use that valid tx in the “verifies
valid ContractUserDelete” case to call validateContractUserDelete(tx) and
validate(tx). For the missing-field tests, start from the same cloned valid
fixture and remove the specific field under test (rather than starting from {}),
so validateContractUserDelete/validateBaseTransaction reject the intended
missing-field and not some other base-field first.
- Around line 113-125: Replace the placeholder RHS for tx.Parameters with a
concrete invalid value (e.g., a wrong type like 123 or an empty object) so the
test actually exercises validation; set tx.Parameters = 123 (or {}) and keep the
existing assertions that validateContractUserDelete(tx) and validate(tx) throw
ValidationError to confirm the field is rejected. Ensure you update the
assignment in the test where tx.Parameters is currently /*TODO*/ and leave the
assert.throws lines unchanged.
---
Minor comments:
In `@packages/ripple-binary-codec/src/types/data.ts`:
- Around line 428-446: The switch branch for SerializedTypeID.STI_VL declares
const vlLength at case scope which can leak—wrap that branch body in braces
(e.g., case SerializedTypeID.STI_VL: { const vlLength =
parser.readVariableLengthLength(); return Blob.fromParser(parser, vlLength); } )
and update the thrown error in Data.getValue() to include the actual innerType
value instead of typeof innerType (e.g., interpolate innerType into the message)
so the error reports the real type ID.
In `@packages/ripple-binary-codec/src/types/dataType.ts`:
- Around line 147-150: setInnerType currently mutates this.bytes via
writeUInt16BE which corrupts the shared DataType.defaultBytes for other
instances; to fix, ensure instances do not share the static buffer by making a
defensive copy of DataType.defaultBytes in the constructor when bytes is not
provided (or when this.bytes === DataType.defaultBytes), or alternatively have
setInnerType perform a copy-on-write (clone this.bytes into a new Buffer before
calling writeUInt16BE) so that setInnerType always mutates an instance-local
buffer rather than the shared defaultBytes.
In `@packages/xrpl/src/models/ledger/ContractSource.ts`:
- Around line 4-8: The JSDoc for the ledger entry type ContractSource is empty;
add a concise description above the ContractSource declaration explaining what
this ledger entry represents (e.g., stores on-ledger smart contract
bytecode/source and metadata for XRPL contracts), include any relevant usage or
important fields it models, and keep the `@category` Ledger Entries tag — update
the JSDoc block immediately preceding the ContractSource type/class so readers
and docs generators get a meaningful summary.
In `@packages/xrpl/src/models/transactions/contractCall.ts`:
- Line 18: Fix the typo in the inline comment in
packages/xrpl/src/models/transactions/contractCall.ts: change "beacuse" to
"because" in the comment that reads "// Optional beacuse autofill will add it if
missing" (update the comment text within the ContractCall/contract call-related
code to the corrected spelling).
In `@packages/xrpl/src/sugar/getFeeXrp.ts`:
- Line 53: The JSDoc for getGasEstimate in getFeeXrp.ts is incorrect: it says
the function "returns the estimated gas as a string" but getGasEstimate actually
returns a Promise<number>; update the `@returns` description to accurately state
it returns a Promise<number> (or "the estimated gas as a number" / "a Promise
resolving to a number") so the documentation matches the function signature and
behavior.
---
Nitpick comments:
In `@packages/ripple-binary-codec/src/types/data.ts`:
- Around line 172-181: The UInt32 branch (case SerializedTypeID.STI_UINT32) is
missing validation: after parsing json.value (parseInt/Number) but before
calling UInt32.from and toBytes, verify the value is a finite number (not NaN),
an integer, and within the allowed range 0..4294967295; if it fails, throw or
handle the same error path used in the UInt8/UInt16 cases. Update the logic that
produces val (the parsed value) and add the range/NaN checks prior to calling
UInt32.from so only valid UInt32 values are passed to UInt32.from and toBytes.
In `@packages/ripple-binary-codec/src/types/dataType.ts`:
- Around line 44-59: The constructor in DataType currently bypasses type safety
by using readUInt16BE(used, 0) as unknown as SerializedTypeID; instead, read the
16-bit value with readUInt16BE(used, 0), validate that the numeric value is a
known/allowed SerializedTypeID (e.g., check against the SerializedTypeID enum or
a lookup set), and only then assign it to this.innerType with a single safe cast
or direct assignment; if the value is not valid, throw a descriptive Error.
Locate this logic in the DataType constructor (constructor, innerType field) and
replace the double-cast with explicit validation and a clear error path.
In `@packages/ripple-binary-codec/src/types/json.ts`:
- Around line 128-143: The method makeValueFromVLWithType currently only handles
STJson.JsonType.Object/Array and throws for all other SerializedType values;
update it by either (A) documenting this limitation in the class/docstring for
json.ts and improving the thrown error to explicitly state only Object/Array are
supported, or (B) implement a proper dispatch: read typeId in
makeValueFromVLWithType, map known SerializedType IDs to their concrete classes'
fromParser methods (e.g., UInt32.fromParser, Hash160.fromParser,
STAmount.fromParser, STJson.fromParser, etc.), and call the appropriate
fromParser for supported types (fall back to a clear Unsupported type error
listing supported IDs). Ensure you reference and use the makeValueFromVLWithType
function and STJson.JsonType symbols when making the change.
In `@packages/ripple-binary-codec/test/st-data.test.ts`:
- Around line 202-485: The test file contains large blocks of commented-out
tests (e.g., describe('AMOUNT'), describe('CURRENCY'), describe('ISSUE'), error
handling, Data equality, and Data.getValue checks that reference Data.from,
BinaryParser, and getValue) which should be cleaned up: either remove these
commented tests and open a follow-up issue to re-add them when features are
implemented, or convert them to pending tests using it.skip()/describe.skip() so
they remain visible in test output; update the test file to apply one consistent
approach (remove or skip) for the blocks named AMOUNT, CURRENCY, ISSUE,
Number/Complex roundtrip/Error handling/Data equality/Data getValue to keep the
test suite tidy and actionable.
In `@packages/xrpl/src/models/common/index.ts`:
- Around line 330-335: The exported interface named Function conflicts with the
global JS/TS Function and should be renamed to a clearer public type (e.g.,
ContractFunction); update the exported declaration (change interface Function {
Function: { ... } } to interface ContractFunction { Function: { FunctionName:
string; Parameters?: Parameter[] } }) and then update all imports/usages of that
type in the three consumers (contractCreate.ts, contractModify.ts,
ContractSource.ts) to import and reference ContractFunction instead of Function;
ensure any type annotations, export statements, and re-exports are adjusted
accordingly so compilation and typings remain correct.
In `@packages/xrpl/src/models/ledger/ContractData.ts`:
- Around line 3-10: The JSDoc for the ContractData ledger entry is empty; update
the comment block above the ContractData interface to include a concise
description of what a ContractData ledger entry represents (e.g., purpose, key
fields, and relation to BaseLedgerEntry and HasPreviousTxnID), so consumers and
generated docs can understand this type; edit the JSDoc immediately above the
ContractData interface declaration to replace the placeholder with a meaningful
summary and any relevant tags like `@category` Ledger Entries.
In `@packages/xrpl/src/models/methods/contractInfo.ts`:
- Around line 24-29: Remove the informal development note inside the
ContractInfoResponse interface: delete the "// IDEA" comment block (the
commented potential fields for contract, contract_source, contract_account) or
convert it to a proper TODO/FIXME comment if you intend to track it, so the
ContractInfoResponse (which extends BaseResponse) no longer contains stray dev
comments.
- Line 34: The field functions is currently typed as any[] which discards type
safety; change its type to the shared Function type from ../common (i.e.,
replace functions: any[] with functions: Function[] or the exact exported name
used in ../common) in the contractInfo model so it matches other contract models
and preserves shape information for the contract functions.
In `@packages/xrpl/src/models/transactions/contractCall.ts`:
- Around line 34-44: The validateContractCall function currently only invokes
validateBaseTransaction and has all specific field validators commented out;
restore the runtime checks by re-enabling the validators in
validateContractCall: call validateRequiredField(tx, 'ContractAccount',
isString) and validateRequiredField(tx, 'FunctionName', isString), keep
ComputationAllowance optional (validateOptionalField(tx, 'ComputationAllowance',
isNumber) or leave commented if intentionally autofilled), and
validateOptionalField(tx, 'Parameters', isAnyArray) (or the existing isany[]
helper) so ContractAccount and FunctionName are enforced as strings and
Parameters is validated when present.
In `@packages/xrpl/src/sugar/autofill.ts`:
- Line 426: Extract the magic number 100000000000 used to set
copyTx.ComputationAllowance into a clearly named constant (e.g.,
DEFAULT_COMPUTATION_ALLOWANCE or PLACEHOLDER_COMPUTATION_ALLOWANCE) with a brief
doc comment explaining it's a placeholder for gas estimation and why that
specific value was chosen; replace the inline literal in the autofill logic
(where copyTx.ComputationAllowance is assigned) with that constant and update
any related tests or references to use the new constant name.
- Around line 421-431: getComputationAllowance currently uses a shallow copy
(copyTx = { ...tx }) which can mutate nested properties on tx and also doesn't
explicitly set Fee to zero as the JSDoc implies; change the function to create a
safe copy (either a deep clone of tx or a documented precondition) and
explicitly set copyTx.Fee = "0" (or 0 depending on encoding expectations) before
setting copyTx.ComputationAllowance and calling encode/getGasEstimate; update
references in the function (copyTx, tx, ComputationAllowance, Fee, encode,
getGasEstimate) accordingly so nested fields like Memos aren't accidentally
modified or document that tx must not contain nested objects if you prefer not
to deep clone.
🪄 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: 976ae45b-bd35-4671-b9b9-eb79b744e599
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (60)
packages/ripple-binary-codec/src/enums/definitions.jsonpackages/ripple-binary-codec/src/serdes/binary-serializer.tspackages/ripple-binary-codec/src/types/account-id.tspackages/ripple-binary-codec/src/types/amount.tspackages/ripple-binary-codec/src/types/blob.tspackages/ripple-binary-codec/src/types/currency.tspackages/ripple-binary-codec/src/types/data.tspackages/ripple-binary-codec/src/types/dataType.tspackages/ripple-binary-codec/src/types/hash-128.tspackages/ripple-binary-codec/src/types/hash-160.tspackages/ripple-binary-codec/src/types/hash-192.tspackages/ripple-binary-codec/src/types/hash-256.tspackages/ripple-binary-codec/src/types/index.tspackages/ripple-binary-codec/src/types/issue.tspackages/ripple-binary-codec/src/types/json.tspackages/ripple-binary-codec/src/types/path-set.tspackages/ripple-binary-codec/src/types/serialized-type.tspackages/ripple-binary-codec/src/types/st-array.tspackages/ripple-binary-codec/src/types/st-number.tspackages/ripple-binary-codec/src/types/st-object.tspackages/ripple-binary-codec/src/types/uint-16.tspackages/ripple-binary-codec/src/types/uint-32.tspackages/ripple-binary-codec/src/types/uint-64.tspackages/ripple-binary-codec/src/types/uint-8.tspackages/ripple-binary-codec/src/types/vector-256.tspackages/ripple-binary-codec/src/types/xchain-bridge.tspackages/ripple-binary-codec/test/binary-parser.test.tspackages/ripple-binary-codec/test/binary-serializer.test.tspackages/ripple-binary-codec/test/fixtures/contract-call-binary.jsonpackages/ripple-binary-codec/test/fixtures/contract-call-tx.jsonpackages/ripple-binary-codec/test/quality.test.tspackages/ripple-binary-codec/test/st-data.test.tspackages/ripple-binary-codec/tools/generateDefinitions.jspackages/xrpl/src/Wallet/utils.tspackages/xrpl/src/client/index.tspackages/xrpl/src/models/common/index.tspackages/xrpl/src/models/ledger/AccountRoot.tspackages/xrpl/src/models/ledger/Contract.tspackages/xrpl/src/models/ledger/ContractData.tspackages/xrpl/src/models/ledger/ContractSource.tspackages/xrpl/src/models/methods/contractInfo.tspackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/contractCall.tspackages/xrpl/src/models/transactions/contractClawback.tspackages/xrpl/src/models/transactions/contractCreate.tspackages/xrpl/src/models/transactions/contractDelete.tspackages/xrpl/src/models/transactions/contractModify.tspackages/xrpl/src/models/transactions/contractUserDelete.tspackages/xrpl/src/models/transactions/index.tspackages/xrpl/src/models/transactions/metadata.tspackages/xrpl/src/models/transactions/transaction.tspackages/xrpl/src/sugar/autofill.tspackages/xrpl/src/sugar/getFeeXrp.tspackages/xrpl/test/models/contractCall.test.tspackages/xrpl/test/models/contractClawback.test.tspackages/xrpl/test/models/contractCreate.test.tspackages/xrpl/test/models/contractDelete.test.tspackages/xrpl/test/models/contractModify.test.tspackages/xrpl/test/models/contractUserDelete.test.tspackages/xrpl/tools/generateModels.js
Comment on lines
+329
to
+336
| const array = this.data as (SerializedType | null)[] | ||
| // Auto-resize with nulls if needed | ||
| if (index >= array.length) { | ||
| array.length = index + 1 | ||
| array.fill(null) | ||
| } | ||
| array[index] = value | ||
| } |
Contributor
There was a problem hiding this comment.
Critical bug: array.fill(null) overwrites existing elements.
When auto-resizing the array, array.fill(null) fills the entire array with nulls, destroying any existing elements. For example, if the array is [A, B, C] and you call setArrayElement(5, D), the result would be [null, null, null, null, null, D] instead of [A, B, C, null, null, D].
🐛 Proposed fix
const array = this.data as (SerializedType | null)[]
// Auto-resize with nulls if needed
if (index >= array.length) {
+ const oldLength = array.length
array.length = index + 1
- array.fill(null)
+ array.fill(null, oldLength)
}
array[index] = value📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const array = this.data as (SerializedType | null)[] | |
| // Auto-resize with nulls if needed | |
| if (index >= array.length) { | |
| array.length = index + 1 | |
| array.fill(null) | |
| } | |
| array[index] = value | |
| } | |
| const array = this.data as (SerializedType | null)[] | |
| // Auto-resize with nulls if needed | |
| if (index >= array.length) { | |
| const oldLength = array.length | |
| array.length = index + 1 | |
| array.fill(null, oldLength) | |
| } | |
| array[index] = value | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ripple-binary-codec/src/types/json.ts` around lines 329 - 336, The
resize logic in setArrayElement is overwriting existing elements by calling
array.fill(null) after changing array.length; instead, preserve existing
entries: when index >= array.length record the old length (const oldLen =
array.length), then set array.length = index + 1 and only assign null to the new
slots (for i from oldLen to index set array[i] = null), then set array[index] =
value; update the code that manipulates this.data / array to remove the
array.fill(null) call and use this targeted loop to avoid wiping A, B, C when
expanding.
Comment on lines
+352
to
+357
| const array = this.data as (SerializedType | null)[] | ||
| // Auto-resize with nulls if needed | ||
| if (index >= array.length) { | ||
| array.length = index + 1 | ||
| array.fill(null) | ||
| } |
Contributor
There was a problem hiding this comment.
Same array.fill bug exists here.
This is the same issue as in setArrayElement - the array.fill(null) call overwrites all existing elements.
🐛 Proposed fix
const array = this.data as (SerializedType | null)[]
// Auto-resize with nulls if needed
if (index >= array.length) {
+ const oldLength = array.length
array.length = index + 1
- array.fill(null)
+ array.fill(null, oldLength)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const array = this.data as (SerializedType | null)[] | |
| // Auto-resize with nulls if needed | |
| if (index >= array.length) { | |
| array.length = index + 1 | |
| array.fill(null) | |
| } | |
| const array = this.data as (SerializedType | null)[] | |
| // Auto-resize with nulls if needed | |
| if (index >= array.length) { | |
| const oldLength = array.length | |
| array.length = index + 1 | |
| array.fill(null, oldLength) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ripple-binary-codec/src/types/json.ts` around lines 352 - 357, The
code extends the array by setting array.length = index + 1 and then calls
array.fill(null), which overwrites all existing elements; fix by preserving the
old length and only filling the newly added slots (e.g. const oldLen =
array.length; array.length = index + 1; array.fill(null, oldLen)) or
equivalently use a for-loop to set null for i from oldLen to array.length-1;
apply this change to the block that casts this.data to (SerializedType | null)[]
and modifies array length so only new entries are initialized to null.
Comment on lines
+144
to
+145
| getSType(): SerializedTypeID { | ||
| return this.getSType() |
Contributor
There was a problem hiding this comment.
Stop the base getSType() from recursing forever.
Line 145 just calls this.getSType() again, so any subtype that misses an override will blow the stack instead of surfacing a useful error.
🔧 Proposed fix
getSType(): SerializedTypeID {
- return this.getSType()
+ throw new Error('getSType not implemented')
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| getSType(): SerializedTypeID { | |
| return this.getSType() | |
| getSType(): SerializedTypeID { | |
| throw new Error('getSType not implemented') | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ripple-binary-codec/src/types/serialized-type.ts` around lines 144 -
145, The base SerializedType.getSType() currently calls this.getSType()
recursively and will overflow if a subclass forgets to override it; replace the
recursive call with a non-recursive failure path: have SerializedType.getSType()
throw a clear error (e.g., "getSType must be implemented by subclass") or
otherwise return a meaningful default, so missing overrides surface a clear
message; update the method in the SerializedType class (getSType) to stop the
recursion and surface the error instead.
Comment on lines
+29
to
+35
| export function validateContractClawback(tx: Record<string, unknown>): void { | ||
| validateBaseTransaction(tx) | ||
|
|
||
| // validateRequiredField(tx, 'Amount', isAmount) | ||
|
|
||
| // validateOptionalField(tx, 'ContractAccount', isString) | ||
| } |
Contributor
There was a problem hiding this comment.
Re-enable the Amount and ContractAccount checks here.
With Lines 32-34 commented out, malformed ContractClawback payloads pass client-side validation even though this model and the tests added in the same PR treat those fields as validated.
🔧 Proposed fix
import {
BaseTransaction,
- // isAmount,
- // isString,
+ isAmount,
+ isString,
validateBaseTransaction,
- // validateOptionalField,
- // validateRequiredField,
+ validateOptionalField,
+ validateRequiredField,
} from './common'
@@
export function validateContractClawback(tx: Record<string, unknown>): void {
validateBaseTransaction(tx)
- // validateRequiredField(tx, 'Amount', isAmount)
-
- // validateOptionalField(tx, 'ContractAccount', isString)
+ validateRequiredField(tx, 'Amount', isAmount)
+ validateOptionalField(tx, 'ContractAccount', isString)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/models/transactions/contractClawback.ts` around lines 29 -
35, Re-enable validation in validateContractClawback by uncommenting and
restoring the checks for the 'Amount' and 'ContractAccount' fields: call
validateRequiredField(tx, 'Amount', isAmount) to enforce Amount is present and
valid, and validateOptionalField(tx, 'ContractAccount', isString) to validate
ContractAccount when provided; these calls should be placed inside the
validateContractClawback function after validateBaseTransaction so client-side
validation matches the model and tests.
Comment on lines
+23
to
+26
| export function validateContractDelete(tx: Record<string, unknown>): void { | ||
| validateBaseTransaction(tx) | ||
|
|
||
| // validateRequiredField(tx, 'ContractAccount', isString) |
Contributor
There was a problem hiding this comment.
Re-enable the required ContractAccount check.
Right now this validator only runs validateBaseTransaction(), so ContractDelete payloads missing ContractAccount still pass client-side validation.
🔧 Proposed fix
import {
BaseTransaction,
- // isString,
+ isString,
validateBaseTransaction,
- // validateRequiredField,
+ validateRequiredField,
} from './common'
@@
export function validateContractDelete(tx: Record<string, unknown>): void {
validateBaseTransaction(tx)
- // validateRequiredField(tx, 'ContractAccount', isString)
+ validateRequiredField(tx, 'ContractAccount', isString)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/models/transactions/contractDelete.ts` around lines 23 -
26, The validateContractDelete function currently only calls
validateBaseTransaction and is missing the required field check for
ContractAccount; re-enable the required validation by calling
validateRequiredField(tx, 'ContractAccount', isString) inside
validateContractDelete (uncomment or add that line), and ensure
validateRequiredField and isString are imported or available in this module so
ContractDelete payloads without ContractAccount fail client-side validation.
Comment on lines
+53
to
+66
| it('throws w/ invalid Functions', function () { | ||
| tx.Functions = /*TODO*/ | ||
|
|
||
| assert.throws( | ||
| () => validateContractCreate(tx), | ||
| ValidationError, | ||
| 'ContractCreate: invalid field Functions', | ||
| ) | ||
| assert.throws( | ||
| () => validate(tx), | ||
| ValidationError, | ||
| 'ContractCreate: invalid field Functions', | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
Syntax error: incomplete assignment.
tx.Functions = /*TODO*/ is invalid JavaScript/TypeScript syntax. The assignment statement is incomplete and will cause a compilation error.
🐛 Proposed fix
it('throws w/ invalid Functions', function () {
- tx.Functions = /*TODO*/
+ tx.Functions = 123📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('throws w/ invalid Functions', function () { | |
| tx.Functions = /*TODO*/ | |
| assert.throws( | |
| () => validateContractCreate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field Functions', | |
| ) | |
| assert.throws( | |
| () => validate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field Functions', | |
| ) | |
| }) | |
| it('throws w/ invalid Functions', function () { | |
| tx.Functions = 123 | |
| assert.throws( | |
| () => validateContractCreate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field Functions', | |
| ) | |
| assert.throws( | |
| () => validate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field Functions', | |
| ) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/test/models/contractCreate.test.ts` around lines 53 - 66, The
test contains an incomplete assignment for tx.Functions causing a syntax error;
replace the TODO with an explicit invalid value (for example a non-object or
malformed value) so the validator fails: set tx.Functions to a clearly invalid
value (e.g. a number or an empty object/string) before calling
validateContractCreate(tx) and validate(tx) to ensure both
validateContractCreate and validate throw ValidationError for the
'ContractCreate: invalid field Functions' case.
Comment on lines
+68
to
+81
| it('throws w/ invalid InstanceParameters', function () { | ||
| tx.InstanceParameters = /*TODO*/ | ||
|
|
||
| assert.throws( | ||
| () => validateContractCreate(tx), | ||
| ValidationError, | ||
| 'ContractCreate: invalid field InstanceParameters', | ||
| ) | ||
| assert.throws( | ||
| () => validate(tx), | ||
| ValidationError, | ||
| 'ContractCreate: invalid field InstanceParameters', | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
Syntax error: incomplete assignment.
Same issue as above - tx.InstanceParameters = /*TODO*/ will not compile.
🐛 Proposed fix
it('throws w/ invalid InstanceParameters', function () {
- tx.InstanceParameters = /*TODO*/
+ tx.InstanceParameters = 123📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('throws w/ invalid InstanceParameters', function () { | |
| tx.InstanceParameters = /*TODO*/ | |
| assert.throws( | |
| () => validateContractCreate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field InstanceParameters', | |
| ) | |
| assert.throws( | |
| () => validate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field InstanceParameters', | |
| ) | |
| }) | |
| it('throws w/ invalid InstanceParameters', function () { | |
| tx.InstanceParameters = 123 | |
| assert.throws( | |
| () => validateContractCreate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field InstanceParameters', | |
| ) | |
| assert.throws( | |
| () => validate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field InstanceParameters', | |
| ) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/test/models/contractCreate.test.ts` around lines 68 - 81, The
test contains an incomplete assignment that won't compile: replace the
placeholder tx.InstanceParameters = /*TODO*/ with a concrete invalid value so
the validators throw; for example assign a wrong type (e.g., a string or number)
or an object missing required fields to tx.InstanceParameters so
validateContractCreate(tx) and validate(tx) raise ValidationError. Update the
test case in contractCreate.test.ts to set tx.InstanceParameters to that invalid
value and keep the existing assert.throws checks using validateContractCreate
and validate.
Comment on lines
+83
to
+96
| it('throws w/ invalid InstanceParameterValues', function () { | ||
| tx.InstanceParameterValues = /*TODO*/ | ||
|
|
||
| assert.throws( | ||
| () => validateContractCreate(tx), | ||
| ValidationError, | ||
| 'ContractCreate: invalid field InstanceParameterValues', | ||
| ) | ||
| assert.throws( | ||
| () => validate(tx), | ||
| ValidationError, | ||
| 'ContractCreate: invalid field InstanceParameterValues', | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
Syntax error: incomplete assignment.
Same issue - tx.InstanceParameterValues = /*TODO*/ will not compile.
🐛 Proposed fix
it('throws w/ invalid InstanceParameterValues', function () {
- tx.InstanceParameterValues = /*TODO*/
+ tx.InstanceParameterValues = 123📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('throws w/ invalid InstanceParameterValues', function () { | |
| tx.InstanceParameterValues = /*TODO*/ | |
| assert.throws( | |
| () => validateContractCreate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field InstanceParameterValues', | |
| ) | |
| assert.throws( | |
| () => validate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field InstanceParameterValues', | |
| ) | |
| }) | |
| it('throws w/ invalid InstanceParameterValues', function () { | |
| tx.InstanceParameterValues = 123 | |
| assert.throws( | |
| () => validateContractCreate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field InstanceParameterValues', | |
| ) | |
| assert.throws( | |
| () => validate(tx), | |
| ValidationError, | |
| 'ContractCreate: invalid field InstanceParameterValues', | |
| ) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/test/models/contractCreate.test.ts` around lines 83 - 96, The
test contains a syntax error because tx.InstanceParameterValues is left
unassigned; replace the placeholder with a concrete invalid value so the test
compiles and triggers validation errors — e.g., set tx.InstanceParameterValues =
'invalid' (or another wrong type such as 42 or {}), then keep the existing
assertions that call validateContractCreate(tx) and validate(tx) to expect
ValidationError for InstanceParameterValues.
Comment on lines
+15
to
+16
| tx = { /* TODO: add sample transaction */ } as any | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify TODO placeholders and incomplete assignments in this test file.
fd -i 'contractModify.test.ts' | xargs -r rg -n 'TODO|=\s*/\*TODO\*/'Repository: XRPLF/xrpl.js
Length of output: 230
🏁 Script executed:
cat -n packages/xrpl/test/models/contractModify.test.tsRepository: XRPLF/xrpl.js
Length of output: 4070
🏁 Script executed:
fd 'ContractModify.ts' -type f | head -5Repository: XRPLF/xrpl.js
Length of output: 227
🏁 Script executed:
fd 'ContractModify.ts' packages/xrpl/src/Repository: XRPLF/xrpl.js
Length of output: 39
🏁 Script executed:
rg -l 'ContractModify' packages/xrpl/src/ | head -10Repository: XRPLF/xrpl.js
Length of output: 212
🏁 Script executed:
cat -n packages/xrpl/src/models/transactions/contractModify.tsRepository: XRPLF/xrpl.js
Length of output: 1656
🏁 Script executed:
rg -A 30 'export interface BaseTransaction' packages/xrpl/src/models/transactions/common.ts | head -50Repository: XRPLF/xrpl.js
Length of output: 1255
🏁 Script executed:
ls -1 packages/xrpl/test/models/*.test.ts | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -20 {}'Repository: XRPLF/xrpl.js
Length of output: 3759
🏁 Script executed:
rg -A 5 'export (interface|type) Function' packages/xrpl/src/models/common.ts | head -20Repository: XRPLF/xrpl.js
Length of output: 131
🏁 Script executed:
rg -l 'export (interface|type) Function' packages/xrpl/src/models/ | head -5Repository: XRPLF/xrpl.js
Length of output: 97
🏁 Script executed:
rg -A 10 'export (interface|type) Function' packages/xrpl/src/models/common/index.tsRepository: XRPLF/xrpl.js
Length of output: 210
🏁 Script executed:
rg -A 10 'export (interface|type) InstanceParameter' packages/xrpl/src/models/common/index.tsRepository: XRPLF/xrpl.js
Length of output: 319
🏁 Script executed:
rg -l '/\*TODO\*/' packages/xrpl/test/models/ | wc -lRepository: XRPLF/xrpl.js
Length of output: 58
🏁 Script executed:
rg -l '/\*TODO\*/' packages/xrpl/test/models/Repository: XRPLF/xrpl.js
Length of output: 254
Fix incomplete test: provide concrete transaction object and replace TODO placeholders with invalid values.
The test has syntax errors and cannot execute. Line 15 uses an empty object as the valid baseline, and lines 69/84/99 have incomplete assignments (= /*TODO*/) missing right-hand values. Define a complete baseline transaction in beforeEach, then assign invalid values (e.g., numbers) to test field validation:
Example fix
beforeEach(function () {
- tx = { /* TODO: add sample transaction */ } as any
+ tx = {
+ TransactionType: 'ContractModify',
+ Account: 'rG1QQv2nh2gr7RCZ1P8YYcBUKCCN633jCn',
+ ContractAccount: 'rBziFEmkBf1QaVKq36EHu9BXaLGmvDC6ww',
+ ContractCode: 'DEADBEEF',
+ ContractHash:
+ 'D955DAC2E77519F05AD151A5D3C99FC8125FB39D58FF9F106F1ACA4491902C25',
+ } as any
})
@@
- tx.Functions = /*TODO*/
+ tx.Functions = 123 as any
@@
- tx.InstanceParameters = /*TODO*/
+ tx.InstanceParameters = 123 as any
@@
- tx.InstanceParameterValues = /*TODO*/
+ tx.InstanceParameterValues = 123 as any📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tx = { /* TODO: add sample transaction */ } as any | |
| }) | |
| tx = { | |
| TransactionType: 'ContractModify', | |
| Account: 'rG1QQv2nh2gr7RCZ1P8YYcBUKCCN633jCn', | |
| ContractAccount: 'rBziFEmkBf1QaVKq36EHu9BXaLGmvDC6ww', | |
| ContractCode: 'DEADBEEF', | |
| ContractHash: | |
| 'D955DAC2E77519F05AD151A5D3C99FC8125FB39D58FF9F106F1ACA4491902C25', | |
| } as any | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/test/models/contractModify.test.ts` around lines 15 - 16, The
test is incomplete because the shared fixture tx in beforeEach is just an empty
object and several assertions assign `= /*TODO*/` with no values; replace that
by initializing a concrete baseline transaction object inside the beforeEach
(use the same `tx` variable used by the tests) containing all required fields
for the contract modify scenario, and then in each specific test replace the
TODO placeholders by explicit invalid values (e.g., numeric values or other
wrong types) to exercise validation; locate references to tx and the beforeEach
block and update the test cases that currently set fields to `/*TODO*/` so they
assign explicit invalid values instead.
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
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
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan