feat: add currency name to currency code conversion utilities#3223
Open
slurpyone wants to merge 1 commit intoXRPLF:mainfrom
Open
feat: add currency name to currency code conversion utilities#3223slurpyone wants to merge 1 commit intoXRPLF:mainfrom
slurpyone wants to merge 1 commit intoXRPLF:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new currency conversion utility module exporting four functions, re-exports them from the utils barrel, and adds two Node.js test scripts that exercise encoding/decoding, 40-char padding/trimming, and edge cases including XRP handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/xrpl/src/utils/currencyConversion.ts`:
- Around line 91-99: The code is stripping hex nibbles with
currencyCode.replace(/0+$/u, '') which corrupts valid payloads; update the
trimming to remove only padding bytes ('00') from the end of currencyCode (e.g.,
strip repeating "00" byte sequences) before calling convertHexToString, keep the
existing check that throws if the result is empty, and ensure trimmedHex has an
even length (or otherwise error) so convertHexToString receives valid
byte-aligned hex; refer to the variables/function names currencyCode, trimmedHex
and convertHexToString in currencyConversion.ts when making the change.
- Around line 29-35: The short-path for treating 1–3 character inputs as
standard ASCII codes must only be taken after normalizing and verifying ASCII
letters; in currencyNameToCode normalize (trim and toUpperCase) the input first,
check for reserved 'XRP' after normalization, then only return the uppercase
code for length <= 3 if it matches /^[A-Z]{1,3}$/ (or reuse
isStandardCurrencyCode after making it enforce ASCII A–Z), and update the
corresponding short-code checks in the other occurrences (the other branches
using the same fast-path) to use the same normalized + ASCII-gated logic.
In `@test-currency-conversion.js`:
- Around line 4-68: The test reimplements conversion logic (currencyNameToCode,
currencyCodeToName) instead of using the canonical exported helpers and misses
the 20-byte length validation; update the test to import and use the utility
from packages/xrpl/src/utils/currencyConversion.ts (or the package export)
rather than redefining convertStringToHex/convertHexToString, and ensure the
test asserts the same validation rules (including the exact 40-hex / 20-byte
check and error messages for invalid hex/padding cases) so the script exercises
the same codepaths and validations as the real helpers.
- Line 2: The test imports TypeScript source via
require('./packages/xrpl/src/utils/stringConversion') which will fail at
runtime; instead update the import to a runtime-resolvable module or move the
test into the package test harness: either change the require to the built
package path that exists at runtime (e.g.,
require('xrpl/utils/stringConversion')) so convertStringToHex and
convertHexToString resolve, or move this test file into packages/xrpl/test/ and
run it through Jest (or inline the functions like test-currency-simple.js does)
so the convertStringToHex and convertHexToString implementations are available
at test time.
In `@test-currency-simple.js`:
- Around line 1-123: This demo script reimplements currency helpers and only
logs results; replace the local implementations and ad-hoc console checks with
imports of the canonical exported functions (currencyNameToCode and
currencyCodeToName) from the repository's currency conversion module and convert
the script into proper automated tests using the repo test runner (assert exact
return values and error cases rather than printing), removing the duplicated
functions (stringToHex/hexToString/currencyNameToCode/currencyCodeToName) and
any manual console-based "All tests passed" logic so the real helpers are
exercised and failures surface in CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17a3a004-f320-4cd2-86d6-d44098c89a29
📒 Files selected for processing (4)
packages/xrpl/src/utils/currencyConversion.tspackages/xrpl/src/utils/index.tstest-currency-conversion.jstest-currency-simple.js
Comment on lines
+29
to
+35
| if (currencyName === 'XRP') { | ||
| throw new Error('XRP cannot be used as a currency code') | ||
| } | ||
|
|
||
| // For names 3 characters or less, use as-is (standard ASCII codes like USD, EUR) | ||
| if (currencyName.length <= 3) { | ||
| return currencyName.toUpperCase() |
Contributor
There was a problem hiding this comment.
Validate short codes before taking the fast path.
Right now any 1–3 code-unit string is accepted as a “standard” code. That lets currencyNameToCode('xrp') return reserved XRP, and helpers like isStandardCurrencyCode('€') report true even though the API/docs describe these as ASCII codes. Normalize first, then gate the short-code path with a shared ASCII check.
Also applies to: 79-81, 120-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/utils/currencyConversion.ts` around lines 29 - 35, The
short-path for treating 1–3 character inputs as standard ASCII codes must only
be taken after normalizing and verifying ASCII letters; in currencyNameToCode
normalize (trim and toUpperCase) the input first, check for reserved 'XRP' after
normalization, then only return the uppercase code for length <= 3 if it matches
/^[A-Z]{1,3}$/ (or reuse isStandardCurrencyCode after making it enforce ASCII
A–Z), and update the corresponding short-code checks in the other occurrences
(the other branches using the same fast-path) to use the same normalized +
ASCII-gated logic.
Comment on lines
+91
to
+99
| try { | ||
| // Remove trailing zeros and convert from hex | ||
| const trimmedHex = currencyCode.replace(/0+$/u, '') | ||
| if (trimmedHex.length === 0) { | ||
| throw new Error('Invalid currency code: empty after removing padding') | ||
| } | ||
|
|
||
| return convertHexToString(trimmedHex) | ||
| } catch (error) { |
Contributor
There was a problem hiding this comment.
Trim 00 padding bytes, not individual 0 nibbles.
replace(/0+$/u, '') corrupts valid payloads whose UTF-8 hex legitimately ends in 0. For example, a name ending with ASCII 0 produces a final byte of 30; this code strips the trailing 0, leaves odd-length hex, and makes the round-trip fail for a valid currency name.
🐛 Proposed fix
- const trimmedHex = currencyCode.replace(/0+$/u, '')
+ const trimmedHex = currencyCode.replace(/(?:00)+$/u, '')📝 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
| try { | |
| // Remove trailing zeros and convert from hex | |
| const trimmedHex = currencyCode.replace(/0+$/u, '') | |
| if (trimmedHex.length === 0) { | |
| throw new Error('Invalid currency code: empty after removing padding') | |
| } | |
| return convertHexToString(trimmedHex) | |
| } catch (error) { | |
| try { | |
| // Remove trailing zeros and convert from hex | |
| const trimmedHex = currencyCode.replace(/(?:00)+$/u, '') | |
| if (trimmedHex.length === 0) { | |
| throw new Error('Invalid currency code: empty after removing padding') | |
| } | |
| return convertHexToString(trimmedHex) | |
| } catch (error) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/utils/currencyConversion.ts` around lines 91 - 99, The code
is stripping hex nibbles with currencyCode.replace(/0+$/u, '') which corrupts
valid payloads; update the trimming to remove only padding bytes ('00') from the
end of currencyCode (e.g., strip repeating "00" byte sequences) before calling
convertHexToString, keep the existing check that throws if the result is empty,
and ensure trimmedHex has an even length (or otherwise error) so
convertHexToString receives valid byte-aligned hex; refer to the
variables/function names currencyCode, trimmedHex and convertHexToString in
currencyConversion.ts when making the change.
Comment on lines
+4
to
+68
| function currencyNameToCode(currencyName) { | ||
| if (typeof currencyName !== 'string') { | ||
| throw new Error('Currency name must be a string') | ||
| } | ||
|
|
||
| if (currencyName.length === 0) { | ||
| throw new Error('Currency name cannot be empty') | ||
| } | ||
|
|
||
| if (currencyName === 'XRP') { | ||
| throw new Error('XRP cannot be used as a currency code') | ||
| } | ||
|
|
||
| // For names 3 characters or less, use as-is (standard ASCII codes like USD, EUR) | ||
| if (currencyName.length <= 3) { | ||
| return currencyName.toUpperCase() | ||
| } | ||
|
|
||
| // For longer names, convert to 40-character hex string | ||
| const hexString = convertStringToHex(currencyName).toUpperCase() | ||
|
|
||
| // Pad to 40 characters (20 bytes) with zeros | ||
| return hexString.padEnd(40, '0') | ||
| } | ||
|
|
||
| function currencyCodeToName(currencyCode) { | ||
| if (typeof currencyCode !== 'string') { | ||
| throw new Error('Currency code must be a string') | ||
| } | ||
|
|
||
| if (currencyCode.length === 0) { | ||
| throw new Error('Currency code cannot be empty') | ||
| } | ||
|
|
||
| if (currencyCode === 'XRP') { | ||
| return 'XRP' | ||
| } | ||
|
|
||
| // If it's a short code (3 characters or less), return as-is | ||
| if (currencyCode.length <= 3) { | ||
| return currencyCode | ||
| } | ||
|
|
||
| // If it's a 40-character hex string, convert back to string | ||
| if (currencyCode.length === 40) { | ||
| // Check if it's valid hex | ||
| if (!/^[0-9A-Fa-f]+$/u.test(currencyCode)) { | ||
| throw new Error('Invalid currency code: not valid hexadecimal') | ||
| } | ||
|
|
||
| try { | ||
| // Remove trailing zeros and convert from hex | ||
| const trimmedHex = currencyCode.replace(/0+$/u, '') | ||
| if (trimmedHex.length === 0) { | ||
| throw new Error('Invalid currency code: empty after removing padding') | ||
| } | ||
|
|
||
| return convertHexToString(trimmedHex) | ||
| } catch (error) { | ||
| throw new Error(`Invalid currency code: ${error instanceof Error ? error.message : 'conversion failed'}`) | ||
| } | ||
| } | ||
|
|
||
| throw new Error('Invalid currency code: must be 3 characters or less, or exactly 40 characters hex') | ||
| } |
Contributor
There was a problem hiding this comment.
This still isn’t validating the shipped utility.
The file redefines the conversion logic locally, only prints outputs, and already diverges from packages/xrpl/src/utils/currencyConversion.ts by omitting the 20-byte length check. That means regressions in the exported helpers can slip through even when this script reports success.
Also applies to: 70-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-currency-conversion.js` around lines 4 - 68, The test reimplements
conversion logic (currencyNameToCode, currencyCodeToName) instead of using the
canonical exported helpers and misses the 20-byte length validation; update the
test to import and use the utility from
packages/xrpl/src/utils/currencyConversion.ts (or the package export) rather
than redefining convertStringToHex/convertHexToString, and ensure the test
asserts the same validation rules (including the exact 40-hex / 20-byte check
and error messages for invalid hex/padding cases) so the script exercises the
same codepaths and validations as the real helpers.
Comment on lines
+1
to
+123
| // Simple standalone test for currency conversion functions | ||
| function stringToHex(str) { | ||
| return Buffer.from(str, 'utf8').toString('hex') | ||
| } | ||
|
|
||
| function hexToString(hex) { | ||
| return Buffer.from(hex, 'hex').toString('utf8') | ||
| } | ||
|
|
||
| function currencyNameToCode(currencyName) { | ||
| if (typeof currencyName !== 'string') { | ||
| throw new Error('Currency name must be a string') | ||
| } | ||
|
|
||
| if (currencyName.length === 0) { | ||
| throw new Error('Currency name cannot be empty') | ||
| } | ||
|
|
||
| if (currencyName === 'XRP') { | ||
| throw new Error('XRP cannot be used as a currency code') | ||
| } | ||
|
|
||
| // For names 3 characters or less, use as-is (standard ASCII codes like USD, EUR) | ||
| if (currencyName.length <= 3) { | ||
| return currencyName.toUpperCase() | ||
| } | ||
|
|
||
| // For longer names, convert to hex string | ||
| const hexString = stringToHex(currencyName).toUpperCase() | ||
|
|
||
| // Check if the hex string is too long (more than 40 characters = 20 bytes) | ||
| if (hexString.length > 40) { | ||
| throw new Error(`Currency name "${currencyName}" is too long. Maximum length is 20 bytes when UTF-8 encoded.`) | ||
| } | ||
|
|
||
| // Pad to exactly 40 characters (20 bytes) with zeros | ||
| return hexString.padEnd(40, '0') | ||
| } | ||
|
|
||
| function currencyCodeToName(currencyCode) { | ||
| if (typeof currencyCode !== 'string') { | ||
| throw new Error('Currency code must be a string') | ||
| } | ||
|
|
||
| if (currencyCode.length === 0) { | ||
| throw new Error('Currency code cannot be empty') | ||
| } | ||
|
|
||
| if (currencyCode === 'XRP') { | ||
| return 'XRP' | ||
| } | ||
|
|
||
| // If it's a short code (3 characters or less), return as-is | ||
| if (currencyCode.length <= 3) { | ||
| return currencyCode | ||
| } | ||
|
|
||
| // If it's a 40-character hex string, convert back to string | ||
| if (currencyCode.length === 40) { | ||
| // Check if it's valid hex | ||
| if (!/^[0-9A-Fa-f]+$/u.test(currencyCode)) { | ||
| throw new Error('Invalid currency code: not valid hexadecimal') | ||
| } | ||
|
|
||
| try { | ||
| // Remove trailing zeros and convert from hex | ||
| const trimmedHex = currencyCode.replace(/0+$/u, '') | ||
| if (trimmedHex.length === 0) { | ||
| throw new Error('Invalid currency code: empty after removing padding') | ||
| } | ||
|
|
||
| return hexToString(trimmedHex) | ||
| } catch (error) { | ||
| throw new Error(`Invalid currency code: ${error instanceof Error ? error.message : 'conversion failed'}`) | ||
| } | ||
| } | ||
|
|
||
| throw new Error('Invalid currency code: must be 3 characters or less, or exactly 40 characters hex') | ||
| } | ||
|
|
||
| // Test cases | ||
| console.log('Testing currency conversion functions...\n'); | ||
|
|
||
| try { | ||
| // Test standard 3-char codes | ||
| console.log('USD ->', currencyNameToCode('USD')); | ||
| console.log('EUR ->', currencyNameToCode('EUR')); | ||
| console.log('BTC ->', currencyNameToCode('BTC')); | ||
|
|
||
| // Test longer names | ||
| console.log('MyCustomToken ->', currencyNameToCode('MyCustomToken')); | ||
| console.log('GOLD ->', currencyNameToCode('GOLD')); | ||
| console.log('MediumLengthToken ->', currencyNameToCode('MediumLengthToken')); | ||
|
|
||
| // Test reverse conversion | ||
| const hexCode1 = currencyNameToCode('MyCustomToken'); | ||
| console.log('\nReverse conversions:'); | ||
| console.log(hexCode1, '->', currencyCodeToName(hexCode1)); | ||
|
|
||
| const hexCode2 = currencyNameToCode('MediumLengthToken'); | ||
| console.log(hexCode2, '->', currencyCodeToName(hexCode2)); | ||
|
|
||
| // Test standard codes reverse | ||
| console.log('USD reverse:', currencyCodeToName('USD')); | ||
| console.log('EUR reverse:', currencyCodeToName('EUR')); | ||
|
|
||
| // Test edge cases | ||
| console.log('\nEdge cases:'); | ||
| console.log('XRP reverse:', currencyCodeToName('XRP')); | ||
|
|
||
| // Test error case | ||
| console.log('\nTesting error case:'); | ||
| try { | ||
| currencyNameToCode('SomeVeryLongTokenNameThatExceedsTwentyBytes'); | ||
| } catch (error) { | ||
| console.log('Expected error for long name:', error.message); | ||
| } | ||
|
|
||
| console.log('\n✅ All tests passed!'); | ||
|
|
||
| } catch (error) { | ||
| console.error('❌ Test failed:', error.message); | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
This is a demo script, not an automated test.
The file reimplements the conversion helpers locally and only logs outputs, so regressions in packages/xrpl/src/utils/currencyConversion.ts can still finish with “All tests passed”. Please import the exported helpers and assert exact results in the repo test runner instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-currency-simple.js` around lines 1 - 123, This demo script reimplements
currency helpers and only logs results; replace the local implementations and
ad-hoc console checks with imports of the canonical exported functions
(currencyNameToCode and currencyCodeToName) from the repository's currency
conversion module and convert the script into proper automated tests using the
repo test runner (assert exact return values and error cases rather than
printing), removing the duplicated functions
(stringToHex/hexToString/currencyNameToCode/currencyCodeToName) and any manual
console-based "All tests passed" logic so the real helpers are exercised and
failures surface in CI.
slurpyone
force-pushed
the
issue-2185-currency-conversion
branch
from
1ce2ae7 to
34623fa
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test-currency-simple.js (1)
2-123:⚠️ Potential issue | 🟠 MajorUse canonical exports with real assertions instead of self-testing duplicated logic.
Line 2 through Line 123 currently re-implements the conversion helpers locally and validates behavior via
console.log, which means regressions inpackages/xrpl/src/utils/currencyConversion.tscan still slip through CI. Please convert this into an actual repo test that imports the exported utilities and asserts exact outputs/errors.Suggested direction
- function stringToHex(str) { ... } - function hexToString(hex) { ... } - function currencyNameToCode(currencyName) { ... } - function currencyCodeToName(currencyCode) { ... } - // console.log-based test harness + import { strict as assert } from 'assert' + import { currencyNameToCode, currencyCodeToName } from '<canonical utils export path>' + + assert.equal(currencyNameToCode('USD'), 'USD') + assert.equal(currencyCodeToName('USD'), 'USD') + assert.throws( + () => currencyNameToCode('SomeVeryLongTokenNameThatExceedsTwentyBytes'), + /too long/i + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-currency-simple.js` around lines 2 - 123, Replace the self-contained helpers and console-based checks with a real unit test that imports the canonical exports currencyNameToCode and currencyCodeToName from packages/xrpl/src/utils/currencyConversion.ts and uses your test framework assertions (e.g., assert.strictEqual / expect) to assert exact outputs and thrown errors; specifically test: 3-char codes (USD, EUR, BTC) return same uppercase code, long names produce 40-char padded hex (verify hex format and round-trip with currencyCodeToName), 'XRP' behavior, and that overly long UTF-8 names throw the expected error; remove the local stringToHex/hexToString implementations and all console.log usage so the test exercises the real exported functions and their error messages exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test-currency-simple.js`:
- Around line 2-123: Replace the self-contained helpers and console-based checks
with a real unit test that imports the canonical exports currencyNameToCode and
currencyCodeToName from packages/xrpl/src/utils/currencyConversion.ts and uses
your test framework assertions (e.g., assert.strictEqual / expect) to assert
exact outputs and thrown errors; specifically test: 3-char codes (USD, EUR, BTC)
return same uppercase code, long names produce 40-char padded hex (verify hex
format and round-trip with currencyCodeToName), 'XRP' behavior, and that overly
long UTF-8 names throw the expected error; remove the local
stringToHex/hexToString implementations and all console.log usage so the test
exercises the real exported functions and their error messages exactly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9f55989-272a-4986-940a-5ca7fdb6b44e
📒 Files selected for processing (4)
packages/xrpl/src/utils/currencyConversion.tspackages/xrpl/src/utils/index.tstest-currency-conversion.jstest-currency-simple.js
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/xrpl/src/utils/currencyConversion.ts
- packages/xrpl/src/utils/index.ts
- test-currency-conversion.js
slurpyone
force-pushed
the
issue-2185-currency-conversion
branch
from
34623fa to
2f5d10b
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test-currency-simple.js (1)
1-123:⚠️ Potential issue | 🟠 MajorUse canonical exports + real assertions instead of reimplementing and logging.
Line 1-Line 79 duplicates conversion logic, and Line 81-Line 123 only logs output. This can report “✅ All tests passed!” even if
packages/xrpl/src/utils/currencyConversion.tsregresses. Please convert this into repo-runner assertions that import the exported helpers directly (currencyNameToCode,currencyCodeToName, and related validators).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-currency-simple.js` around lines 1 - 123, Replace the local duplicate implementations of stringToHex, hexToString, currencyNameToCode and currencyCodeToName with imports of the canonical exports (currencyNameToCode, currencyCodeToName and any validators) from the package module, and convert console-logging tests into repo-runner style assertions (use the repo's assertion helpers or plain assert) to validate expected outputs and error cases (e.g., assert(currencyNameToCode('USD') === 'USD'), assert.throws(() => currencyNameToCode(longName), /too long/), and assert(currencyCodeToName(hexCode) === 'MyCustomToken')); ensure tests reference the exported symbols (currencyNameToCode, currencyCodeToName) only and remove the duplicated local functions and loose console output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test-currency-simple.js`:
- Around line 1-123: Replace the local duplicate implementations of stringToHex,
hexToString, currencyNameToCode and currencyCodeToName with imports of the
canonical exports (currencyNameToCode, currencyCodeToName and any validators)
from the package module, and convert console-logging tests into repo-runner
style assertions (use the repo's assertion helpers or plain assert) to validate
expected outputs and error cases (e.g., assert(currencyNameToCode('USD') ===
'USD'), assert.throws(() => currencyNameToCode(longName), /too long/), and
assert(currencyCodeToName(hexCode) === 'MyCustomToken')); ensure tests reference
the exported symbols (currencyNameToCode, currencyCodeToName) only and remove
the duplicated local functions and loose console output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99e32753-0652-43a7-80b4-30c37cca6b3e
📒 Files selected for processing (4)
packages/xrpl/src/utils/currencyConversion.tspackages/xrpl/src/utils/index.tstest-currency-conversion.jstest-currency-simple.js
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/xrpl/src/utils/index.ts
- packages/xrpl/src/utils/currencyConversion.ts
- test-currency-conversion.js
slurpyone
force-pushed
the
issue-2185-currency-conversion
branch
from
2f5d10b to
b4e4e3f
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test-currency-conversion.js (2)
4-68:⚠️ Potential issue | 🟠 MajorTest logic is duplicated and already diverges from production behavior.
This script re-implements conversion instead of calling
packages/xrpl/src/utils/currencyConversion.ts, and it misses the 20-byte UTF-8 max-length validation. It can mask regressions in the real exported helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-currency-conversion.js` around lines 4 - 68, This file re-implements currency conversion logic (currencyNameToCode and currencyCodeToName) and diverges from the production helpers; replace the local implementations by importing and calling the canonical helpers from packages/xrpl/src/utils/currencyConversion.ts instead of re-implementing convertStringToHex/convertHexToString here, and ensure the production validation for UTF-8 max 20-byte names (reject names whose UTF-8 encoding > 20 bytes) is preserved; remove duplicated conversion code and any differing error messages so tests exercise the real exported helpers directly.
2-2:⚠️ Potential issue | 🟠 MajorRuntime module resolution is likely broken at Line 2.
require('./packages/xrpl/src/utils/stringConversion')points to TS source from a plain JS script; this typically fails under raw Node before tests execute.#!/bin/bash # Verify whether this script is executed via raw Node or a TS-aware test harness, # and whether a runtime-resolvable JS target exists. set -euo pipefail rg -n "test-currency-conversion\\.js|node test-currency-conversion\\.js" package.json packages/xrpl/package.json || true rg -n "require\\('./packages/xrpl/src/utils/stringConversion'\\)" test-currency-conversion.js fd '^stringConversion\.(ts|js)$' packages/xrpl/src/utils packages/xrpl/dist/npm/utils 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-currency-conversion.js` at line 2, The test imports TS source directly (require('./packages/xrpl/src/utils/stringConversion')) which will fail under raw Node; update test-currency-conversion.js to load the runtime JS build instead (use the built module under packages/xrpl/dist/npm/utils/stringConversion) or adjust the test runner to use a TS-aware loader (e.g., ts-node/register) so convertStringToHex and convertHexToString resolve at runtime; locate the require of stringConversion in test-currency-conversion.js and replace it with a runtime-resolvable module path or enable TS transpilation in the test harness.
🧹 Nitpick comments (1)
test-currency-conversion.js (1)
70-92: Use assertions instead of console logs for deterministic test outcomes.Current checks are output-based and can pass without validating exact behavior. Please switch to assertion-based tests (or move this into the project test harness).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-currency-conversion.js` around lines 70 - 92, Replace the console.log-based checks in the test block with deterministic assertions: import or require Node's assert and use assert.strictEqual/assert.ok to verify currencyNameToCode and currencyCodeToName return the exact expected values (e.g., assert.strictEqual(currencyNameToCode('USD'), 'USD') and assert.strictEqual(currencyCodeToName(currencyNameToCode('MyCustomToken')), 'MyCustomToken')); remove noisy console output, keep the try/catch only to rethrow or fail the test harness, and ensure tests call currencyNameToCode and currencyCodeToName (the functions referenced) with explicit expected values so failures surface as assertion errors instead of passing via logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test-currency-conversion.js`:
- Around line 4-68: This file re-implements currency conversion logic
(currencyNameToCode and currencyCodeToName) and diverges from the production
helpers; replace the local implementations by importing and calling the
canonical helpers from packages/xrpl/src/utils/currencyConversion.ts instead of
re-implementing convertStringToHex/convertHexToString here, and ensure the
production validation for UTF-8 max 20-byte names (reject names whose UTF-8
encoding > 20 bytes) is preserved; remove duplicated conversion code and any
differing error messages so tests exercise the real exported helpers directly.
- Line 2: The test imports TS source directly
(require('./packages/xrpl/src/utils/stringConversion')) which will fail under
raw Node; update test-currency-conversion.js to load the runtime JS build
instead (use the built module under
packages/xrpl/dist/npm/utils/stringConversion) or adjust the test runner to use
a TS-aware loader (e.g., ts-node/register) so convertStringToHex and
convertHexToString resolve at runtime; locate the require of stringConversion in
test-currency-conversion.js and replace it with a runtime-resolvable module path
or enable TS transpilation in the test harness.
---
Nitpick comments:
In `@test-currency-conversion.js`:
- Around line 70-92: Replace the console.log-based checks in the test block with
deterministic assertions: import or require Node's assert and use
assert.strictEqual/assert.ok to verify currencyNameToCode and currencyCodeToName
return the exact expected values (e.g.,
assert.strictEqual(currencyNameToCode('USD'), 'USD') and
assert.strictEqual(currencyCodeToName(currencyNameToCode('MyCustomToken')),
'MyCustomToken')); remove noisy console output, keep the try/catch only to
rethrow or fail the test harness, and ensure tests call currencyNameToCode and
currencyCodeToName (the functions referenced) with explicit expected values so
failures surface as assertion errors instead of passing via logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56da08e9-568d-4d74-bba7-7bdeac3a9481
📒 Files selected for processing (4)
packages/xrpl/src/utils/currencyConversion.tspackages/xrpl/src/utils/index.tstest-currency-conversion.jstest-currency-simple.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test-currency-simple.js
- packages/xrpl/src/utils/currencyConversion.ts
slurpyone
force-pushed
the
issue-2185-currency-conversion
branch
from
b4e4e3f to
cc12a99
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test-currency-conversion.js (1)
70-92: Tests lack assertions—script always reports "passed" regardless of correctness.The test section only logs outputs without verifying them. If conversions produce incorrect results, the script still prints "All tests passed!" because no actual assertions fail.
♻️ Proposed fix with assertions
// Test cases console.log('Testing currency conversion functions...\n'); try { // Test standard 3-char codes - console.log('USD ->', currencyNameToCode('USD')); - console.log('EUR ->', currencyNameToCode('EUR')); + const usdCode = currencyNameToCode('USD'); + if (usdCode !== 'USD') throw new Error(`Expected 'USD', got '${usdCode}'`); + console.log('USD ->', usdCode); + + const eurCode = currencyNameToCode('EUR'); + if (eurCode !== 'EUR') throw new Error(`Expected 'EUR', got '${eurCode}'`); + console.log('EUR ->', eurCode); // Test longer names - console.log('MyCustomToken ->', currencyNameToCode('MyCustomToken')); + const customCode = currencyNameToCode('MyCustomToken'); + if (customCode.length !== 40) throw new Error(`Expected 40-char hex, got length ${customCode.length}`); + console.log('MyCustomToken ->', customCode); // Test reverse conversion - const hexCode = currencyNameToCode('MyCustomToken'); - console.log('Reverse conversion:', hexCode, '->', currencyCodeToName(hexCode)); + const reversed = currencyCodeToName(customCode); + if (reversed !== 'MyCustomToken') throw new Error(`Round-trip failed: expected 'MyCustomToken', got '${reversed}'`); + console.log('Reverse conversion:', customCode, '->', reversed); // Test standard codes reverse - console.log('USD reverse:', currencyCodeToName('USD')); + const usdReverse = currencyCodeToName('USD'); + if (usdReverse !== 'USD') throw new Error(`Expected 'USD', got '${usdReverse}'`); + console.log('USD reverse:', usdReverse); console.log('\nAll tests passed!');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-currency-conversion.js` around lines 70 - 92, Add real assertions to the test block so failures stop the script instead of always printing "All tests passed!". Replace the current console-only checks by verifying expected values from currencyNameToCode and currencyCodeToName: assert that currencyNameToCode('USD') === expectedCodeForUSD and currencyNameToCode('EUR') === expectedCodeForEUR, assert that currencyNameToCode('MyCustomToken') returns the expected hex and that currencyCodeToName(currencyNameToCode('MyCustomToken')) equals 'MyCustomToken', and assert that currencyCodeToName('USD') yields the expected name; throw an Error or exit nonzero when any assertion fails so the catch block reports real failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test-currency-conversion.js`:
- Around line 70-92: Add real assertions to the test block so failures stop the
script instead of always printing "All tests passed!". Replace the current
console-only checks by verifying expected values from currencyNameToCode and
currencyCodeToName: assert that currencyNameToCode('USD') === expectedCodeForUSD
and currencyNameToCode('EUR') === expectedCodeForEUR, assert that
currencyNameToCode('MyCustomToken') returns the expected hex and that
currencyCodeToName(currencyNameToCode('MyCustomToken')) equals 'MyCustomToken',
and assert that currencyCodeToName('USD') yields the expected name; throw an
Error or exit nonzero when any assertion fails so the catch block reports real
failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7613b3ef-489c-4a9e-befa-4857c0b521c3
📒 Files selected for processing (4)
packages/xrpl/src/utils/currencyConversion.tspackages/xrpl/src/utils/index.tstest-currency-conversion.jstest-currency-simple.js
✅ Files skipped from review due to trivial changes (1)
- packages/xrpl/src/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test-currency-simple.js
slurpyone
force-pushed
the
issue-2185-currency-conversion
branch
from
cc12a99 to
3af772b
Compare
- Add currencyNameToCode() to convert human-readable names to XRPL currency codes - Add currencyCodeToName() to convert XRPL currency codes back to readable names - Add isStandardCurrencyCode() and isHexCurrencyCode() helper functions - Support both 3-character ASCII codes (USD, EUR) and hex-encoded longer names - Include proper validation and error handling for oversized names - Addresses issue XRPLF#2185
slurpyone
force-pushed
the
issue-2185-currency-conversion
branch
from
3af772b to
8fcaba7
Compare
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
1 participant
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.
Description
This PR adds utility functions to convert between human-readable currency names and XRPL currency codes, addressing a common need for developers working with custom currencies.
Changes Made
currencyNameToCode()function to convert human-readable names to XRPL currency codescurrencyCodeToName()function to convert XRPL currency codes back to readable namesisStandardCurrencyCode()andisHexCurrencyCode()helper functionsUsage Examples
Related Issues
Fixes #2185
Testing
Breaking Changes
None - this adds new utility functions without modifying existing functionality.