Add a new formatDenomWithBadge that returns a ReactNode #252
Conversation
This will add a badge if the denom is verified. Fixes liftedinit#250
WalkthroughThe changes update how token denominations are formatted and displayed. In the bank components, the old Changes
Sequence Diagram(s)sequenceDiagram
participant HB as HistoryBox
participant FN as formatDenomWithBadge (Utils)
participant DD as DenomVerifiedBadge
HB->>FN: Call formatDenomWithBadge(fee.denom, small?)
FN->>FN: Clean and format denom (via formatDenom)
alt Verified Token
FN->>DD: Render verification badge
DD-->>FN: Return badge component
end
FN-->>HB: Return formatted node with badge
HB->>HB: Render fee display with verified badge
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
+ Coverage 55.02% 55.06% +0.04%
==========================================
Files 204 205 +1
Lines 17636 17654 +18
==========================================
+ Hits 9704 9722 +18
Misses 7932 7932 ☔ View full report in Codecov by Sentry. |
![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
🧹 Nitpick comments (2)
components/factory/components/DenomDisplay.tsx (1)
5-12: Consider making the verification check configurable.The verification check is hardcoded to 'umfx'. Consider making this configurable through environment variables or a configuration object for better maintainability and flexibility.
+import env from '@/config/env'; + export const DenomVerifiedBadge = ({ base, ...props }: { base?: string } & { [i: string]: unknown }) => { - const verified = base === 'umfx'; + const verified = base === env.VERIFIED_DENOM; return verified ? <VerifiedIcon {...props} /> : <></>; };components/bank/handlers/createMessageUtils.tsx (1)
2-8: Consider removing unused import.The
formatDenomimport appears to be unused since it's been replaced byformatDenomWithBadge.import { denomToAsset, formatAmount, - formatDenom, formatDenomWithBadge, formatLargeNumber, } from '@/utils';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/bank/components/historyBox.tsx(2 hunks)components/bank/handlers/createMessageUtils.tsx(2 hunks)components/factory/components/DenomDisplay.tsx(2 hunks)utils/format.tsx(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
utils/format.tsx
[warning] 60-60: utils/format.tsx#L60
Added line #L60 was not covered by tests
🔇 Additional comments (5)
components/factory/components/DenomDisplay.tsx (1)
35-38: LGTM!The integration of
DenomVerifiedBadgeintoDenomDisplayis clean and well-structured.components/bank/handlers/createMessageUtils.tsx (1)
21-22: LGTM!The replacement of
formatDenomwithformatDenomWithBadgeis correctly implemented.utils/format.tsx (2)
36-50: LGTM!The new
formatDenomWithBadgefunction is well-documented and properly implemented.
59-61: Add test coverage for IBC denom handling.The IBC denom handling logic is not covered by tests according to static analysis.
Could you add test cases to cover this scenario? Here's a suggested test structure:
describe('formatDenom', () => { it('should handle IBC denoms correctly', () => { const mockAssetInfo = { display: 'ATOM' }; jest.spyOn(utils, 'denomToAsset').mockReturnValue(mockAssetInfo); expect(formatDenom('ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2')) .toBe('ATOM'); }); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-60: utils/format.tsx#L60
Added line #L60 was not covered by testscomponents/bank/components/historyBox.tsx (1)
162-169: LGTM!The fee display logic is well-structured and correctly uses the new
formatDenomWithBadgefunction with the appropriate size prop.
|
It's the same spacing as the others. The fees could look better let me see. |
|
@fmorency Let me know if this looks better: 
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utils/formatNode.tsx (1)
9-23: Consider improving the badge positioning.The function implementation is clean and well-documented. However, using relative positioning with
bottom-1for the badge might be fragile across different contexts.Consider using flexbox alignment instead:
- const classes = `${small ? 'w-3' : 'w-5'} inline relative bottom-1 text-primary`; + const classes = `${small ? 'w-3' : 'w-5'} inline-flex items-center text-primary`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
utils/format.ts(2 hunks)utils/formatNode.tsx(1 hunks)utils/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- utils/index.ts
🔇 Additional comments (2)
utils/formatNode.tsx (1)
1-8: LGTM! Well-documented file purpose and clean imports.The file is well-documented with a clear purpose statement and has clean, organized imports.
utils/format.ts (1)
4-6: LGTM! Clean import organization.The new imports are well-organized and properly scoped.
utils/format.ts
Outdated
| let cleanDenom = denom.replace(/^factory\/[^/]+\//, ''); | ||
|
|
||
| // Skip cleaning for IBC denoms as they should be resolved via assetInfo | ||
| if (cleanDenom.startsWith('ibc/')) { | ||
| return assetInfo?.display.toUpperCase() ?? ''; | ||
| } | ||
|
|
||
| if (cleanDenom?.startsWith('u')) { | ||
| return cleanDenom.slice(1).toUpperCase(); | ||
| cleanDenom = assetInfo?.display.toUpperCase() ?? ''; | ||
| } else if (cleanDenom.startsWith('u')) { | ||
| cleanDenom = cleanDenom.slice(1).toUpperCase(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider handling empty assetInfo.display case.
The code handles IBC denoms by using assetInfo?.display, but if assetInfo exists with an empty display property, it would still fallback to an empty string.
Let's verify if this edge case is handled elsewhere:
Also, consider adding a fallback:
- cleanDenom = assetInfo?.display.toUpperCase() ?? '';
+ cleanDenom = assetInfo?.display?.toUpperCase() ?? cleanDenom;🏁 Script executed:
#!/bin/bash
# Search for assetInfo.display usage to check edge case handling
ast-grep --pattern 'assetInfo?.display'Length of output: 346
IBC Denom Formatting Edge Case – Consider Fallback Improvement
The analysis confirms that in utils/format.ts the IBC denom branch uses the expression
assetInfo?.display.toUpperCase() ?? ''
which will return an empty string if assetInfo exists but its display property is an empty string. The same edge case isn’t specially handled elsewhere in the codebase (as seen in pages/bank.tsx and other files where a similar pattern is used). This inconsistency may lead to an undesired empty result for IBC denoms.
• Update the logic to ensure a fallback to the computed cleanDenom value rather than an empty string. For example, consider changing the line to:
cleanDenom = assetInfo?.display?.toUpperCase() ?? cleanDenom;
This will add a badge if the denom is verified.
Fixes #250
Summary by CodeRabbit
New Features
• Enhanced the display of transaction fees and token messages with a visual badge, making verified denominations clearer to users.
• Introduced a new
DenomVerifiedBadgecomponent for better modularity in displaying verified denominations.Refactor
• Standardized the formatting of denomination details across multiple views for improved consistency and visual appeal.
• Improved the rendering logic for transaction fees to provide additional context.