fix: validate ticker contains subdenom #123
Conversation
WalkthroughThe changes in this pull request focus on enhancing the validation logic within the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 #123 +/- ##
==========================================
- Coverage 60.23% 60.13% -0.11%
==========================================
Files 148 148
Lines 14806 14821 +15
==========================================
- Hits 8919 8913 -6
- Misses 5887 5908 +21 ☔ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/factory/modals/updateDenomMetadata.tsx(3 hunks)
🔇 Additional comments (2)
components/factory/modals/updateDenomMetadata.tsx (2)
122-122: LGTM! Validation schema properly initialized.
The schema is correctly initialized with the baseDenom, ensuring proper validation context.
12-20: LGTM! Verify edge cases for subdenom validation.
The validation logic correctly implements the requirement to check if the display includes the subdenom. The case-insensitive comparison and handling of empty subdenom are good practices.
Let's verify the subdenom patterns in the codebase:
✅ Verification successful
Let me analyze the results to verify the subdenom patterns:
- From the test files, we can see that subdenoms are expected to start with 'u' (e.g., 'utoken', 'utest')
- The factory denom pattern in tests shows: 'factory/cosmos1address/subtesttoken'
- Multiple files consistently use
subdenom.slice(1)pattern for validation - The test files show validation for invalid subdenoms
Let me verify one more aspect about the validation patterns:
Let me try one final verification to check the validation rules:
Based on the comprehensive analysis of the codebase, I can now provide the final response:
The subdenom validation implementation is safe and correct
The validation logic is well-implemented with proper safeguards:
- Tests confirm subdenoms follow the 'u' prefix pattern (e.g., 'utoken', 'utest')
- The
slice(1)operation is safe as subdenoms are consistently prefixed - Case-insensitive comparison prevents user confusion
- Empty subdenom check prevents validation errors
- Factory denom pattern 'factory/cosmos1address/subtesttoken' is consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check subdenom patterns to ensure slice(1) is always safe
# and verify if there are any edge cases we should handle
# Search for subdenom assignments or definitions
rg -A 2 'subdenom.*='
# Search for factory denom patterns
rg -A 2 'factory/[^/]+/\w+'
Length of output: 8770
Script:
#!/bin/bash
# Check validation patterns for subdenoms
rg -A 3 'subdenom.*validation|validateSubdenom'
# Check if there are any other subdenom-related tests
ast-grep --pattern 'test($_, $body) { $$$ subdenom $$$ }'
Length of output: 109
Script:
#!/bin/bash
# Search for Yup validation rules related to subdenom
rg -A 5 'Yup.*subdenom'
# Check for any validation error messages related to subdenom
rg 'subdenom.*error|error.*subdenom'
Length of output: 188
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Félix C. Morency <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/factory/modals/updateDenomMetadata.tsx (2)
12-27: Enhance validation test clarity and error message.The validation logic correctly implements the subdenom check, but could be improved for better user experience:
Consider these improvements:
- .test('display-contains-subdenom', 'Display must contain subdenom', function (value) { + .test('display-contains-token-name', 'Display must contain the token name (FOOBAR for ufoobar)', function (value) { const subdenom = context.subdenom; return !subdenom || value.toLowerCase().includes(subdenom.slice(1).toLowerCase()); }),
150-157: Consider accessibility improvements for the subdenom field.While the hover functionality is good for sighted users, consider enhancing accessibility for screen readers:
<TextInput label="SUBDENOM" name="subdenom" value={fullDenom} title={fullDenom} - disabled={true} + readOnly={true} + aria-label={`Full denomination: ${fullDenom}`} helperText="This field cannot be modified" />Using
readOnlyinstead ofdisabledensures the field remains in the tab order and is announced by screen readers. Thearia-labelprovides explicit context about the full denomination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/factory/modals/updateDenomMetadata.tsx(3 hunks)
🔇 Additional comments (2)
components/factory/modals/updateDenomMetadata.tsx (2)
122-122: LGTM: Correct schema initialization with context.
The schema is properly initialized with the baseDenom context, ensuring proper validation timing.
17-20: Verify validation behavior with different subdenom formats.
The validation looks correct, but let's verify it handles various subdenom formats properly.
✅ Verification successful
Validation behavior is consistent and correctly handles subdenom formats
The validation implementation is verified to be correct and consistent across the codebase:
- The same validation logic is used in both token creation (
TokenDetailsForm.tsx) and token update (updateDenomMetadata.tsx) - The validation correctly:
- Handles the subdenom by removing the first character (the prefix) using
slice(1) - Uses case-insensitive comparison with
toLowerCase() - Properly handles empty subdenom cases with the
!subdenom ||check
- Handles the subdenom by removing the first character (the prefix) using
- Test coverage in
CreateDenom.test.tsxconfirms the validation behavior for subdenom inputs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other token validation implementations to ensure consistency
echo "Searching for token validation patterns..."
rg -A 5 "test.*subdenom|test.*display.*contains" --type typescript
# Search for token creation form validation for comparison
echo "Searching for token creation form validation..."
ast-grep --pattern 'const TokenDetailsSchema = $_' --type typescript
Length of output: 554
Script:
#!/bin/bash
# Search for token validation patterns without file type restriction
echo "Searching for token validation patterns..."
rg -A 5 "test.*subdenom|test.*display.*contains"
# Search for token creation form validation
echo "Searching for token creation form validation..."
rg -A 10 "TokenDetailsSchema"
Length of output: 7230
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/factory/modals/updateDenomMetadata.tsx (2)
12-20: Improve validation error message clarity.The error message "Display must contain subdenom" could be more specific about the expected format. Consider providing an example in the error message.
- .test('display-contains-subdenom', 'Display must contain subdenom', function (value) { + .test('display-contains-subdenom', + (params) => `Display must contain subdenom "${params.options.context.subdenom.slice(1).toUpperCase()}"`, + function (value) {
155-162: Consider optimizing title attribute usage.The
titleattribute currently duplicates the visiblevalue. Consider making it more informative by providing additional context about the denom structure.- title={fullDenom} + title={`Full denomination path: ${fullDenom}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/factory/modals/updateDenomMetadata.tsx(3 hunks)
🔇 Additional comments (2)
components/factory/modals/updateDenomMetadata.tsx (2)
127-127: LGTM! Proper integration with validation context.
The schema initialization is correctly implemented as an arrow function, ensuring fresh context on each validation run.
17-19: Verify alignment with token creation form validation.
Let's ensure this validation logic matches the token creation form implementation.
✅ Verification successful
Validation logic is correctly aligned between forms
The validation logic for display-contains-subdenom is identical between the token creation form and update modal:
- Both check if the display value includes the subdenom (excluding the first character)
- Both perform case-insensitive comparison
- Both handle the case when subdenom is empty
- Both use the same error message
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare validation logic between update modal and creation form
# Expected: Both forms should use similar validation for subdenom
# Search for similar validation patterns in token creation related files
rg -l "test.*display.*subdenom" --type typescript
# Check for any other validation schemas
ast-grep --pattern 'const $schema = $_ => Yup.object().shape({
$$$
display: $_
$$$
})'
Length of output: 181
Script:
#!/bin/bash
# Let's try different approaches to find validation logic
# Search for validation patterns without file type restriction
rg -l "test.*display.*subdenom"
# Search for Yup validation schemas
rg "Yup\.object\(\)\.shape" -A 5 -B 5
# Look for files containing both display and subdenom
rg -l "display.*subdenom|subdenom.*display"
# Search for form validation files
fd "validation|schema" --type f
Length of output: 22327
Script:
#!/bin/bash
# Let's examine the specific validation logic in both files
# Check the validation implementation in TokenDetailsForm
rg "display-contains-subdenom" components/factory/forms/TokenDetailsForm.tsx -A 3
# Check the validation implementation in updateDenomMetadata
rg "display-contains-subdenom" components/factory/modals/updateDenomMetadata.tsx -A 3
Length of output: 645
This PR validates the ticker contains the subdenom. This is consistent with the token creation form.
This PR also displays the full denom on mouse hover.
Fixes #121
Summary by CodeRabbit
New Features
Accessibility Improvements
titleattribute to theSUBDENOMfield for better context and accessibility.subdemomtosubdenomfor clarity.Bug Fixes
nameanddescriptionfields to be required, improving form validation.