Conversation
WalkthroughThree internal encoding-related items are exposed to the public API: type Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span three files with consistent patterns (API exposure and function exporting), but the encoding logic updates in Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/utils/parameterEncoding.ts (2)
168-179: Bug: switch fallthrough sets non-null objects to NULLIn resolveValueType, falling through from case 'object' to case 'undefined' marks any non-Uint8Array, non-null object as NULL. That silently mis-types inputs (e.g., Date, custom objects) and bypasses the error path.
Apply this guard and prevent fallthrough:
switch (typeof value) { @@ - case 'object': - if (value instanceof Uint8Array) { - varType = VarType.BYTEA; - break; - } - if (value === null) { - varType = VarType.NULL; - break; - } - case 'undefined': + case 'object': + if (value instanceof Uint8Array) { + varType = VarType.BYTEA; + break; + } + if (value === null) { + varType = VarType.NULL; + break; + } + throw new Error( + `Unsupported object type: ${Object.prototype.toString.call(value)}. If using a uuid, blob, or uint256, convert to a string or Uint8Array.` + ); + case 'undefined': varType = VarType.NULL; break;
92-104: Cannot infer type from empty arrays—throw error for ambiguous empty array parametersEmpty arrays passed to
formatDataTypewithout an override are mis-typed asVarType.NULLinstead of their intended element type, breaking API calls expecting e.g.INT8[]with zero elements.Root cause: When an empty array
[]is processed,resolveValueType(value[0])receivesundefinedat line 144, which triggers thecase 'undefined':handler at line 162 and defaults toVarType.NULL.Fix required: Add a guard in
formatDataTypeafter the override check to fail fast:function formatDataType(val: ValueType | ValueType[], o?: DataInfo): { type: DataInfo; data: ValueType; } { // handle override case if (o) { return { type: o, data: val, }; } + // Prevent ambiguous typing for empty arrays without an override + if (Array.isArray(val) && val.length === 0) { + throw new Error( + 'Cannot infer element type from an empty array. Provide a DataInfo override with the desired VarType.' + ); + }Also add a test case covering empty arrays with and without overrides to prevent regression.
src/utils/kwilEncoding.ts (1)
121-137: Guard optional parameters in encodeRawStatementstatement.parameters is used in a for..of without a null/undefined guard. This will throw if the field is absent.
- let encodedParameters: Uint8Array; - encodedParameters = numberToUint16LittleEndian( - statement.parameters ? statement.parameters.length : 0 - ); - // then, for each parameter.. - for (const param of statement.parameters) { + const params = statement.parameters ?? []; + let encodedParameters: Uint8Array = numberToUint16LittleEndian(params.length); + // then, for each parameter.. + for (const param of params) {
🧹 Nitpick comments (3)
src/utils/parameterEncoding.ts (1)
115-126: Numeric metadata derivation is fragile for e-notationanalyzeNumber uses Number.toString(); large/small magnitudes may render in scientific notation, skewing precision/scale. It also counts leading zeros toward “precision.”
Consider a safer approach (no e-notation), e.g., normalize via toLocaleString with numberingSystem 'latn' and then strip separators, or use a decimal library to compute digits and scale precisely.
src/utils/kwilEncoding.ts (2)
185-187: Typo“Concact” → “Concat”.
189-203: Endian inconsistencies are intentional — add explicit noteencodeDataType uses big-endian for version and lengths while other areas use little-endian. You added a brief note; consider linking to the kwil-db struct definition or adding a reference comment for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.ts(3 hunks)src/utils/kwilEncoding.ts(1 hunks)src/utils/parameterEncoding.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/index.ts (3)
src/core/payload.ts (1)
EncodedValue(34-37)src/utils/parameterEncoding.ts (1)
formatEncodedValue(62-81)src/utils/kwilEncoding.ts (1)
encodeEncodedValue(164-187)
src/utils/kwilEncoding.ts (2)
src/index.ts (2)
encodeEncodedValue(116-116)EncodedValue(71-71)src/core/payload.ts (1)
EncodedValue(34-37)
src/utils/parameterEncoding.ts (4)
src/index.ts (3)
formatEncodedValue(110-110)ValueType(70-70)EncodedValue(71-71)src/utils/types.ts (1)
ValueType(66-74)src/core/database.ts (1)
DataInfo(32-36)src/core/payload.ts (1)
EncodedValue(34-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
src/utils/parameterEncoding.ts (2)
54-62: Good call exposing formatEncodedValuePublic export + JSDoc make the encoding path usable externally. No functional concerns here.
62-80: Minor: consistent override usageIn formatEncodedValue (array branch), you pass o?.name to encodeValue for each element, which is correct. For non-array, you pass base.data with o?.name — also fine. Add a quick unit test to cover both override and inference paths, including UUID, BYTEA, NUMERIC with scale, and NULL.
src/utils/kwilEncoding.ts (2)
175-183: Spec mismatch: data length field width (comment says uint32, code uses uint16)encodeEncodedValue builds the data array with numberToUint16LittleEndian(ev.data.length), but the comment says “prepend 4 bytes (uint32)”. Clarify and align with kwil-db’s MarshalBinary spec to avoid interop breaks.
Two options:
- If spec is uint16 (count of elements), fix the comment.
- If spec is uint32, change the code:
- const dataLength = numberToUint16LittleEndian(ev.data.length); + const dataLength = numberToUint32BigEndian(ev.data.length); let encodedData = concatBytes(dataLength);
205-246: encodeValue: behavior surfaces look goodCovers override, UUID, null, BYTEA, decimal-as-string, and scalars. Recommend a few tests for edge cases: -0, large integers, 1e21, 1e-7, empty Uint8Array, and override with NULL when v is non-null (should still encode null per current logic).
src/index.ts (1)
42-45: Public API surface looks consistentRe-exporting Types.EncodedValue and Utils.{formatEncodedValue, encodeEncodedValue} aligns with internal implementations.
Please update CHANGELOG and public docs with:
- New exports in Utils.
- Guidance on empty-array typing (require DataInfo override).
- Example showing formatEncodedValue + encodeEncodedValue usage.
Also applies to: 106-117, 71-71
Time Submission Status
|
2 participants
resolves: https://github.com/trufnetwork/truf-network/issues/1274
Summary by CodeRabbit
EncodedValuetype,formatEncodedValue, andencodeEncodedValuefunctions for developers to handle encoded value formatting and encoding.