Add a verified icon next to the denomination name for MFX #242
Conversation
WalkthroughThis pull request introduces the Changes
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 (
|
|
Created #243 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #242 +/- ##
==========================================
- Coverage 55.08% 54.97% -0.12%
==========================================
Files 200 203 +3
Lines 16873 16916 +43
==========================================
+ Hits 9294 9299 +5
- Misses 7579 7617 +38 ☔ View full report in Codecov by Sentry. |
|
I could fix #243 as part of this PR as well if you're okay with delaying it. |
![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 (2)
components/icons/VerifiedIcon.tsx (1)
4-32: Add accessibility attributes to the SVG icon.The SVG icon should include accessibility attributes to improve screen reader support.
Apply this diff:
export const VerifiedIcon: React.FC<SVGProps<SVGSVGElement>> = props => ( <svg width="1em" height="1em" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" data-reactroot="" + role="img" + aria-label="Verified" {...props} >components/bank/components/tokenList.tsx (1)
110-118: Consider extracting token display logic.Since there's a planned refactor to create a
DenomNamecomponent, consider extracting this token display logic into a separate component now to make the future refactor easier.Example implementation:
interface TokenDisplayProps { metadata: any; truncateLength?: number; } const TokenDisplay: React.FC<TokenDisplayProps> = ({ metadata, truncateLength = 12 }) => ( <> {truncateString(metadata?.display ?? '', truncateLength).toUpperCase()} {metadata?.base?.includes('umfx') && ( <VerifiedIcon style={{ margin: '0 4px' }} className="inline-block text-primary" /> )} </> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/bank/components/tokenList.tsx(2 hunks)components/bank/forms/sendForm.tsx(3 hunks)components/icons/VerifiedIcon.tsx(1 hunks)components/icons/index.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
components/bank/components/tokenList.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#161
File: components/factory/components/DenomList.tsx:408-417
Timestamp: 2025-01-07T01:23:26.489Z
Learning: In the manifest-app project, MintModal and BurnModal are intentionally kept separate with their own forms, transactions, and queries, prioritizing code clarity and maintainability over abstraction.
🔇 Additional comments (2)
components/icons/index.ts (1)
28-28: LGTM!The export statement follows the established pattern in the file.
components/bank/components/tokenList.tsx (1)
112-117: LGTM!The implementation correctly adds the verified icon for MFX tokens with appropriate styling.
|
Here's the latest screenshot: 
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
components/factory/components/DenomList.tsx (1)
511-511:🧩 Analysis chain
Verify the MFX token identification logic.
The code uses
denom.base.includes('umfx')to identify MFX tokens. This string matching approach might be fragile if there are other tokens with similar names.Consider using one of these alternatives:
- Use an exact match:
-disabled={denom.base.includes('umfx')} +disabled={denom.base === 'umfx'}
- Use a constant:
const MFX_DENOM = 'umfx'; // Then use: disabled={denom.base === MFX_DENOM}Let's verify the current usage:
Also applies to: 519-519, 527-527
🏁 Script executed:
#!/bin/bash # Search for other instances of 'umfx' string matching rg -i "umfx" --type ts --type tsxLength of output: 61
I'll re-run the search with an updated command that explicitly looks for
*.tsand*.tsxfiles.
🏁 Script executed:
#!/bin/bash # Re-run search for 'umfx' in both .ts and .tsx files using glob patterns rg -i "umfx" -g "*.ts" -g "*.tsx"Length of output: 3115
Action: Update the MFX token matching logic for consistency and precision
The search results reveal that while some parts of the code use strict equality (e.g., in DenomDisplay.tsx and send forms), others (such as DenomList.tsx and DenomImage.tsx) use the substring check with
.includes('umfx'). This discrepancy can lead to potential false positives if a denom contains extra characters. It is advisable to refactor the code to use a constant and a strict equality check instead.
- Update DenomList.tsx (lines 511, 519, and 527) to use an equality check.
- Consider defining a constant (e.g.
const MFX_DENOM = 'umfx') in a shared utility or config file to ensure consistency.[iam_verified_issues_found]
🧹 Nitpick comments (1)
components/factory/components/DenomDisplay.tsx (1)
16-16: Consider making the verified token check configurable.The hard-coded check for
'umfx'reduces reusability. Consider making this configurable through props or environment variables.- const verified = metadata?.base === 'umfx'; + const verifiedTokens = ['umfx']; // Could be moved to config/env + const verified = metadata?.base && verifiedTokens.includes(metadata.base);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/bank/components/tokenList.tsx(2 hunks)components/bank/forms/ibcSendForm.tsx(3 hunks)components/bank/forms/sendForm.tsx(3 hunks)components/factory/components/DenomDisplay.tsx(1 hunks)components/factory/components/DenomList.tsx(2 hunks)components/factory/components/index.ts(1 hunks)components/icons/VerifiedIcon.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/bank/components/tokenList.tsx
- components/bank/forms/sendForm.tsx
🧰 Additional context used
🪛 GitHub Check: codecov/patch
components/icons/VerifiedIcon.tsx
[warning] 4-31: components/icons/VerifiedIcon.tsx#L4-L31
Added lines #L4 - L31 were not covered by tests
🔇 Additional comments (6)
components/factory/components/index.ts (1)
3-3: LGTM!The export statement for the new
DenomDisplaycomponent follows the established pattern.components/factory/components/DenomDisplay.tsx (1)
18-29: LGTM on the component structure!The component effectively combines the token image and name display with conditional verified icon rendering.
components/icons/VerifiedIcon.tsx (1)
4-33: Add test coverage for the VerifiedIcon component.Static analysis indicates missing test coverage for this component.
Would you like me to generate test cases for the VerifiedIcon component? The tests should verify:
- Component renders without crashing
- Props are correctly passed to the SVG element
- Accessibility attributes are present
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 4-31: components/icons/VerifiedIcon.tsx#L4-L31
Added lines #L4 - L31 were not covered by testscomponents/bank/forms/ibcSendForm.tsx (1)
287-294: LGTM on DenomDisplay integration!The new DenomDisplay component is correctly integrated in both the token selector and dropdown list, with appropriate props passed for denomination and metadata.
Also applies to: 330-334
components/factory/components/DenomList.tsx (2)
3-4: LGTM! Import changes align with the PR objectives.The addition of
DenomDisplayimport while retainingDenomImagesuggests a gradual transition, which is consistent with the planned refactoring mentioned in the PR description.
486-486: LGTM! DenomDisplay integration looks good.The replacement of the token display logic with the new
DenomDisplaycomponent aligns with the PR objective of adding verified icons next to denomination names.
|
@fmorency This is ready to review. We can move the list of verified token somewhere else when we have more than 1 :) |
There is a refactor to be done here which I'll file an issue for: moving all the logic of having the
<DenomImage />and<VerifiedIcon />inside a new componentDenomNameor similar. I think this should be done as a follow up.Right now I found 3 places where this token shows up as is. You can see all 3 in the screenshot:
Let me know if that's sufficient to consider the issue fixed.
Fixes #218
Fixes #243
Summary by CodeRabbit
New Features
DenomDisplaycomponent for improved token information presentation, consolidating the display of token images and names.Bug Fixes
Style