Add denom prefix #182
Add denom prefix #182
Conversation
WalkthroughThe pull request introduces changes to the denom creation process across three components: Changes
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 (
|
|
fixes #179 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/factory/forms/CreateDenom.tsx (1)
59-59: Add helper text about automatic prefix.While removing the 'u' prefix from the placeholder aligns with the new design, users should be informed that a 'u' prefix will be automatically added to their input. Consider adding helper text to improve user experience.
<TextInput label="Token Sub Denom" name="subdenom" placeholder="token" + helperText="A 'u' prefix will be automatically added to your token name" value={formData.subdenom}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/factory/forms/ConfirmationForm.tsx(2 hunks)components/factory/forms/CreateDenom.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
components/factory/forms/CreateDenom.tsx (2)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/factory/forms/CreateDenom.tsx:106-120
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In the file `components/factory/forms/CreateDenom.tsx`, the submit button is intentionally placed outside the `Form` component for stylistic reasons, as per the user's preference, even if it requires some workarounds.
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/factory/forms/CreateDenom.tsx:20-31
Timestamp: 2024-11-12T13:03:18.927Z
Learning: Validation schemas that are specific to a single form and unlikely to be reused should remain within the form file.
🔇 Additional comments (3)
components/factory/forms/CreateDenom.tsx (2)
127-127: LGTM! Good accessibility improvement.Adding
type="submit"improves accessibility and follows HTML form best practices.
Line range hint
22-34: LGTM! Validation changes align with the new design.The removal of the 'u' prefix validation while maintaining other important validations (length, profanity, simulation) aligns well with the new approach of handling the prefix programmatically.
components/factory/forms/ConfirmationForm.tsx (1)
Line range hint
55-70: Verify metadata structure for token creationThe metadata structure, particularly the denomUnits, needs verification to ensure it aligns with the new denom prefixing strategy.
Run this script to check existing token metadata patterns:
Also, consider adding validation to ensure the metadata structure matches the chain's requirements:
- Base denom should use the prefixed subdenom
- Display denom should use the correct symbol
- Exponents and aliases should be consistent
Also applies to: 119-134
✅ Verification successful
Metadata structure verification completed - Implementation is correct
The current metadata structure in ConfirmationForm.tsx follows the established pattern across the codebase, with proper denom prefixing, consistent exponents (0 and 6), and correct alias cross-referencing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for token metadata patterns in the codebase # Look for denomUnits patterns rg -A 10 'denomUnits:.*\[' # Look for existing token creation patterns ast-grep --pattern 'createDenom({ $$$, subdenom: $_, $$$})'Length of output: 4995
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/factory/forms/TokenDetailsForm.tsx (1)
81-81: Consider improving UI feedbackWhile the 'u' prefix is correctly displayed, consider adding helper text to explain the automatic prefix addition to users.
<TextInput label="Subdenom" name="subdenom" disabled={true} value={`u${formData.subdenom}`} + helperText="The 'u' prefix is automatically added to your token's subdenom" aria-label="Token subdenom" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/factory/forms/ConfirmationForm.tsx(5 hunks)components/factory/forms/CreateDenom.tsx(5 hunks)components/factory/forms/TokenDetailsForm.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/factory/forms/ConfirmationForm.tsx
🧰 Additional context used
📓 Learnings (1)
components/factory/forms/CreateDenom.tsx (2)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/factory/forms/CreateDenom.tsx:20-31
Timestamp: 2024-11-12T13:03:18.927Z
Learning: Validation schemas that are specific to a single form and unlikely to be reused should remain within the form file.
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/factory/forms/CreateDenom.tsx:106-120
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In the file `components/factory/forms/CreateDenom.tsx`, the submit button is intentionally placed outside the `Form` component for stylistic reasons, as per the user's preference, even if it requires some workarounds.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
components/factory/forms/TokenDetailsForm.tsx (2)
46-47: LGTM: Denom prefix handling implemented correctlyThe
fullDenomconstruction now correctly includes the 'u' prefix for the subdenom, aligning with the PR objective.
52-52: Verify the impact of using display value in denomUnitsThe change from using
subdenomtodisplayin denomUnits array might affect token metadata creation. Please ensure this aligns with the blockchain's token metadata requirements.Also applies to: 56-56
✅ Verification successful
Using
displayin denomUnits is correct and consistentThe change from
subdenomtodisplayaligns with the token metadata patterns used throughout the codebase. All token implementations consistently use exponent 6 for the display/symbol denomination, while maintaining the full denom with exponent 0 as the base unit.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing token metadata creation patterns rg -A 5 "denomUnits.*exponent.*6"Length of output: 35
Script:
#!/bin/bash # Search for denomUnits patterns and token metadata rg -U "denomUnits.*\[" -A 5 rg -U "interface.*Token" -A 10 rg -U "type.*Token" -A 10Length of output: 4877
components/factory/forms/CreateDenom.tsx (3)
30-30: LGTM: Simulation now includes denom prefixThe
simulateDenomCreationfunction now correctly includes the 'u' prefix when checking denom availability.
68-68: LGTM: UI improvementsGood improvements to the UI:
- Simplified placeholder text
- Added error clearing on change
- Proper submit button type
Also applies to: 77-77, 137-137
53-54: Verify validation timing implicationsSetting both
validateOnChangeandvalidateOnBlurtofalsemeans validation only occurs on submit. This might delay feedback to users about invalid inputs.Consider enabling at least one of these for better user experience:
-validateOnChange={false} -validateOnBlur={false} +validateOnChange={false} +validateOnBlur={true}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
components/factory/forms/__tests__/CreateDenom.test.tsx (2)
Line range hint
35-41: Update test input to align with new prefix handlingThe test is still manually including the 'u' prefix in the input value ('utest'), while according to the PR objectives, this prefix should be added programmatically. Update the test to use 'test' instead of 'utest' to verify the automatic prefix addition.
- fireEvent.change(subdenomInput, { target: { value: 'utest' } }); + fireEvent.change(subdenomInput, { target: { value: 'test' } }); await waitFor(() => { expect(mockProps.dispatch).toHaveBeenCalledWith({ type: 'UPDATE_FIELD', field: 'subdenom', - value: 'utest', + value: 'test', }); });
Line range hint
49-55: Add test coverage for automatic prefix handlingThe test suite is missing coverage for the new automatic 'u' prefix functionality. Add a test case to verify that the denom creation simulation receives the correct prefixed value.
Here's a suggested test case to add:
test('automatically adds u prefix during simulation', async () => { renderWithChainProvider(<CreateDenom {...mockProps} />); const subdenomInput = screen.getByPlaceholderText('token'); fireEvent.change(subdenomInput, { target: { value: 'test' } }); fireEvent.blur(subdenomInput); const simulateDenomCreation = require('@/utils/transactionUtils').useSimulateDenomCreation().simulateDenomCreation; await waitFor(() => { expect(simulateDenomCreation).toHaveBeenCalledWith( expect.objectContaining({ subdenom: 'utest' }) ); }); });
🧹 Nitpick comments (2)
components/factory/forms/CreateDenom.tsx (2)
43-64: Optimize error handling reducerGood improvement on error handling with proper TypeScript types! However, the spread operator in the reducer could cause performance issues with large error sets.
Consider this optimization:
- const errors = err.inner.reduce( - (acc: Record<string, string>, error) => ({ - ...acc, - [error.path as string]: error.message, - }), - {} - ); + const errors: Record<string, string> = {}; + err.inner.forEach((error) => { + if (error.path) { + errors[error.path] = error.message; + } + });🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
80-80: Add helper text about 'u' prefixSince the 'u' prefix is now added programmatically, consider adding helper text to inform users that their input will be prefixed with 'u'.
<TextInput label="Token Sub Denom" name="subdenom" placeholder="token" + helperText="Will be prefixed with 'u' automatically" value={formData.subdenom}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/factory/forms/ConfirmationForm.tsx(5 hunks)components/factory/forms/CreateDenom.tsx(5 hunks)components/factory/forms/__tests__/CreateDenom.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/factory/forms/ConfirmationForm.tsx
🧰 Additional context used
📓 Learnings (1)
components/factory/forms/CreateDenom.tsx (2)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/factory/forms/CreateDenom.tsx:20-31
Timestamp: 2024-11-12T13:03:18.927Z
Learning: Validation schemas that are specific to a single form and unlikely to be reused should remain within the form file.
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/factory/forms/CreateDenom.tsx:106-120
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In the file `components/factory/forms/CreateDenom.tsx`, the submit button is intentionally placed outside the `Form` component for stylistic reasons, as per the user's preference, even if it requires some workarounds.
🪛 Biome (1.9.4)
components/factory/forms/CreateDenom.tsx
[error] 51-51: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (4)
components/factory/forms/CreateDenom.tsx (4)
65-66: Verify UX impact of delayed validationDisabling validation on change and blur means users won't get immediate feedback. While this might reduce unnecessary simulations, it could impact user experience.
Consider enabling validation for basic rules (length, required) while keeping simulation validation on submit only.
89-89: LGTM!Good practice to clear errors on input change to avoid showing stale error messages.
149-149: LGTM!Adding
type="submit"improves HTML semantics while maintaining the intentional button placement outside the Form component.
30-30: Verify prefix handlingWhile adding the 'u' prefix is correct, consider validating that the subdenom doesn't already start with 'u' to prevent potential double prefixing.
✅ Verification successful
Prefix handling is correctly implemented
The current implementation follows the codebase's consistent pattern where subdenom is stored without prefix in formData and the 'u' prefix is added only when needed. The validation logic and related components already handle prefix management appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing uses of the subdenom to ensure consistent prefix handling rg -l "formData\.subdenom" | xargs rg "u.*formData\.subdenom"Length of output: 723
Script:
#!/bin/bash # Check getDenomInfo implementation and validation rules ast-grep --pattern 'function getDenomInfo($_) { $$$ }' rg -A 5 "subdenom.*validation|validation.*subdenom"Length of output: 717
Script:
#!/bin/bash # Search for TokenDetailsSchema implementation and related validation ast-grep --pattern 'const TokenDetailsSchema = $_' rg -A 10 "TokenDetailsSchema.*="Length of output: 5779
This PR:
Summary by CodeRabbit
New Features
subdenomwith 'u'.fullDenomanddenomUnits.Bug Fixes
type="submit".