fix(Coin): align fromString denom regex with Cosmos denom spec#168
Conversation
Coin.fromString used the denom character class [0-9a-zA-Z/], which is
both narrower and looser than the Cosmos SDK denom spec
([a-zA-Z][a-zA-Z0-9/:._-]{2,127}):
- denoms containing ':' '.' '_' '-' (e.g. tokenfactory subdenoms)
failed to parse, so fromString(coin.toString()) did not round-trip;
- numeric-only strings (e.g. '5000') matched and were mis-parsed into a
garbage Coin('0', 500) instead of throwing.
Require the denom to start with a letter and allow the full Cosmos denom
charset. Existing denoms (uinit, ibc/<hash>, decimal amounts) are
unaffected. Adds round-trip and numeric-only tests.
|
Warning Review limit reached
More reviews will be available in 54 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. WalkthroughThis PR fixes ChangesCoin denomination parsing
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/Coin.spec.ts (1)
151-158: ⚡ Quick winMake the test match its stated character coverage.
This case only exercises
-, but the title says"-", ".", "_". Please either rename the test or add explicit.and_denom cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/Coin.spec.ts` around lines 151 - 158, The test titled to cover "-", ".", and "_" only verifies "-"; update the test in Coin.spec.ts so it actually exercises denoms with ".", and "_" as well (or rename the test to only mention "-"). Specifically, add two additional cases using new Coin('factory/init1abc/my.token', 1001) / Coin.fromString('1001factory/init1abc/my.token') and new Coin('factory/init1abc/my_token', 1001) / Coin.fromString('1001factory/init1abc/my_token'), and assert equality and round-trip with Coin.fromString(coin.toString()) for each, referencing the Coin constructor, Coin.fromString and Coin.toString methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/Coin.ts`:
- Line 72: The denom regex in the match on the line with "const m =
str.match(...)" currently allows any length (using *) and permits 1–2 character
denoms; update that capture group (the one starting with the leading alphabetic
character) so the part after the initial [a-zA-Z] uses a bounded quantifier of 2
to 127 characters instead of *, ensuring the denom follows Cosmos SDK rules of
an initial letter and total length 3–128; keep the surrounding numeric amount
group and anchors unchanged.
---
Nitpick comments:
In `@src/core/Coin.spec.ts`:
- Around line 151-158: The test titled to cover "-", ".", and "_" only verifies
"-"; update the test in Coin.spec.ts so it actually exercises denoms with ".",
and "_" as well (or rename the test to only mention "-"). Specifically, add two
additional cases using new Coin('factory/init1abc/my.token', 1001) /
Coin.fromString('1001factory/init1abc/my.token') and new
Coin('factory/init1abc/my_token', 1001) /
Coin.fromString('1001factory/init1abc/my_token'), and assert equality and
round-trip with Coin.fromString(coin.toString()) for each, referencing the Coin
constructor, Coin.fromString and Coin.toString methods.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e57b1517-67a6-48d0-8e62-38e13a402335
📒 Files selected for processing (2)
src/core/Coin.spec.tssrc/core/Coin.ts
… _ in round-trip test
Address review feedback on the denom regex:
- bound the denom to the Cosmos default length range ([a-zA-Z][...]{2,127}),
matching the SDK's reDnmString (total length 3-128) instead of an unbounded
quantifier;
- extend the round-trip test denom to include '.', '_' and '-' so it matches
the cases named in the test title.
|
Updated to address the review feedback:
Existing cases ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/Coin.ts`:
- Line 72: The long regex in the const m = str.match(...) expression exceeds
line length; refactor by extracting the pattern into multiple concatenated
string parts (or named constants) and build a RegExp before matching (e.g.,
create a const patternParts = ['^', '(-?[0-9]+(\\.[0-9]+)?)',
'([a-zA-Z][0-9a-zA-Z/:._-]{2,127})', '$'].join('') and const re = new
RegExp(patternParts) ) and then replace const m = str.match(...) with const m =
str.match(re) so the regex is split across lines to satisfy Prettier while
preserving the same pattern.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32cc2d75-2651-42f4-8b05-332bbf2c9c93
📒 Files selected for processing (2)
src/core/Coin.spec.tssrc/core/Coin.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/Coin.spec.ts
2 participants
Fixes #167.
Coin.fromStringused the denom character class[0-9a-zA-Z/], which is both narrower and looser than the Cosmos SDK denom spec ([a-zA-Z][a-zA-Z0-9/:._-]{2,127})::._-(e.g. tokenfactory subdenoms likefactory/<addr>/my-token) failed to parse, soCoin.fromString(coin.toString())threw instead of round-tripping.Coin.fromString('5000')returnedCoin('0', 500)instead of throwing the documented error.Change
The denom group now requires a leading letter and allows the full Cosmos denom charset. Existing denoms (
uinit,ibc/<hash>, decimal amounts) parse identically — verified against the currentfromStringtests.Tests
-(factory/init1abc/my-token), includingfromString(coin.toString());'5000','12') now throw.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests