feat: token factory group controls #158
Conversation
WalkthroughThis pull request introduces comprehensive changes across multiple components to enhance group address management, token interaction, and user experience. The modifications primarily focus on integrating a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (3)
package.json (1)
Line range hint
27-31: Update resolutions to match new React versionsThe resolutions section still pins React to older versions (18.2.0) while the dependencies use 18.3.1. This could lead to version conflicts.
Apply this diff to update the resolutions:
"resolutions": { - "react": "18.2.0", - "react-dom": "18.2.0", - "@types/react": "18.0.25", - "@types/react-dom": "18.0.9", + "react": "18.3.1", + "react-dom": "18.3.1", + "@types/react": "18.3.5", + "@types/react-dom": "18.3.0", "**/@chain-registry/types": "0.25.0", "**/@cosmjs/stargate": "npm:@liftedinit/[email protected]" },components/factory/modals/BurnModal.tsx (1)
Line range hint
41-42: Remove hardcoded admin addressThe fallback admin address should not be hardcoded in the component. Consider moving it to a configuration file or environment variable.
const { groupByAdmin, isGroupByAdminLoading } = useGroupsByAdmin( - poaAdmin ?? 'manifest1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzsfmy9qj' + poaAdmin ?? process.env.NEXT_PUBLIC_DEFAULT_ADMIN_ADDRESS );components/factory/modals/MintModal.tsx (1)
Line range hint
45-46: Remove hardcoded admin addressSimilar to BurnModal, the fallback admin address should not be hardcoded.
const { groupByAdmin, isGroupByAdminLoading } = useGroupsByAdmin( - admin ?? 'manifest1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzsfmy9qj' + admin ?? process.env.NEXT_PUBLIC_DEFAULT_ADMIN_ADDRESS );
🧹 Nitpick comments (13)
components/factory/modals/TransferModal.tsx (3)
11-12: Consolidate imports forMsgChangeAdminandAny.Currently,
MsgChangeAdminandAnyare imported from specific paths within themanifestjspackage:import { MsgChangeAdmin } from '@liftedinit/manifestjs/dist/codegen/osmosis/tokenfactory/v1beta1/tx'; import { Any } from '@liftedinit/manifestjs/dist/codegen/google/protobuf/any';It's preferable to import these modules directly from the main package exports to improve maintainability and reduce dependency on internal package structures.
Consider updating the imports as follows:
-import { MsgChangeAdmin } from '@liftedinit/manifestjs/dist/codegen/osmosis/tokenfactory/v1beta1/tx'; -import { Any } from '@liftedinit/manifestjs/dist/codegen/google/protobuf/any'; +import { osmosis, google } from '@liftedinit/manifestjs'; +const { MsgChangeAdmin } = osmosis.tokenfactory.v1beta1; +const { Any } = google.protobuf;
78-98: Simplify message construction and improve readability.The construction of
transferPropis complex and may affect readability. Refactoring can help simplify the code and make it more maintainable.Consider extracting the message creation into a separate function:
const createTransferProposalMessage = (newAdmin: string): cosmos.group.v1.MsgSubmitProposal => { return submitProposal({ groupPolicyAddress: selectedAddress ?? '', messages: [ Any.fromPartial({ typeUrl: MsgChangeAdmin.typeUrl, value: MsgChangeAdmin.encode( changeAdmin({ sender: selectedAddress ?? '', denom: denom?.base ?? '', newAdmin: newAdmin, }).value ).finish(), }), ], metadata: '', proposers: [address], title: 'Transfer Ownership', summary: `This proposal will transfer ownership of ${denom?.display?.split('/').pop()} to ${newAdmin}`, exec: 0, }); };Then use this function in your
handleTransfer:try { const msg = changeAdmin({ sender: address, denom: denom?.base ?? '', newAdmin: values.newAdmin, }); - const transferProp = submitProposal({ ... }); + const transferProp = createTransferProposalMessage(values.newAdmin); const fee = await estimateFee(address, [isGroup ? transferProp : msg]); await tx([isGroup ? transferProp : msg], { fee, onSuccess: () => { ... } });
99-99: Optimize fee estimation by reusing the message variable.The code currently repeats the conditional check for
isGroupwhen preparing the fee estimation and transaction execution. This can be optimized by assigning the message to a variable.Assign the message based on
isGrouponce, then use it for both fee estimation and transaction execution:+const message = isGroup ? transferProp : msg; -const fee = await estimateFee(address, [isGroup ? transferProp : msg]); +const fee = await estimateFee(address, [message]); -await tx([isGroup ? transferProp : msg], { +await tx([message], { fee, onSuccess: () => { onSuccess(); handleCloseModal(resetForm); }, });hooks/usePoaLcdQueryClient.ts (2)
10-10: Consider consistent return property namingWhile the query key change to 'poaQueryClient' aligns with the naming pattern, the return property
lcdQueryClientis inconsistent with the pattern established inuseManifestLcdQueryClient. Consider renaming it topoaLcdQueryClientfor consistency.return { - lcdQueryClient: lcdQueryClient.data, + poaLcdQueryClient: lcdQueryClient.data, };Also applies to: 19-21
Line range hint
1-21: Consider standardizing LCD client hook patternsThe codebase now has three LCD client hooks with similar patterns but slight inconsistencies. Consider:
- Creating a shared interface/type for LCD clients
- Standardizing the return property naming convention
- Documenting the query key naming pattern
This would improve maintainability and make the codebase more predictable.
stores/groupAddressStore.tsx (1)
16-27: Consider adding validation for address formatWhile the
setSelectedAddressimplementation handles empty/null cases, it should also validate the address format to prevent invalid states.setSelectedAddress: address => set({ - selectedAddress: address && address.length > 0 ? address : null, + selectedAddress: address && address.length > 0 && isValidAddress(address) ? address : null, }),components/factory/modals/MintModal.tsx (1)
Line range hint
1-134: Consider extracting common modal logicBoth MintModal and BurnModal share significant code duplication in terms of modal structure, styling, and group management logic. Consider extracting these into a reusable base component.
Create a new component
BaseTokenModalthat handles the common functionality:
- Modal structure and styling
- Escape key handling
- Group management logic
- Loading states
Then extend it for specific mint/burn operations:
function BaseTokenModal({ denom, title, children, ...commonProps }: BaseTokenModalProps) { // Common modal logic } function MintModal(props: MintModalProps) { return ( <BaseTokenModal title="Mint" {...props} > <MintForm {...mintFormProps} /> </BaseTokenModal> ); }pages/factory/index.tsx (1)
29-30: Simplify address fallback logicThe ternary operation with nullish coalescing can be simplified using the
effectiveAddressvariable already defined.- useTokenBalances(selectedAddress ? selectedAddress : (address ?? '')); + useTokenBalances(effectiveAddress);components/groups/modals/voteDetailsModal.tsx (2)
582-600: Reduce code duplication in syntax highlighting implementation.The syntax highlighting configuration is duplicated in two places. Consider extracting it into a reusable component.
Create a new component like this:
interface JsonHighlighterProps { code: Record<string, unknown>; theme: string; } const JsonHighlighter: React.FC<JsonHighlighterProps> = ({ code, theme }) => ( <SyntaxHighlighter language="json" style={theme === 'dark' ? oneDark : oneLight} customStyle={{ backgroundColor: 'transparent', padding: '1rem', borderRadius: '0.5rem', }} > {prettyPrintJSON(code)} </SyntaxHighlighter> );Then use it in both places:
<JsonHighlighter code={fieldsToShow.reduce<Record<string, any>>((acc, field) => { acc[field] = message[field]; return acc; }, {})} theme={theme} />Also applies to: 771-789
Line range hint
1-892: Consider breaking down the component for better maintainability.The component has grown quite large and handles multiple responsibilities:
- Vote details display
- Message formatting
- Chart rendering
- Modal management
- Vote actions
Consider splitting it into smaller, focused components:
- VoteHeader
- VoteSummary
- VoteMessages
- VoteChart
- VoteActions
This would improve:
- Code maintainability
- Testing
- Reusability
- Performance (through better memoization opportunities)
components/factory/forms/MintForm.tsx (1)
35-36: Eliminate redundant state variables by utilizing Formik's state managementThe component manages
amountandrecipientboth in component state and within Formik's state. This redundancy can be removed by directly using Formik's values, simplifying state management and reducing potential inconsistencies.Apply this diff to refactor the code:
- const [amount, setAmount] = useState(''); - const [recipient, setRecipient] = useState(address); ... - value={amount} - onChange={(e: React.ChangeEvent<HTMLInputElement>) => { - setAmount(e.target.value); - setFieldValue('amount', e.target.value); - }} + value={values.amount} + onChange={handleChange} ... - value={recipient} - onChange={(e: React.ChangeEvent<HTMLInputElement>) => { - setRecipient(e.target.value); - setFieldValue('recipient', e.target.value); - }} + value={values.recipient} + onChange={handleChange} ... Update `handleMint` to use Formik's values: ```diff - const handleMint = async () => { - if (!amount || Number.isNaN(Number(amount))) { + const handleMint = async (values: { amount: string; recipient: string }) => { + if (!values.amount || Number.isNaN(Number(values.amount))) { ... - const amountInBaseUnits = parseNumberToBigInt(amount, exponent).toString(); + const amountInBaseUnits = parseNumberToBigInt(values.amount, exponent).toString(); ... - mintToAddress: recipient, + mintToAddress: values.recipient, ...Adjust the form submission to pass Formik's values:
- onSubmit={values => { - setAmount(values.amount); - setRecipient(values.recipient); - handleMint(); - }} + onSubmit={values => { + handleMint(values); + }}Ensure to remove the
amountandrecipientstate variables and rely on Formik's state management.Also applies to: 156-158, 176-180
components/factory/forms/BurnForm.tsx (1)
45-46: Eliminate redundant state variables by utilizing Formik's state managementThe component maintains
amountandrecipientin both component state and Formik's state. This duplication can be removed by using Formik's values directly, simplifying state handling and minimizing inconsistencies.Apply this diff to refactor the code:
- const [amount, setAmount] = useState(''); - const [recipient, setRecipient] = useState(address); ... - value={amount} - onChange={(e: React.ChangeEvent<HTMLInputElement>) => { - setAmount(e.target.value); - setFieldValue('amount', e.target.value); - }} + value={values.amount} + onChange={handleChange} ... - value={recipient} - onChange={(e: React.ChangeEvent<HTMLInputElement>) => { - setRecipient(e.target.value); - setFieldValue('recipient', e.target.value); - }} + value={values.recipient} + onChange={handleChange} ... Update `handleBurn` to use Formik's values: ```diff - const handleBurn = async () => { - if (!amount || Number.isNaN(Number(amount))) { + const handleBurn = async (values: { amount: string; recipient: string }) => { + if (!values.amount || Number.isNaN(Number(values.amount))) { ... - const amountInBaseUnits = parseNumberToBigInt(amount, exponent).toString(); + const amountInBaseUnits = parseNumberToBigInt(values.amount, exponent).toString(); ... - burnFromAddress: recipient, + burnFromAddress: values.recipient, ...Adjust the form submission to pass Formik's values:
- onSubmit={values => { - setAmount(values.amount); - setRecipient(values.recipient); - handleBurn(); - }} + onSubmit={values => { + handleBurn(values); + }}Ensure to remove the
amountandrecipientstate variables and rely on Formik's state management.Also applies to: 187-196, 217-221
components/factory/components/MyDenoms.tsx (1)
219-245: Replace<a>elements with<button>for better semanticsIn the address selection dropdown,
<a>elements withouthrefattributes are used as interactive items. For better semantics and accessibility, consider using<button>elements instead.Apply this diff:
- <a - className="flex flex-row items-center gap-2 px-2 py-2" - onClick={() => setSelectedAddress(address)} - > + <button + type="button" + className="flex flex-row items-center gap-2 w-full text-left px-2 py-2" + onClick={() => setSelectedAddress(address)} + > ... - </a> + </button>Make similar changes for other address items in the dropdown to enhance accessibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (22)
components/factory/components/MyDenoms.tsx(5 hunks)components/factory/forms/BurnForm.tsx(2 hunks)components/factory/forms/MintForm.tsx(4 hunks)components/factory/modals/BurnModal.tsx(3 hunks)components/factory/modals/MintModal.tsx(4 hunks)components/factory/modals/TransferModal.tsx(6 hunks)components/factory/modals/updateDenomMetadata.tsx(5 hunks)components/groups/components/myGroups.tsx(4 hunks)components/groups/modals/voteDetailsModal.tsx(6 hunks)components/icons/GithubIcon.tsx(1 hunks)components/icons/index.ts(1 hunks)components/wallet.tsx(1 hunks)contexts/groupAddressProvider.tsx(1 hunks)hooks/useLcdQueryClient.ts(1 hunks)hooks/useManifestLcdQueryClient.ts(2 hunks)hooks/usePoaLcdQueryClient.ts(1 hunks)hooks/useQueries.ts(5 hunks)package.json(2 hunks)pages/_app.tsx(2 hunks)pages/factory/index.tsx(3 hunks)pages/groups/index.tsx(2 hunks)stores/groupAddressStore.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/icons/index.ts
🧰 Additional context used
📓 Learnings (2)
pages/groups/index.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#128
File: components/groups/components/groupControls.tsx:0-0
Timestamp: 2024-12-09T18:31:14.035Z
Learning: In the `GroupProposals` component of `components/groups/components/groupControls.tsx`, data loading is handled in the parent component, so null checks for `groupByMemberData.groups` are not necessary.
components/factory/forms/BurnForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/factory/forms/BurnForm.tsx:262-265
Timestamp: 2024-11-12T13:03:18.927Z
Learning: When suggesting the removal of state variables managed by Formik, ensure that any functions or components relying on those state variables outside of Formik are refactored to use Formik's values.
🔇 Additional comments (17)
components/factory/modals/TransferModal.tsx (1)
78-98: Verify that all required fields for submitProposal are correctly populated.
Ensure that fields like metadata and exec in the submitProposal message are set according to the application's requirements. An empty string for metadata might not be sufficient.
Run the following script to check for required fields and their expected values in the submitProposal documentation:
Review the output to confirm that all required fields are properly set.
✅ Verification successful
Based on the code analysis, I can now generate the final response:
The submitProposal implementation is correct and complete
All required fields for submitProposal are properly populated according to the SubmitProposalMessage type definition found in the codebase:
groupPolicyAddress: Set from selected addressmessages: Contains the encodedMsgChangeAdminmessagemetadata: Empty string is valid as per type definitionproposers: Set with the user's addressexec: Set to 0, which is a valid value for execution strategy- Additional fields like
titleandsummaryare properly set with meaningful values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify required fields for MsgSubmitProposal.
# Check the protobuf definition for MsgSubmitProposal.
grep -A 10 'message MsgSubmitProposal' $(fd proto | grep cosmos/group/v1/tx.proto)
Length of output: 85
Script:
#!/bin/bash
# Let's try to find the submitProposal function definition and usage in the codebase
ast-grep --pattern 'function submitProposal($_) {
$$$
}'
# Also search for any type definitions or interfaces related to submitProposal
rg "type.*submitProposal" -A 5
rg "interface.*Proposal" -A 10
Length of output: 393
Script:
#!/bin/bash
# Let's check the formReducer.tsx file for more context about submitProposal
cat $(fd formReducer.tsx)
# Also search for any imports or usage of submitProposal in TransferModal.tsx
rg "submitProposal" components/factory/modals/TransferModal.tsx -B 2 -A 2
Length of output: 9392
hooks/useLcdQueryClient.ts (1)
9-9: Verify cache invalidation strategy for query key change
The query key change from 'lcdQueryClient' to 'cosmosQueryClient' is more descriptive, but this breaking change will invalidate existing caches. Ensure this is the intended behavior.
✅ Verification successful
Query key change is safe and follows consistent pattern
The change from 'lcdQueryClient' to 'cosmosQueryClient' is safe as:
- No references to the old 'lcdQueryClient' query key exist in the codebase
- The hook is consistently used across the codebase through its exported interface
- No explicit cache invalidation logic depends on the query key
- The change only affects the internal implementation while the hook's API (
useLcdQueryClient) remains stable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old query key
rg "'lcdQueryClient'" --type ts
Length of output: 39
Script:
#!/bin/bash
# Search for any references to lcdQueryClient or cosmosQueryClient in the codebase
rg -i "queryKey.*('lcdQueryClient'|'cosmosQueryClient')" --type ts -A 2
# Also check for any React Query cache invalidation related code
rg -i "invalidateQueries.*('lcdQueryClient'|'cosmosQueryClient')" --type ts -A 2
# Look for the hook usage to understand the impact
rg "useLcdQueryClient" --type ts -A 2
Length of output: 3551
hooks/useManifestLcdQueryClient.ts (1)
9-9: Verify consumers of the renamed return property
The changes improve naming specificity, but the return property rename from lcdQueryClient to manifestLcdQueryClient requires verification of all consuming components.
Also applies to: 19-19
✅ Verification successful
Let me verify the changes in the codebase by checking the actual implementation of the hook.
Based on the verification results, I can now generate the final response:
Return property rename is properly handled
The change from lcdQueryClient to manifestLcdQueryClient in the return value is safe. The search results show:
- The old
lcdQueryClientreferences found in the codebase are local variables, not consuming the hook's return value - The only consumer (
useQueries.ts) is already using the newmanifestLcdQueryClientproperty name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of the old property name
rg "lcdQueryClient\s*=" --type ts
# Search for destructuring of useManifestLcdQueryClient
rg "const.*=\s*useManifestLcdQueryClient" -A 2 --type ts
Length of output: 475
Script:
#!/bin/bash
# Check the implementation of useManifestLcdQueryClient
cat hooks/useManifestLcdQueryClient.ts
Length of output: 596
stores/groupAddressStore.tsx (1)
6-14: LGTM: Well-structured store interface
The interface clearly defines all necessary state and methods. Good separation of concerns between data, actions, and loading states.
package.json (1)
48-48: LGTM: Syntax highlighter dependencies are properly configured
The addition of @types/react-syntax-highlighter and update of react-syntax-highlighter align with the PR objective of improving syntax highlighting for messages in the vote details modal.
Also applies to: 74-74
components/factory/modals/BurnModal.tsx (1)
16-16: Verify group permission checks
The addition of isGroup prop aligns with the PR objectives for group controls. However, we should verify that proper permission checks are implemented in the BurnForm component.
Also applies to: 26-26, 98-98
✅ Verification successful
Group permission checks are properly implemented
The verification shows that proper group permission checks are implemented in the BurnForm component. Specifically:
- The component handles group-based burns by:
- Using
useGroupAddressStorehook to get the selected group address - Creating a group proposal for burns when
isGroupis true (lines 107-129) - Properly switching between direct burn message and group proposal based on
isGroupflag (line 131-132) - Including appropriate metadata and proposer information in group proposals
- Using
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check BurnForm implementation for group permission checks
ast-grep --pattern 'function BurnForm({ $$$, isGroup, $$$}) {
$$$
if (isGroup) {
$$$
}
$$$
}'
Length of output: 107
Script:
#!/bin/bash
# Let's try a broader search for BurnForm component and its usage of isGroup
ast-grep --pattern 'function BurnForm($$$) {
$$$
}'
# Also search for any isGroup related checks in the file
rg "isGroup" -A 5 -B 5 "components/factory/modals/BurnModal.tsx"
Length of output: 21325
components/factory/modals/MintModal.tsx (1)
20-20: Verify group permission checks
Similar to BurnModal, we should verify that proper permission checks are implemented in the MintForm component.
Also applies to: 32-32, 106-106
✅ Verification successful
Group permission checks are properly implemented
The MintForm component correctly handles group permissions by:
- Using the
isGroupprop to determine the transaction type - Utilizing
selectedAddressfromuseGroupAddressStore()for group transactions - Creating a group proposal via
submitProposalwhenisGroupis true - Properly constructing the mint message within the group proposal
The implementation follows the same pattern as BurnModal and includes all necessary permission checks for group-based minting operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check MintForm implementation for group permission checks
ast-grep --pattern 'function MintForm({ $$$, isGroup, $$$}) {
$$$
if (isGroup) {
$$$
}
$$$
}'
Length of output: 107
Script:
#!/bin/bash
# Let's try a broader search for MintForm component and its usage of isGroup
ast-grep --pattern 'function MintForm($$$) {
$$$
}'
# Also search for any isGroup related checks in files containing MintForm
rg "isGroup" -A 5 -B 5 --glob "**/*MintForm*.{ts,tsx,js,jsx}"
Length of output: 20114
pages/groups/index.tsx (1)
24-24: 🛠️ Refactor suggestion
Verify group policy address mapping
The mapping assumes each group has exactly one policy at index 0. This could lead to runtime errors if a group has no policies.
Consider handling groups with no policies or multiple policies:
- const groupPolicyAddresses = groups?.map(group => group.policies[0].address) ?? [];
+ const groupPolicyAddresses = groups?.reduce((addresses, group) => {
+ if (group.policies.length > 0) {
+ addresses.push(group.policies[0].address);
+ }
+ return addresses;
+ }, [] as string[]) ?? [];pages/_app.tsx (1)
196-234: Verify provider dependencies and initialization order
The GroupAddressProvider is correctly placed in the provider chain, but we should verify it doesn't have any dependencies on other contexts.
✅ Verification successful
Let me gather more information about the GroupAddressProvider to ensure there are no context dependencies.
Let me try a different search strategy to locate the GroupAddressProvider implementation.
Let me check the dependencies of GroupAddressProvider, particularly the useChain hook it's using.
From the search results, I can see that GroupAddressProvider depends on ChainProvider and needs to be placed after it in the provider chain. Let me verify the complete provider order in _app.tsx.
Based on the complete analysis, I can now generate the final response:
Provider order is correctly implemented
The GroupAddressProvider is properly placed in the provider chain:
- It's nested inside ChainProvider, which provides the required chain context it depends on (verified from the useChain hook usage)
- The provider order follows the dependency chain:
ToastProvider └── QueryClientProvider └── ChainProvider └── GroupAddressProvider └── ThemeProvider └── ContactsModalProvider
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for context dependencies in GroupAddressProvider
ast-grep --pattern 'function GroupAddressProvider({ children }) {
$$$
use$_($$$)
$$$
}'
# Search for useContext usage
rg -A 5 'useContext|createContext'
Length of output: 4260
Script:
#!/bin/bash
# Search for GroupAddressProvider implementation and its dependencies
rg -A 10 "GroupAddressProvider" --type tsx --type ts
# Search for any imports in the file containing GroupAddressProvider
fd -e tsx -e ts GroupAddressProvider --exec cat {}
Length of output: 135
Script:
#!/bin/bash
# Search for GroupAddressProvider using ripgrep without file type restriction
rg -A 10 "GroupAddressProvider"
# Find the file containing GroupAddressProvider
fd GroupAddressProvider
Length of output: 2504
Script:
#!/bin/bash
# Check the ChainProvider implementation and useChain hook
rg -A 5 "useChain|ChainProvider"
# Check the provider order in _app.tsx to verify ChainProvider's position
rg -B 10 -A 10 "ChainProvider" pages/_app.tsx
Length of output: 69062
Script:
#!/bin/bash
# Get the full content of _app.tsx to verify provider order
cat pages/_app.tsx
Length of output: 7841
components/wallet.tsx (1)
215-216: LGTM! Clean and simplified button implementation.
The changes streamline the button's click handler and styling while maintaining the hover effect, making the code more maintainable.
components/groups/components/myGroups.tsx (3)
37-37: LGTM! Improved type definition.
The updated prop type better represents the data structure and improves type safety.
57-59: LGTM! Simplified filtering logic.
The filtering logic has been streamlined to work directly with the array, making it more readable and maintainable.
Line range hint 71-84: Verify group selection edge cases.
While the logic looks correct, please ensure it handles these edge cases:
- When no policies exist for a group
- When policies array is empty
- When policy address doesn't match any group
✅ Verification successful
Based on the search results, I can now generate the final response as the code shows consistent handling of edge cases across the codebase:
The group selection logic properly handles all edge cases through multiple safeguards
The implementation includes multiple layers of null checks and fallbacks:
- Explicit check for
group.policies && group.policies.length > 0before accessing policies - Optional chaining (
?.) when accessing policy properties - Nullish coalescing (
??) operators providing fallback values - Type casting with proper null handling for decision policy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for group policy handling patterns
ast-grep --pattern 'group.policies && group.policies.length > 0'
# Search for null checks on policies
rg -A 2 'policies\??\[0\]'
Length of output: 4727
hooks/useQueries.ts (2)
456-456: LGTM! Consistent error handling across hooks.
The addition of error properties to the return objects of multiple hooks improves error handling consistency and enables better error reporting throughout the application.
Also applies to: 613-613, 672-672, 701-701
585-605: Verify the dependency on selectedAddress.
The hook now depends on selectedAddress from the store. Ensure proper error handling and loading states when:
selectedAddressis undefinedmanifestLcdQueryClientis not ready- Network errors occur during fetching
Good practices implemented:
- Retry logic with
retry: 2 - Proper type checking with
Boolean() - Frequent refetching with
staleTime: 0
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information about the error handling and selectedAddress usage patterns.
Error handling and loading states are properly implemented
The hook's dependency on selectedAddress is well-handled:
- Early return with empty denoms array when
selectedAddressormanifestLcdQueryClientis undefined - Query is enabled only when both dependencies are present:
enabled: Boolean(manifestLcdQueryClient && selectedAddress) - The store indicates
selectedAddresscan be null, and the hook handles this case - Network errors are managed through:
- Retry logic with
retry: 2 - Frequent refetching with
staleTime: 0andrefetchOnWindowFocus: true - Query key properly includes all dependencies:
['denoms', selectedAddress, !!manifestLcdQueryClient]
- Retry logic with
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for selectedAddress usage patterns
rg -A 3 'selectedAddress'
# Search for error handling in hooks
ast-grep --pattern 'const { $_, error } = use$_Query'
Length of output: 16096
components/groups/modals/voteDetailsModal.tsx (1)
4-7: LGTM: Clean setup of syntax highlighting.
The imports and language registration for syntax highlighting are well-structured, with proper separation of themes for dark/light modes.
Also applies to: 36-37
components/factory/components/MyDenoms.tsx (1)
184-257: Implementing address selection dropdown enhances user experience
The addition of the address selection dropdown correctly integrates group address management, improving usability and allowing users to switch between different addresses seamlessly.
| @@ -22,6 +25,7 @@ export default function TransferModal({ | |||
| isOpen, | |||
| onClose, | |||
| onSuccess, | |||
| isGroup, | |||
There was a problem hiding this comment.
Ensure isGroup prop is properly documented and has a default value.
The isGroup prop is added to the component props but lacks documentation and a default value. This could lead to misunderstandings about its usage and potential undefined errors if not provided.
Consider adding a default value and documenting the prop:
export default function TransferModal({
openTransferDenomModal,
setOpenTransferDenomModal,
denom,
address,
modalId,
isOpen,
onClose,
onSuccess,
+ isGroup = false,
}: {
openTransferDenomModal: boolean;
setOpenTransferDenomModal: (open: boolean) => void;
denom: ExtendedMetadataSDKType | null;
address: string;
modalId: string;
isOpen: boolean;
onClose: () => void;
onSuccess: () => void;
+ isGroup?: boolean;
}) {Also, update any documentation or comments to explain the purpose of isGroup.
Also applies to: 38-38
| @@ -43,7 +48,7 @@ | |||
| return () => document.removeEventListener('keydown', handleEscape); | |||
| }, [isOpen]); | |||
| const { setToastMessage } = useToast(); | |||
|
|
|||
| const { selectedAddress } = useGroupAddressStore(); | |||
There was a problem hiding this comment.
Handle potential undefined selectedAddress from useGroupAddressStore.
The selectedAddress retrieved from useGroupAddressStore() may be undefined, which could cause issues when used in transaction messages where an address is required.
Before using selectedAddress, ensure it is defined. If not, provide appropriate error handling or user feedback:
const { selectedAddress } = useGroupAddressStore();
+if (isGroup && !selectedAddress) {
+ setToastMessage({
+ type: 'alert-error',
+ title: 'No Group Address Selected',
+ description: 'Please select a group address before proceeding.',
+ });
+ return;
+}Committable suggestion skipped: line range outside the PR's diff.
| changeAdmin({ | ||
| sender: selectedAddress ?? '', | ||
| denom: denom?.base ?? '', | ||
| newAdmin: values.newAdmin, | ||
| }).value | ||
| ).finish(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid redundant object creation in message encoding.
When encoding the MsgChangeAdmin message, the changeAdmin function is called again inside MsgChangeAdmin.encode. This redundancy can be eliminated by reusing the existing msg object or restructuring the code.
Modify the code to reuse the msg object:
-Any.fromPartial({
- typeUrl: MsgChangeAdmin.typeUrl,
- value: MsgChangeAdmin.encode(
- changeAdmin({
- sender: selectedAddress ?? '',
- denom: denom?.base ?? '',
- newAdmin: values.newAdmin,
- }).value
- ).finish(),
+Any.fromPartial({
+ typeUrl: MsgChangeAdmin.typeUrl,
+ value: MsgChangeAdmin.encode({
+ sender: selectedAddress ?? '',
+ denom: denom?.base ?? '',
+ newAdmin: values.newAdmin,
+ }).finish(),
});This avoids unnecessary calls and improves performance.
Committable suggestion skipped: line range outside the PR's diff.
| export const useInitializeGroupStore = (address: string) => { | ||
| const { groupByMemberData, isGroupByMemberLoading, isGroupByMemberError, refetchGroupByMember } = | ||
| useGroupsByMember(address); | ||
|
|
||
| useEffect(() => { | ||
| if (groupByMemberData?.groups) { | ||
| const policyAddresses = groupByMemberData.groups.flatMap(group => | ||
| group.policies.map(policy => policy.address) | ||
| ); | ||
|
|
||
| useGroupAddressStore.setState({ | ||
| groups: groupByMemberData.groups, | ||
| policyAddresses, | ||
| // Prioritize user's address if it exists | ||
| selectedAddress: | ||
| useGroupAddressStore.getState().selectedAddress ?? address ?? policyAddresses[0] ?? null, | ||
| isGroupByMemberLoading, | ||
| isGroupByMemberError, | ||
| refetchGroupByMember, | ||
| }); | ||
| } | ||
| }, [groupByMemberData, address]); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding cleanup in useEffect
The store initialization effect should clean up its state when the component unmounts to prevent memory leaks.
useEffect(() => {
if (groupByMemberData?.groups) {
// ... existing code ...
}
+ return () => {
+ useGroupAddressStore.setState({
+ groups: [],
+ policyAddresses: [],
+ selectedAddress: null,
+ isGroupByMemberLoading: false,
+ isGroupByMemberError: false,
+ });
+ };
}, [groupByMemberData, address]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const useInitializeGroupStore = (address: string) => { | |
| const { groupByMemberData, isGroupByMemberLoading, isGroupByMemberError, refetchGroupByMember } = | |
| useGroupsByMember(address); | |
| useEffect(() => { | |
| if (groupByMemberData?.groups) { | |
| const policyAddresses = groupByMemberData.groups.flatMap(group => | |
| group.policies.map(policy => policy.address) | |
| ); | |
| useGroupAddressStore.setState({ | |
| groups: groupByMemberData.groups, | |
| policyAddresses, | |
| // Prioritize user's address if it exists | |
| selectedAddress: | |
| useGroupAddressStore.getState().selectedAddress ?? address ?? policyAddresses[0] ?? null, | |
| isGroupByMemberLoading, | |
| isGroupByMemberError, | |
| refetchGroupByMember, | |
| }); | |
| } | |
| }, [groupByMemberData, address]); | |
| }; | |
| export const useInitializeGroupStore = (address: string) => { | |
| const { groupByMemberData, isGroupByMemberLoading, isGroupByMemberError, refetchGroupByMember } = | |
| useGroupsByMember(address); | |
| useEffect(() => { | |
| if (groupByMemberData?.groups) { | |
| const policyAddresses = groupByMemberData.groups.flatMap(group => | |
| group.policies.map(policy => policy.address) | |
| ); | |
| useGroupAddressStore.setState({ | |
| groups: groupByMemberData.groups, | |
| policyAddresses, | |
| // Prioritize user's address if it exists | |
| selectedAddress: | |
| useGroupAddressStore.getState().selectedAddress ?? address ?? policyAddresses[0] ?? null, | |
| isGroupByMemberLoading, | |
| isGroupByMemberError, | |
| refetchGroupByMember, | |
| }); | |
| } | |
| return () => { | |
| useGroupAddressStore.setState({ | |
| groups: [], | |
| policyAddresses: [], | |
| selectedAddress: null, | |
| isGroupByMemberLoading: false, | |
| isGroupByMemberError: false, | |
| }); | |
| }; | |
| }, [groupByMemberData, address]); | |
| }; |
| useEffect(() => { | ||
| if (address && isWalletConnected) { | ||
| useGroupAddressStore.getState().setSelectedAddress(address); | ||
| } | ||
| }, [address, isWalletConnected]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error boundary for wallet connection failures
The effect should handle potential wallet connection errors gracefully.
useEffect(() => {
+ try {
if (address && isWalletConnected) {
useGroupAddressStore.getState().setSelectedAddress(address);
}
+ } catch (error) {
+ console.error('Failed to update address:', error);
+ useGroupAddressStore.getState().setSelectedAddress(null);
+ }
}, [address, isWalletConnected]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (address && isWalletConnected) { | |
| useGroupAddressStore.getState().setSelectedAddress(address); | |
| } | |
| }, [address, isWalletConnected]); | |
| useEffect(() => { | |
| try { | |
| if (address && isWalletConnected) { | |
| useGroupAddressStore.getState().setSelectedAddress(address); | |
| } | |
| } catch (error) { | |
| console.error('Failed to update address:', error); | |
| useGroupAddressStore.getState().setSelectedAddress(null); | |
| } | |
| }, [address, isWalletConnected]); |
| // Only proceed with a valid address | ||
| const effectiveAddress = selectedAddress || address || ''; | ||
| const isValidAddress = Boolean(effectiveAddress && effectiveAddress.length > 0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding address validation
The isValidAddress check only verifies string length. For Cosmos addresses, we should validate the address format.
Add proper address validation:
- const isValidAddress = Boolean(effectiveAddress && effectiveAddress.length > 0);
+ const isValidAddress = Boolean(
+ effectiveAddress &&
+ effectiveAddress.length > 0 &&
+ effectiveAddress.startsWith(env.addressPrefix)
+ );Committable suggestion skipped: line range outside the PR's diff.
| const prettyPrintJSON = (obj: any): string => { | ||
| return JSON.stringify(obj, null, 2); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type safety to prettyPrintJSON function.
The function should handle invalid JSON inputs and have proper TypeScript typing.
Consider this improved implementation:
-const prettyPrintJSON = (obj: any): string => {
- return JSON.stringify(obj, null, 2);
-};
+const prettyPrintJSON = (obj: Record<string, unknown>): string => {
+ try {
+ return JSON.stringify(obj, null, 2);
+ } catch (error) {
+ console.error('Failed to stringify JSON:', error);
+ return '{}';
+ }
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const prettyPrintJSON = (obj: any): string => { | |
| return JSON.stringify(obj, null, 2); | |
| }; | |
| const prettyPrintJSON = (obj: Record<string, unknown>): string => { | |
| try { | |
| return JSON.stringify(obj, null, 2); | |
| } catch (error) { | |
| console.error('Failed to stringify JSON:', error); | |
| return '{}'; | |
| } | |
| }; |
| const mintProp = submitProposal({ | ||
| groupPolicyAddress: selectedAddress ?? '', | ||
| messages: [ | ||
| Any.fromPartial({ | ||
| typeUrl: MsgMint.typeUrl, | ||
| value: MsgMint.encode( | ||
| mint({ | ||
| amount: { | ||
| amount: amountInBaseUnits, | ||
| denom: denom.base, | ||
| }, | ||
| sender: selectedAddress ?? '', | ||
| mintToAddress: recipient, | ||
| }).value | ||
| ).finish(), | ||
| }), | ||
| ], | ||
| metadata: '', | ||
| proposers: [address], | ||
| title: `Mint Tokens`, | ||
| summary: `This proposal will mint ${amount} ${denom.display.split('/').pop()} to ${recipient}`, | ||
| exec: 0, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Ensure 'selectedAddress' is defined before use
The selectedAddress is used with a fallback to an empty string when constructing the proposal message. If selectedAddress is undefined or empty, it may lead to invalid transactions. Please ensure that selectedAddress is correctly defined when isGroup is true to prevent potential errors.
| const updateProp = submitProposal({ | ||
| groupPolicyAddress: selectedAddress ?? '', | ||
| messages: [ | ||
| Any.fromPartial({ | ||
| typeUrl: MsgSetDenomMetadata.typeUrl, | ||
| value: MsgSetDenomMetadata.encode( | ||
| setDenomMetadata({ | ||
| sender: selectedAddress ?? '', | ||
| metadata: { | ||
| description: values.description || formData.description, | ||
| denomUnits: | ||
| [ | ||
| { denom: fullDenom, exponent: 0, aliases: [symbol] }, | ||
| { denom: symbol, exponent: 6, aliases: [fullDenom] }, | ||
| ] || formData.denomUnits, | ||
| base: fullDenom, | ||
| display: symbol, | ||
| name: values.name || formData.name, | ||
| symbol: symbol, | ||
| uri: values.uri || formData.uri, | ||
| uriHash: '', | ||
| }, | ||
| }).value | ||
| ).finish(), | ||
| }), | ||
| ], | ||
| metadata: '', | ||
| proposers: [address], | ||
| title: `Update Metadata`, | ||
| summary: `This proposal will update the metadata for ${denom?.display?.split('/').pop()}`, | ||
| exec: 0, | ||
| }); |
There was a problem hiding this comment.
Ensure 'selectedAddress' is defined before use
The selectedAddress is used with a fallback to an empty string when constructing the proposal message for updating denom metadata. If selectedAddress is undefined or empty, it could lead to invalid transactions. Please verify that selectedAddress is properly defined when isGroup is true to prevent potential issues.
| const burnProp = submitProposal({ | ||
| groupPolicyAddress: selectedAddress ?? '', | ||
| messages: [ | ||
| Any.fromPartial({ | ||
| typeUrl: MsgBurn.typeUrl, | ||
| value: MsgBurn.encode( | ||
| burn({ | ||
| amount: { | ||
| amount: amountInBaseUnits, | ||
| denom: denom.base, | ||
| }, | ||
| sender: selectedAddress ?? '', | ||
| burnFromAddress: recipient, | ||
| }).value | ||
| ).finish(), | ||
| }), | ||
| ], | ||
| metadata: '', | ||
| proposers: [address ?? ''], | ||
| title: `Multi Burn MFX`, | ||
| summary: `This proposal includes multiple burn actions for MFX.`, | ||
| proposers: [address], | ||
| title: `Burn Tokens`, | ||
| summary: `This proposal will burn ${amount} ${denom.display.split('/').pop()} from ${recipient}`, | ||
| exec: 0, | ||
| }); | ||
|
|
||
| const fee = await estimateFee(address ?? '', [msg]); | ||
| await tx([msg], { | ||
| const fee = await estimateFee(address ?? '', [isGroup ? burnProp : msg]); |
There was a problem hiding this comment.
Ensure 'selectedAddress' is defined before use
The selectedAddress is used with a fallback to an empty string in the proposal message for burning tokens. Using an undefined or empty selectedAddress may cause transaction failures. Ensure that selectedAddress is defined when isGroup is true to avoid invalid transactions.
There was a problem hiding this comment.
Thanks! Some comments @chalabi2
stores/index.tsneeds to contain something. I fixed it locally but...bun run build&bun run devfail with
⨯ /home/fmorency/dev/manifest-app/node_modules/react-syntax-highlighter/dist/esm/languages/prism/json.js:1
import json from "refractor/lang/json.js";
^^^^^^
SyntaxError: Cannot use import statement outside a module
at internalCompileFunction (node:internal/vm:73:18)
at wrapSafe (node:internal/modules/cjs/loader:1153:20)
at Module._compile (node:internal/modules/cjs/loader:1205:27)
at Module._extensions..js (node:internal/modules/cjs/loader:1295:10)
at Module.load (node:internal/modules/cjs/loader:1091:32)
at Module._load (node:internal/modules/cjs/loader:938:12)
at Module.require (node:internal/modules/cjs/loader:1115:19)
at mod.require (/home/fmorency/dev/manifest-app/node_modules/next/dist/server/require-hook.js:65:28)
at require (node:internal/modules/helpers:130:18)
at react-syntax-highlighter/dist/esm/languages/prism/json (/home/fmorency/dev/manifest-app/.next/server/pages/_app.js:2404:18)
at __webpack_require__ (/home/fmorency/dev/manifest-app/.next/server/webpack-runtime.js:33:42)
at eval (webpack-internal:///./components/groups/modals/voteDetailsModal.tsx:16:112)
at __webpack_require__.a (/home/fmorency/dev/manifest-app/.next/server/webpack-runtime.js:97:13)
at eval (webpack-internal:///./components/groups/modals/voteDetailsModal.tsx:1:21)
at ./components/groups/modals/voteDetailsModal.tsx (/home/fmorency/dev/manifest-app/.next/server/pages/_app.js:964:1)
I pushed a fix in 1b4a7e4 |
This PR allows the user to select a group in the token factory page and manage the token factory tokens for which the selected group is the authority. It includes updates for json syntax highlighting for message renders in the vote details modal and better state management / rendering for loading, error, and no token states in the token factory page. We also added a store for all the groups returned for the connected address and use that same data in both the group and token factory pages removing multiple queries and propagating the same data through the app.
Address / Group Selector
Group Token Display
Syntax Highlighting
Fixes #136 & #137
Summary by CodeRabbit
Release Notes
New Features
GithubIconcomponent for enhanced icon representation.GroupAddressProviderto manage group address state across the application.MyDenoms,BurnForm,MintForm,MintModal,BurnModal,TransferModal,UpdateDenomMetadataModal, and various modals to support group-based operations and improved user interactions.VoteDetailsModal.Bug Fixes
Documentation
Chores
package.jsonfor improved compatibility and performance.react-syntax-highlighterin the Next.js configuration.