Skip to content

fix(calldata): reject lossy JavaScript inputs - #230

Open
maho0638 wants to merge 1 commit into
genlayerlabs:v2-devfrom
maho0638:fix/calldata-lossy-inputs
Open

maho0638 wants to merge 1 commit into
genlayerlabs:v2-devfrom
maho0638:fix/calldata-lossy-inputs

Conversation

@maho0638

@maho0638 maho0638 commented Oct 2, 2026 •

Copy link
Copy Markdown

Fixes #229

What

  • reject unsafe integer JavaScript number inputs and require bigint for exact large integers
  • reject unpaired UTF-16 surrogates before TextEncoder can replace them with U+FFFD
  • apply the same Unicode validation to map keys
  • add regression coverage for unsafe numbers, surrogate-key collisions, and valid emoji round-trips

Why

The previous encoder could silently change caller values before they reached GenVM. Unsafe JavaScript Numbers may already be rounded before conversion to bigint, and TextEncoder replaces malformed surrogate code units. The GenVM Python reference preserves integer precision and rejects unpaired surrogates.

Testing done

  • added regression coverage for unsafe integer inputs, malformed UTF-16 in values and map keys, key-collision cases, and valid Unicode round-trips
  • upstream GitHub Actions are currently awaiting approval to run for this fork PR
  • CodeRabbit review was re-requested after the earlier rate-limit response

Decisions made

  • fail fast on lossy inputs instead of silently coercing them
  • preserve normal JavaScript number support within the safe-integer range
  • require bigint when exact integer precision exceeds that range

Checks

  • I have tested this code through upstream GitHub Actions (workflow approval is still pending)
  • I have reviewed my own PR
  • I have created an issue for this PR
  • I have set a descriptive PR title compliant with conventional commits

Reviewing tips

Focus on the safe-integer guard and surrogate validation paths, especially map keys where replacement could otherwise collapse distinct JavaScript strings.

User facing release notes

calldata.encode now rejects JavaScript inputs that would lose integer precision or Unicode identity before reaching GenVM.

maho0638 commented Oct 2, 2026

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9d753e0a-8492-4416-9c86-54622d9d0ad6
📥 Commits

Reviewing files that changed from the base of the PR and between 4dabdf2 and 2f6545e.

📒 Files selected for processing (2)
  • src/abi/calldata/encoder.ts
  • tests/calldata-lossy-inputs.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Calldata encoding now rejects numbers that are not safe integers and strings with unpaired UTF-16 surrogates. The checks apply to string values and map keys. Tests cover bigint round-tripping and valid surrogate pairs.

Changes

Calldata input validation

Layer / File(s) Summary
String validation
src/abi/calldata/encoder.ts, tests/calldata-lossy-inputs.test.ts
A validating UTF-8 helper rejects unpaired surrogates in string values and map keys. Tests cover malformed strings and preservation of valid surrogate pairs.
Integer validation
src/abi/calldata/encoder.ts, tests/calldata-lossy-inputs.test.ts
Number inputs must be safe integers. The error directs callers to use bigint. Tests cover unsafe numbers and bigint round-tripping.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 2f654

Unsafe numbers and malformed surrogate strings are rejected, while bigint and valid Unicode remain supported. No material merge risk is evident.

Architecture Summary

Architecture risk: 🟡 Medium · up to 2f654

The change affects 2 systems.

Changed systems: src, tests

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (service) was modified; 1 changed file maps to changed impact.
  • observed — tests (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in src/abi/calldata/encoder.ts: Added Unicode validation that rejects unpaired high or low UTF-16 surrogates, and a UTF-8 helper that throws for malformed strings before encoding.
  • observed — Modified behavior in src/abi/calldata/encoder.ts: Map keys now use the validating UTF-8 helper instead of TextEncoder directly, so malformed Unicode keys are rejected.
  • observed — Modified behavior in src/abi/calldata/encoder.ts: Number inputs must now be safe integers; this replaces the prior check that rejected only non-integers, so unsafe integers also throw with guidance to use bigint.
  • observed — Modified behavior in src/abi/calldata/encoder.ts: String values now use the validating UTF-8 helper instead of direct TextEncoder encoding.

Reliability and maintainability

  • inferred — Risk-relevant change factors for src: blast_radius_1; blast_radius_2; direct_dependents_1; direct_dependents_2
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #229 requires rejection of unsafe integer numbers, rejection of unpaired surrogates in string values and map keys, and support for valid surrogate pairs. src/abi/calldata/encoder.ts checks `Nu…
Out of Scope Changes check ✅ Passed The encoder changes implement issue #229. The regression tests verify the requested behavior. No unrelated changes are identified in the reviewed change summary or inspected files.
Title check ✅ Passed The title clearly and concisely describes the main change: rejecting lossy JavaScript inputs during calldata encoding.
Description check ✅ Passed The description covers the required sections, explains what changed and why, reports testing status, records implementation decisions, and includes review tips and release notes. It also identifies is…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

maho0638 commented Oct 4, 2026

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant