Skip to content

fix: uses Decimal to convert denom to udenom value#2318

Merged
stalniy merged 1 commit intomainfrom
fix/udenom-conversion
Dec 4, 2025
Merged

fix: uses Decimal to convert denom to udenom value#2318
stalniy merged 1 commit intomainfrom
fix/udenom-conversion

Conversation

@stalniy
Copy link
Contributor

@stalniy stalniy commented Dec 4, 2025

Why

Because in JS

> 32.73969 * 1000_000
32739690.000000004

Fixes #2229

Summary by CodeRabbit

  • Bug Fixes

    • Improved precision for amount conversions by eliminating reliance on floating-point arithmetic and better handling of string-formatted amounts.
  • Chores

    • Added a math library dependency to support more accurate numeric operations.
  • Documentation

    • Marked floating-point amount usage as deprecated; prefer string-based amount representations for calculations.

✏️ Tip: You can customize this high-level summary in your review settings.

@stalniy stalniy requested a review from a team as a code owner December 4, 2025 08:58
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 4, 2025

Walkthrough

Refactored denomination conversion to use CosmJS Decimal for atomic conversions and updated callers; added the @cosmjs/math dependency. Signature for denomToUdenom now accepts string or number overloads and converts via Decimal.fromUserInput(...).atomics.

Changes

Cohort / File(s) Summary
Dependency
apps/deploy-web/package.json
Added dependency @cosmjs/math version ~0.36.1.
Math utilities
apps/deploy-web/src/utils/mathHelpers.ts
Replaced two-parameter denomToUdenom(amount: number, decimals: number) with overloads denomToUdenom(amount: number): number and denomToUdenom(amount: string): number; implementation uses Decimal.fromUserInput(...).atomics and returns a Number. Added deprecation note discouraging JS floating-point use for denom amounts.
Price utilities (call site)
apps/deploy-web/src/utils/priceUtils.ts
Updated coinToUDenom to pass coin.amount (string) directly into denomToUdenom instead of parsing to number.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review denomToUdenom overloads and implementation for edge cases (empty/invalid strings, negative values, rounding).
  • Verify all other callers (outside changed files) still pass string-compatible amounts.
  • Confirm package.json dependency addition and lockfile alignment in the broader repo/build.

Poem

🐇 I hopped through decimals, neat and spry,
From strings to atomics, numbers fly,
No float shall wobble where precision grows,
I tally coins where tidy math flows. ✨

Pre-merge checks and finishing touches

❌ 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%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title accurately describes the main change—replacing numeric conversion with Decimal-based arithmetic for denom to udenom conversion to fix precision issues.
Linked Issues check ✅ Passed Changes implement Decimal-based conversion logic to address numeric precision issues causing transaction encoding failures reported in issue #2229.
Out of Scope Changes check ✅ Passed All changes directly support the objective of fixing denom-to-udenom conversion precision; no unrelated modifications detected.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/udenom-conversion

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

@codecov
Copy link

codecov bot commented Dec 4, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1717 1 1716 0
View the top 1 failed test(s) by shortest run time
Addresses API GET /v1/addresses/{address} returns address information
Stack Traces | 0.152s run time
Error: 
    at Query.run (.../dialects/postgres/query.js:76:25)
    at .../sequelize/src/sequelize.js:650:28
    at processTicksAndRejections (node:internal/process/task_queues:103:5)
    at PostgresQueryInterface.insert (.../dialects/abstract/query-interface.js:795:21)
    at Lease.save (.../sequelize/src/model.js:4154:35)
    at lease.create (.../sequelize/src/model.js:2305:12)
    at createLease (.../test/seeders/lease.seeder.ts:6:10)
    at async Promise.all (index 1)
    at setup (.../test/functional/addresses.spec.ts:357:5)
    at Object.<anonymous> (.../test/functional/addresses.spec.ts:48:53)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@stalniy stalniy force-pushed the fix/udenom-conversion branch from b037883 to a136641 Compare December 4, 2025 09:01
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 68f086b and a136641.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (3)
  • apps/deploy-web/package.json (1 hunks)
  • apps/deploy-web/src/utils/mathHelpers.ts (2 hunks)
  • apps/deploy-web/src/utils/priceUtils.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (.cursor/rules/general.mdc)

**/*.{ts,tsx,js}: Never use type any or cast to type any. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.

Files:

  • apps/deploy-web/src/utils/priceUtils.ts
  • apps/deploy-web/src/utils/mathHelpers.ts
🧬 Code graph analysis (1)
apps/deploy-web/src/utils/priceUtils.ts (1)
apps/deploy-web/src/utils/mathHelpers.ts (1)
  • denomToUdenom (34-36)
⏰ 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). (9)
  • GitHub Check: validate / validate-app
  • GitHub Check: test-build
  • GitHub Check: validate / validate-app
  • GitHub Check: test-build
  • GitHub Check: test-build
  • GitHub Check: test-build
  • GitHub Check: validate / validate-app
  • GitHub Check: test-build
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/deploy-web/package.json (1)

37-40: Add @cosmjs/math aligned with other CosmJS packages

The new @cosmjs/math dependency at ~0.36.1 is consistent with the other CosmJS packages already pinned to ~0.36.1, so the addition looks correct and cohesive.

Please confirm npm install/npm test on this app to ensure there are no peer/version mismatches involving @cosmjs/math.

apps/deploy-web/src/utils/priceUtils.ts (1)

19-27: Use of string amount in coinToUDenom correctly leverages Decimal path

Passing coin.amount directly as a string into denomToUdenom avoids an intermediate JS floating‑point parse and lets the Decimal-based conversion handle precision, which is exactly what we want here.

apps/deploy-web/src/utils/mathHelpers.ts (1)

1-1: Decimal import matches new precise conversion logic

Importing Decimal from @cosmjs/math is appropriate here and matches the new denomToUdenom implementation and the added runtime dependency in package.json.

Please ensure this file is compiled with the same @cosmjs/math version (~0.36.1) declared in apps/deploy-web/package.json to avoid type/runtime drift.

@stalniy stalniy merged commit d41dbbe into main Dec 4, 2025
65 of 67 checks passed
@stalniy stalniy deleted the fix/udenom-conversion branch December 4, 2025 11:05
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.

POST http://console-api-mainnet-service.prod:3000/v1/tx fails properly encoding Decimal number

2 participants

Comments