feat: add foreign assets support on asset hubs, such as MYTH on PAH #1577
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
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: 36
🧹 Outside diff range and nitpick comments (18)
packages/extension-polkagate/src/fullscreen/accountDetails/components/AssetSelect.tsx (1)
Line range hint
24-25: Consider optimizing theoptionsuseMemo dependency arrayThe
useMemohook foroptionscurrently depends on bothassetsandtokens. However, since we're using the spread operator to create a new array, we could potentially optimize this by using the length of the arrays instead.Here's a suggested optimization:
- const options = useMemo(() => (tokens || []).concat(assets || []), [assets, tokens]); + const options = useMemo(() => (tokens || []).concat(assets || []), [assets?.length, tokens?.length]);This change ensures that the
optionsarray is only recalculated when the length ofassetsortokenschanges, potentially reducing unnecessary recalculations.packages/extension-polkagate/src/hooks/useTokenPrice.ts (2)
35-35: LGTM: Foreign asset handling added.The introduction of
isForeignAssetenhances the function's ability to handle different asset types.Consider improving readability by breaking the condition into multiple lines:
const isForeignAsset = isAssetHub && assetId && typeof assetId === 'string' && assetId.startsWith('0x');
Line range hint
1-61: Summary: Foreign asset support added with minor improvements needed.The changes successfully add support for foreign assets and improve the flexibility of the
useTokenPricefunction. The modifications are generally well-implemented, with clear logic for distinguishing between native and foreign assets. However, there's a potential type safety issue in thepriceIdcalculation that should be addressed. Additionally, some minor readability improvements could be made.Overall, the changes align well with the PR objective of adding foreign assets support on asset hubs.
Consider adding unit tests to verify the behavior of the function with different types of asset IDs, including foreign assets, to ensure the changes work as expected across various scenarios.
packages/extension-polkagate/src/fullscreen/sendFund/index.tsx (1)
53-53: Clarify the purpose ofassetIdchanges and foreign asset supportThe changes in this file consistently use
assetIdas a string instead of parsing it to an integer. While this aligns with the PR title mentioning foreign asset support, there's no explicit handling or mention of foreign assets in the changed code.Could you please clarify:
- How does keeping
assetIdas a string contribute to supporting foreign assets?- Are there any additional changes needed in this file or other components to fully support foreign assets?
- Have the
useBalanceshook andInputPagecomponent been updated to handle stringassetIdvalues correctly?Consider adding comments or documentation to explain the rationale behind these changes and how they relate to foreign asset support. This will help maintain clarity for future developers working on this code.
Also applies to: 108-108
packages/extension-polkagate/src/fullscreen/sendFund/Review.tsx (1)
35-36: LGTM: Usage ofuseInfohook.The replacement of separate
useApianduseChainhooks with the consolidateduseInfohook is a good simplification. The destructuring and usage ofapiandchainare correct.For improved readability, consider removing the empty line 36:
const { api, chain } = useInfo(address); -This small change will make the code more compact without affecting functionality.
packages/extension-polkagate/src/hooks/index.ts (2)
81-81: Consider grouping balance-related hooks togetherThe export of
usePoolBalancesis correctly implemented and follows the established pattern. However, for better organization and readability, consider grouping this hook with the other balance-related hooks (lines 24-26).Suggestion: Move this export to be grouped with the other balance-related hooks for improved code organization.
Line range hint
1-118: Consider future refactoring for improved maintainabilityThe new exports have been successfully added and are consistent with the existing pattern. They appear to support the PR objective of adding foreign assets support on asset hubs. However, as this file continues to grow, it may become harder to maintain.
For future consideration: As the number of hooks increases, it might be beneficial to organize them into categories and split this file into multiple smaller files (e.g.,
balanceHooks.ts,stakingHooks.ts, etc.). This could improve maintainability and make it easier to locate specific hooks.packages/extension-polkagate/src/partials/QuickActionFullScreen.tsx (2)
Line range hint
28-33: LGTM: QuickActionButtonType changed to interface.Changing
QuickActionButtonTypefrom a type alias to an interface is a good practice. It allows for future extension through declaration merging if needed.Consider adding a brief JSDoc comment to describe the purpose of this interface. This would enhance code readability and maintainability.
68-68: Consider adding a comment to explain showHistory values.The
goToHistoryfunction now setsshowHistoryto 1 instead of true, which is consistent with the earlier change to the state type.To improve code clarity, consider adding a comment explaining the possible values of
showHistoryand what they represent. This would make the code more self-documenting and easier to maintain.packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx (1)
26-26: LGTM! Consider adding a comment for clarity.The change to allow
assetIdto be of typenumber | string | undefinedis appropriate, especially considering the PR's objective to add foreign assets support. This modification provides the necessary flexibility to handle various asset ID formats.To improve code readability, consider adding a brief comment explaining why
assetIdcan be a string or number. For example:assetId: number | string | undefined; // Can be string for foreign assetsThis comment would help future developers understand the reasoning behind the flexible type.
packages/extension-polkagate/src/hooks/useBalancesOnMultiAssetChain.ts (1)
80-80: Add a null check forassetInfobefore proceeding.In the line:
assetInfo && api.query['tokens'] && fetchAssetOnMultiAssetChain(assetInfo).catch(console.error);If
assetInfoisundefined, the functionfetchAssetOnMultiAssetChainwill not be called. However, it might be clearer to explicitly handle the case whenassetInfois not found.Consider adding a warning or handling the
undefinedcase to improve code readability.packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (5)
12-12: Rename variable toisForeignAssetfor clarityThe variable
isForeignAssetssuggests plural but represents a Boolean value indicating whether a single asset is foreign. Renaming it toisForeignAssetwould improve readability and reduce confusion.Apply this diff to rename the variable:
- const isForeignAssets = asset.isForeign; + const isForeignAsset = asset.isForeign;
13-14: Update references to the renamed variableAfter renaming the variable, ensure that all subsequent references are updated accordingly.
Apply this diff:
- const section = isForeignAssets ? 'foreignAssets' : 'assets'; - const assetId = isForeignAssets ? decodeMultiLocation(asset.id) : asset.id; + const section = isForeignAsset ? 'foreignAssets' : 'assets'; + const assetId = isForeignAsset ? decodeMultiLocation(asset.id) : asset.id;
29-31: Use the renamed variableisForeignAssetin conditionUpdate the condition to use the renamed variable for consistency.
Apply this diff:
- const isFrozen = isForeignAssets + const isFrozen = isForeignAsset
48-48: Update object property toisForeignAssetFor consistency with the variable name change, update the property name in the object to
isForeignAsset.Apply this diff:
- isForeignAssets: !!isForeignAssets, + isForeignAsset: !!isForeignAsset,Note: Ensure that any code that depends on this property name is also updated accordingly.
76-85: Remove commented-out code to clean up the codebaseThe large block of commented-out code from lines 76 to 85 can be removed to improve readability and maintain a clean codebase. If this code might be needed later, consider retrieving it from version control when necessary.
Apply this diff to remove the commented code:
- /** to calculate a new Foreign Token like MYTH asset id based on its XCM multi-location */ - // const allForeignAssets = await api.query.foreignAssets.asset.entries(); - // for (const [key, _others] of allForeignAssets) { - // const id = key.args[0]; - // const assetMetaData = await api.query.foreignAssets.metadata(id); - // - // if (assetMetaData.toHuman().symbol === 'MYTH') { - // console.log('new foreign asset id:', encodeMultiLocation(id)); - // } - // }packages/extension-polkagate/src/hooks/usePoolBalances.ts (1)
7-7: Consider addressing the underlying TypeScript issues instead of using//@ts-ignoreUsing
//@ts-ignoresuppresses TypeScript errors, which might hide potential problems in the code. It's better to resolve the TypeScript issues or, if necessary, use more specific directives like@ts-expect-errorto indicate intentional exceptions.packages/extension-polkagate/src/util/utils.ts (1)
503-533: Refine type annotations for better type safetyThe functions
encodeMultiLocation,decodeHexValues, anddecodeMultiLocationuseunknownas type annotations for parameters and return types. Using more specific types can enhance type safety, catch potential errors at compile time, and improve code readability.Consider defining specific interfaces or types for
multiLocationand the expected structures of the objects being processed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
- package.json (1 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AssetSelect.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/index.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (9 hunks)
- packages/extension-polkagate/src/fullscreen/sendFund/Review.tsx (2 hunks)
- packages/extension-polkagate/src/fullscreen/sendFund/index.tsx (2 hunks)
- packages/extension-polkagate/src/hooks/index.ts (2 hunks)
- packages/extension-polkagate/src/hooks/useAssetsBalances.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useBalances.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useBalancesOnAssethub.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useBalancesOnMultiAssetChain.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useBalancesOnSingleAssetChain.ts (1 hunks)
- packages/extension-polkagate/src/hooks/usePoolBalances.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useTokenPrice.ts (3 hunks)
- packages/extension-polkagate/src/partials/QuickActionFullScreen.tsx (4 hunks)
- packages/extension-polkagate/src/util/utils.ts (2 hunks)
- packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (3 hunks)
🧰 Additional context used
🪛 Biome
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx
[error] 103-103: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/extension-polkagate/src/hooks/useBalancesOnSingleAssetChain.ts
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/extension-polkagate/src/hooks/usePoolBalances.ts
[error] 70-70: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (30)
packages/extension-polkagate/src/fullscreen/accountDetails/components/AssetSelect.tsx (1)
Line range hint
1-53: Summary of review
- The change to allow
assetIdto be a string improves flexibility.- Consider updating the
setAssetIdprop type for consistency.- A minor optimization for the
optionsuseMemo hook has been suggested.Overall, the changes look good and improve the component's flexibility. The suggested improvements are minor and aimed at maintaining consistency and optimizing performance.
packages/extension-polkagate/src/hooks/useTokenPrice.ts (2)
28-28: LGTM: Function signature update enhances flexibility.The change to accept
assetIdas eithernumberorstringimproves the function's versatility in handling different asset ID formats.
60-60: LGTM: useMemo dependencies updated correctly.The addition of
isForeignAssetto the dependency array ensures that the memoized value is recalculated when the foreign asset status changes, maintaining consistency with the function's logic.package.json (2)
Line range hint
1-138: Clarify if additional changes are required for the PR objectives.The PR objectives mention adding foreign assets support on asset hubs, such as MYTH on PAH. However, the only change in this file is updating the
@polkagate/apps-configpackage version. It's unclear if this single change is sufficient to implement the described feature.To ensure all necessary changes have been made, please run the following verification steps:
#!/bin/bash # Description: Check for other relevant changes in the PR # Test: List all changed files in this PR git diff --name-only origin/master...HEAD # Test: Search for files that might be related to asset hubs or foreign assets git diff --name-only origin/master...HEAD | grep -iE "asset|hub|foreign|MYTH|PAH" # Test: Check for any new dependencies added that might be related to the feature git diff origin/master...HEAD package.json | grep "+"Please review the results of these verification steps and clarify:
- Are there other files changed in this PR that implement the foreign assets support?
- If not, is the update to
@polkagate/apps-configsufficient to add this feature?- Are there any new dependencies or configuration changes required for supporting foreign assets on asset hubs?
Consider providing more context about how the
@polkagate/apps-configupdate enables foreign assets support. If additional changes are needed, ensure they are included in the PR or explain why they are not necessary.
70-70: Verify the necessity and impact of the dependency update.The version of
@polkagate/apps-confighas been updated from^0.140.3to^0.140.5. This minor version bump suggests that new features or non-breaking changes have been introduced in the package.To ensure this update aligns with the PR objectives and doesn't introduce any unintended consequences, please run the following verification steps:
Please review the results of these verification steps to confirm that:
- The package update is directly related to adding foreign assets support.
- There are no breaking changes that could affect the existing functionality.
- The update includes the necessary changes for supporting MYTH on PAH.
packages/extension-polkagate/src/fullscreen/sendFund/index.tsx (2)
108-108: VerifyInputPagecomponent implementation forassetIdprop handlingThe
assetIdis now passed directly to theInputPagecomponent without parsing it to an integer. This change is consistent with the modification in theuseBalanceshook call, but it could potentially cause issues if the component expects an integer value forassetId.To ensure compatibility, please run the following script to check the
InputPagecomponent implementation:#!/bin/bash # Description: Verify the implementation of InputPage component # Test: Search for the InputPage component definition echo "Searching for InputPage component definition:" rg --type typescript -A 20 "export default function InputPage" # Test: Check how assetId prop is used within the InputPage component echo "Checking assetId prop usage in InputPage component:" rg --type typescript -A 5 "assetId.*InputPage"If the
InputPagecomponent expects a stringassetId, this change is correct. Otherwise, consider parsing theassetIdto an integer before passing it to the component.
53-53: VerifyuseBalanceshook implementation forassetIdhandlingThe
assetIdis now passed directly to theuseBalanceshook without parsing it to an integer. This change could potentially cause issues if the hook expects an integer value forassetId.To ensure compatibility, please run the following script to check the
useBalanceshook implementation:If the
useBalanceshook expects a stringassetId, this change is correct. Otherwise, consider parsing theassetIdto an integer before passing it to the hook.✅ Verification successful
useBalanceshook correctly handlesassetIdas string or numberThe
useBalanceshook acceptsassetIdas either a string or a number, ensuring compatibility with the updated usage. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of useBalances hook # Test: Search for the useBalances hook definition echo "Searching for useBalances hook definition:" rg --type typescript -A 10 "export function useBalances" # Test: Check how assetId is used within the useBalances hook echo "Checking assetId usage in useBalances hook:" rg --type typescript -A 5 "assetId.*useBalances"Length of output: 397
Script:
#!/bin/bash # Description: Verify the implementation of useBalances hook using file extensions # Test: Search for the useBalances hook definition echo "Searching for useBalances hook definition in .ts and .tsx files:" rg --glob "*.ts" --glob "*.tsx" -A 10 "export function useBalances" # Test: Check how assetId is used within the useBalances hook echo "Checking assetId usage in useBalances hook in .ts and .tsx files:" rg --glob "*.ts" --glob "*.tsx" -A 5 "assetId.*useBalances"Length of output: 411
Script:
#!/bin/bash # Description: Comprehensive search for useBalances hook implementation # Test 1: Search for any function named useBalances using ast-grep echo "Searching for any function named useBalances using ast-grep:" ast-grep --lang typescript --pattern $'function useBalances($_) { $$$ }' # Test 2: Search for useBalances regardless of context using ripgrep echo "Searching for all occurrences of useBalances in .ts and .tsx files:" rg --glob "*.ts" --glob "*.tsx" "useBalances" # Test 3: Search for useBalances as part of an exported object using ripgrep echo "Searching for useBalances within exported objects in .ts and .tsx files:" rg --glob "*.ts" --glob "*.tsx" "export\s+\{[^}]*useBalances[^}]*\}"Length of output: 10961
packages/extension-polkagate/src/fullscreen/sendFund/Review.tsx (2)
13-13: LGTM: Import statement foruseInfohook.The import of the
useInfohook is correctly placed and aligns with the changes described in the summary. This consolidation of API and chain information retrieval should simplify the component's data fetching process.
Line range hint
1-159: Summary: Successful integration ofuseInfohook.The changes made to this file effectively integrate the new
useInfohook, consolidating the retrieval of API and chain information. This modification aligns well with the PR objectives of adding foreign assets support on asset hubs, such as MYTH on PAH.Key points:
- The
useInfohook replaces separateuseApianduseChainhooks, simplifying data fetching.- The overall structure and functionality of the
Reviewcomponent remain intact.- These changes should improve code maintainability without introducing breaking changes.
The implementation looks solid, and the changes are minimal and focused. Great job on keeping the modifications concise and effective!
packages/extension-polkagate/src/hooks/index.ts (3)
24-24: LGTM: New hook export follows consistent patternThe export of
useBalancesOnAssethubis correctly implemented and follows the established pattern in this file. The naming convention is appropriate and suggests its purpose clearly.
25-25: LGTM: Consistent implementation of new hook exportThe export of
useBalancesOnMultiAssetChainis correctly implemented, maintaining consistency with the file's established pattern. The name clearly indicates its purpose for handling balances on multi-asset chains.
26-26: LGTM: New hook export complements existing functionalityThe export of
useBalancesOnSingleAssetChainis correctly implemented and consistent with the file's pattern. It complements the previously addeduseBalancesOnMultiAssetChain, providing a comprehensive set of hooks for different chain types.packages/extension-polkagate/src/partials/QuickActionFullScreen.tsx (3)
6-7: LGTM: Import of AccountId type.The import of
AccountIdfrom '@polkadot/types/interfaces/runtime' is a good addition. It enhances type safety in the component.
25-25: LGTM: Props interface update for assetId.The change in the
assetIdprop type tonumber | string | undefinedprovides more flexibility. This aligns well with the PR objective of adding foreign assets support.Could you please clarify the scenarios where
assetIdmight be a string? This information would be helpful for future maintainers.
44-44: Please clarify the purpose of changing showHistory to a number.The
showHistorystate has been changed from a boolean to a number type. While this change allows for more nuanced state management, the purpose isn't immediately clear from the context.Could you please explain the reasoning behind this change? How does using a number instead of a boolean enhance the functionality of the component?
packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx (1)
26-26: Verify usage of CommonTasks component across the codebaseWhile the change is appropriate and doesn't introduce issues within this component, it's important to ensure that all usages of the
CommonTaskscomponent across the codebase are compatible with the newassetIdtype.Please run the following script to identify all usages of the
CommonTaskscomponent and verify that they handle the possibility ofassetIdbeing a string:Review the results to ensure that all locations where
CommonTasksis used are prepared to handleassetIdas either a number or a string.packages/extension-polkagate/src/hooks/useAssetsBalances.ts (2)
77-77: Verify impact on other files usingFetchedBalanceWhile the change to
assetIdtype is appropriate, there are no direct usages ofassetIdin this file. To ensure full compatibility:
- Verify that all files importing and using the
FetchedBalanceinterface are updated to handle bothnumberandstringtypes forassetId.- Update any type assertions or comparisons involving
assetIdin other files to account for the new union type.To help identify potentially affected files:
#!/bin/bash # Search for files importing FetchedBalance rg --type typescript 'import.*FetchedBalance' packages/extension-polkagate/src # Search for usages of assetId in typescript files rg --type typescript 'assetId'
77-77: LGTM: ExpandedassetIdtype to support foreign assetsThe change from
numbertonumber | stringfor theassetIdproperty in theFetchedBalanceinterface allows for greater flexibility in asset identification. This aligns with the PR objective of adding foreign assets support.To ensure consistency, let's verify the usage of
assetIdthroughout the file:✅ Verification successful
Verified:
assetIdType Change is ConsistentThe
assetIdproperty's type change fromnumbertonumber | stringin theFetchedBalanceinterface has been verified. No additional usages found inuseAssetsBalances.tsthat could be affected by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of assetId in the file rg --type typescript 'assetId' packages/extension-polkagate/src/hooks/useAssetsBalances.tsLength of output: 181
Script:
#!/bin/bash # Search for usages of assetId in TypeScript files using file extensions rg 'assetId' --glob '*.ts' --glob '*.tsx' packages/extension-polkagate/src/hooks/useAssetsBalances.tsLength of output: 150
packages/extension-polkagate/src/hooks/useBalancesOnMultiAssetChain.ts (1)
53-54:⚠️ Potential issueEnsure correct type handling when accessing
currencyId.In the line:
const currencyId = (found[0].toHuman() as string[])[1];
found[0].toHuman()may not return an array of strings. Using(found[0].toHuman() as string[])[1]assumes a specific structure that might not be accurate. This can lead to runtime errors if the structure differs.Consider verifying the structure of
found[0].toHuman()and adjust the code accordingly. You might need to inspect the actual output or refer to the library documentation.Run the following script to inspect the output structure of
toHuman():packages/extension-polkagate/src/hooks/useBalances.ts (4)
13-13: UpdatedassetIdparameter type for enhanced flexibilityThe
assetIdparameter type has been changed fromnumbertostring | number, allowing for greater flexibility in handling different asset ID formats.
17-22: Effective integration of new balance-fetching hooksThe use of
useBalancesOnSingleAssetChain,useBalancesOnAssethub, anduseBalancesOnMultiAssetChainhooks consolidates balance retrieval logic, improving modularity and code maintainability. This refactoring enhances the separation of concerns within the balance-fetching operations.
Line range hint
74-76: Correctly returningassetBalancewhenassetIdis providedThe condition to return
assetBalancewhenassetIdis defined ensures that balances specific to the given asset are accurately returned. This logic correctly handles cases where foreign assets are involved.
Line range hint
78-82: Ensuring consistency in balance returnsThe return logic prioritizes the
overallbalances when they match the current chain'sgenesisHash,token, anddecimal. If not, it falls back tobalanceswith matchingtokenanddecimal. This ensures that the hook returns balances that are consistent with the current chain context.packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (6)
9-9: ImportingHexStringtype is appropriateThe addition of
HexStringimport from@polkadot/util/typesis necessary for proper type annotations when handling hexadecimal strings, as used indecodeMultiLocation.
26-26: Importing utility functions for asset handlingThe import of
amountToHuman,amountToMachine, anddecodeMultiLocationfrom../../util/utilsaligns with their usage in the code for processing amounts and decoding asset IDs.
34-34: UpdatedassetIdtype tostring | undefinedChanging the
assetIdtype in thePropsinterface fromnumber | undefinedtostring | undefinedreflects the need to handle asset IDs that are strings, such as hexadecimal IDs for foreign assets. Ensure that all usages ofassetIdhave been updated accordingly and that there are no type conflicts or unintended side effects due to this change.
241-245: Ensure correct parameters for fee calculationIn the
calculateFeefunction, verify that the parameters passed toonChainCall(..._params)match the expected arguments for the specific module and function used. Particularly, ensure thatparsedAssetIdis of the correct type and format for the selected module.
Line range hint
308-317: Validate parameters for input settingWhen setting the inputs in the
useEffect, confirm that the constructedparamsarray aligns with the expected input of the on-chain call, and thatparsedAssetId,recipientAddress, andamountAsBNare correctly formatted and validated.
423-423: Display existential deposit using human-readable formatThe change to display the existential deposit using
amountToHumanenhances user experience by showing the amount in a readable format.packages/extension-polkagate/src/hooks/useBalancesOnSingleAssetChain.ts (1)
127-127: Verify the type casting ofstakingAccount?.stakingLedger?.totalThe expression
stakingAccount?.stakingLedger?.total as unknown as BNinvolves casting throughunknown, which might indicate a type mismatch.Please ensure that
stakingLedger.totalis correctly typed asBNto avoid unnecessary casting. Adjust the types if needed to maintain type safety.
packages/extension-polkagate/src/fullscreen/accountDetails/components/AssetSelect.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/accountDetails/index.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/hooks/useBalancesOnMultiAssetChain.ts
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/hooks/useBalancesOnSingleAssetChain.ts
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/hooks/useBalancesOnSingleAssetChain.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (2)
308-308: LGTM: Comprehensive input handling with suggestionThe updates to the
useEffecthook for setting inputs are well-aligned with the new asset ID handling and support for foreign assets. The logic comprehensively covers different scenarios for various asset types and transfer situations.To improve readability, consider extracting the complex condition for determining
paramsinto a separate function:const getParams = () => { if (isCrossChain) return crossChainParams; if (assetId !== undefined) { return ['currencies', 'tokens'].includes(onChainCall?.section || '') ? [recipientAddress, balances.currencyId, amountAsBN] : [parsedAssetId, recipientAddress, amountAsBN]; } return transferType === 'All' ? [recipientAddress, false] : [recipientAddress, amountAsBN]; }; // Then in the setInputs call: params: getParams() as unknown[],This refactoring would make the
useEffecthook more readable and easier to maintain.Also applies to: 317-317
359-359: LGTM: Improved whole amount calculation with suggestionThe updates to the
setWholeAmountfunction enhance the handling of different asset types and provide a more accurate representation of the transferable amount. The use ofamountToHumanimproves user readability of the amount.Consider adding a safeguard against potential negative values when subtracting
_maxFeefromtransferableBalance:const allAmount = canNotTransfer ? '0' : amountToHuman(BN.max(transferableBalance.sub(_maxFee), BN_ZERO).toString(), balances.decimal);This change ensures that even in edge cases where
_maxFeemight be greater thantransferableBalance, the result will never be negative.Also applies to: 365-365, 369-370, 373-373
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountInformationForDetails.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AssetSelect.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/index.tsx (2 hunks)
- packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (9 hunks)
- packages/extension-polkagate/src/hooks/useBalancesOnAssethub.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AssetSelect.tsx
- packages/extension-polkagate/src/fullscreen/accountDetails/index.tsx
- packages/extension-polkagate/src/hooks/useBalancesOnAssethub.ts
🔇 Additional comments (4)
packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountInformationForDetails.tsx (1)
147-147: LGTM. Verify usage ofsetAssetIdOnAssetHubthroughout the codebase.The change to allow string values for asset IDs aligns with the PR objective of adding foreign assets support. This modification provides more flexibility in handling different types of asset identifiers.
To ensure consistency, please verify that all usages of
setAssetIdOnAssetHubthroughout the codebase can handle string asset IDs. Run the following script to check its usage:Review the results to ensure that all occurrences of
setAssetIdOnAssetHuband related asset ID handling are compatible with the new type definition.✅ Verification successful
LGTM. Verify all usages of
setAssetIdOnAssetHubfor type compatibility.The change to allow string values for asset IDs has been consistently implemented. However, to ensure full compatibility across the codebase:
- Review all instances where
setAssetIdOnAssetHubis called withas numbertype assertions.- Verify that any logic depending on the type of the asset ID can handle string values.
- Check if any additional type guards or conversions are needed in functions that use the asset ID.
This change aligns with the PR objective of adding foreign assets support, providing more flexibility in handling different types of asset identifiers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of setAssetIdOnAssetHub in the codebase # Search for setAssetIdOnAssetHub usage echo "Usages of setAssetIdOnAssetHub:" rg "setAssetIdOnAssetHub" --type typescript --type tsx -A 5 -B 5 # Search for potential type assertions or checks related to asset IDs echo "\nPotential type assertions or checks for asset IDs:" rg "as (number|string)" --type typescript --type tsx -A 3 -B 3 rg "typeof.*===.*(\"number\"|\"string\")" --type typescript --type tsx -A 3 -B 3Length of output: 501
Script:
#!/bin/bash # Description: Check usage of setAssetIdOnAssetHub in the codebase # Search for setAssetIdOnAssetHub usage echo "Usages of setAssetIdOnAssetHub:" rg "setAssetIdOnAssetHub" --type ts --type tsx -A 5 -B 5 # Search for potential type assertions or checks related to asset IDs echo "\nPotential type assertions or checks for asset IDs:" rg "as (number|string)" --type ts --type tsx -A 3 -B 3 rg "typeof.*===.*(\"number\"|\"string\")" --type ts --type tsx -A 3 -B 3Length of output: 456
Script:
#!/bin/bash # Description: Check usage of setAssetIdOnAssetHub in the codebase # Search for setAssetIdOnAssetHub usage echo "Usages of setAssetIdOnAssetHub:" rg "setAssetIdOnAssetHub" -g '*.ts' -g '*.tsx' -A 5 -B 5 # Search for potential type assertions or checks related to asset IDs echo "\nPotential type assertions or checks for asset IDs:" rg "as (number|string)" -g '*.ts' -g '*.tsx' -A 3 -B 3 rg "typeof.*===.*(\"number\"|\"string\")" -g '*.ts' -g '*.tsx' -A 3 -B 3Length of output: 280471
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (3)
9-9: LGTM: Enhanced asset ID handlingThe changes to import
HexStringand update theassetIdtype in thePropsinterface fromnumber | undefinedtostring | undefinedindicate improved support for various asset ID formats, including hexadecimal strings. This enhancement allows for more flexible asset identification across different chains and standards.Also applies to: 34-34
241-241: LGTM: Consistent fee calculation with new asset ID handlingThe updates to the
calculateFeefunction, particularly the use ofparsedAssetIdinstead ofassetId, align well with the new asset ID handling introduced earlier in the file. This change ensures consistency in how asset IDs are processed throughout the component.Also applies to: 245-245
423-423: LGTM: Enhanced readability of Existential Deposit displayThe update to use
amountToHumanfor displaying the Existential Deposit improves the user experience by presenting the amount in a more readable format. This change is consistent with the overall improvements in amount handling and display throughout the component.
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
packages/extension-polkagate/src/hooks/useMyVote.ts (1)
Line range hint
1-43: Consider enhancing error handling and optimizing performanceWhile the current implementation is functional, consider the following suggestions for improvement:
Error Handling: The catch block in
fetchVoteonly logs the error to the console. Consider implementing a more robust error handling mechanism, such as setting an error state that can be used to display an error message to the user.Performance Optimization: The hook uses two separate
useEffecthooks for fetching the vote. Consider combining these into a singleuseEffectwith a combined dependency array to reduce the number of potential re-renders.Type Safety: The file has a
@ts-nocheckcomment at the top. Consider removing this and properly typing the function parameters and return values to improve type safety and catch potential issues at compile-time.Example of combining the
useEffecthooks:useEffect(() => { if (refresh) { fetchVote().catch(console.error); } }, [fetchVote, refresh]);Would you like me to provide a more detailed refactoring suggestion for these improvements?
packages/extension-polkagate/src/hooks/useAssetHubAssets.ts (2)
23-23: Simplify condition using optional chainingThe condition in line 23 can be simplified by using optional chaining to enhance code readability.
Apply this diff to simplify the condition:
-if (!(api?.query[assetType] && api?.query[assetType]['asset'])) { +if (!api?.query[assetType]?.['asset']) {🧰 Tools
🪛 Biome
[error] 23-23: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
40-40: Avoid unnecessary type assertions in 'assetIndex' assignmentThe use of
'as unknown as string'can be avoided by properly handling the types. This improves code maintainability and reduces potential runtime errors.Modify the assignment as follows:
-const assetIndex = assetType === 'assets' ? assetIds[index] as unknown as string : encodeMultiLocation(assetIds[index]) as string; +const assetIndex = assetType === 'assets' + ? assetIds[index].toString() + : encodeMultiLocation(assetIds[index]) as string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountInformationForDetails.tsx (2 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/index.tsx (3 hunks)
- packages/extension-polkagate/src/hooks/useAssetHubAssets.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useBalancesOnAssethub.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useBalancesOnMultiAssetChain.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useBalancesOnSingleAssetChain.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useMyVote.ts (1 hunks)
- packages/extension-polkagate/src/hooks/usePoolBalances.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useStakingAccount.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useTokenPrice.ts (3 hunks)
- packages/extension-polkagate/src/util/utils.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountInformationForDetails.tsx
- packages/extension-polkagate/src/fullscreen/accountDetails/index.tsx
- packages/extension-polkagate/src/hooks/useBalancesOnAssethub.ts
- packages/extension-polkagate/src/hooks/useBalancesOnMultiAssetChain.ts
- packages/extension-polkagate/src/hooks/useBalancesOnSingleAssetChain.ts
- packages/extension-polkagate/src/hooks/usePoolBalances.ts
- packages/extension-polkagate/src/hooks/useTokenPrice.ts
🧰 Additional context used
🪛 Biome
packages/extension-polkagate/src/hooks/useAssetHubAssets.ts
[error] 23-23: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
packages/extension-polkagate/src/hooks/useMyVote.ts (1)
27-27: Improved null safety with optional chainingThe change from
setRefresh && setRefresh(false);tosetRefresh?.(false);is a good improvement. It uses the optional chaining operator (?.) which provides a more concise and idiomatic way to safely call thesetRefreshfunction only if it's defined.This change maintains the existing functionality while slightly improving code readability and adhering to modern JavaScript best practices.
packages/extension-polkagate/src/hooks/useStakingAccount.ts (2)
73-73: Approve: Improved syntax for conditional function callThe change from
refresh && setRefresh && setRefresh(false);torefresh && setRefresh?.(false);is a good improvement. It uses the optional chaining operator?.to achieve the same result more concisely. This modern JavaScript feature enhances code readability and maintains the same behavior while reducing the likelihood of errors when dealing with potentially undefined values.
Line range hint
1-173: Summary: Minimal impact, improved code qualityThe change in the
fetchfunction is a localized improvement that doesn't alter the overall behavior of theuseStakingAccounthook. It maintains the existing functionality while adopting a more modern and concise syntax. This update aligns with best practices in JavaScript development and slightly improves code readability without introducing any risks or necessitating changes in other parts of the file.
… other chains (#1580) * refactor: set different asset ids for native assets on asset hubs vs. other chains * fix: native token fetch * chore: replace mayBe with maybe * fix: correct path * fix: to show test net asset hubs on home screen * refactor: fix type issues
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (37)
packages/extension-polkagate/src/util/workers/utils/toGetNativeToken.js (1)
23-26: LGTM: Function changes correctly implement asset hub support.The modifications to
toGetNativeTokenfunction effectively implement support for foreign assets on asset hubs:
- The
isAssetHubcheck correctly determines if the current chain is an asset hub.- The conditional assignment of
assetIduses the appropriate constant based on whether it's an asset hub or not.These changes align well with the PR objectives.
Consider extracting the
isAssetHubcheck into a separate utility function for better reusability and easier testing. For example:const isAssetHub = (genesisHash) => ASSET_HUBS.includes(genesisHash.toString()); // Usage in the function const isCurrentChainAssetHub = isAssetHub(api.genesisHash);This would make the code more modular and easier to maintain if the logic for determining asset hubs changes in the future.
packages/extension-polkagate/src/util/getLogo.ts (1)
37-37: Improved variable naming conventionThe renaming of
mayBeExternalLogotomaybeExternalLogoenhances code readability by correctly using camelCase and improving grammatical accuracy. This change maintains the existing functionality while making the code more consistent with standard naming conventions.packages/extension-polkagate/src/util/getLogo2.ts (2)
44-44: Approved: Improved variable namingThe renaming of
mayBeExternalLogotomaybeExternalLogois a good change. It corrects the spelling and improves code readability without affecting the functionality.
Line range hint
1-60: Summary: Minor improvements, no direct impact on PR objectiveThe changes in this file are limited to a variable renaming from
mayBeExternalLogotomaybeExternalLogo. While these changes improve code readability and consistency, they don't directly contribute to the PR's objective of adding foreign assets support on asset hubs.However, maintaining clean and consistent code is important for the overall health of the project. These changes, although minor, align with good coding practices.
Consider reviewing other parts of the codebase where the foreign assets support is actually implemented, as that would be more relevant to the PR's main objective.
packages/extension-polkagate/src/components/ChainLogo.tsx (1)
25-25: Approved: Good catch on the spelling correction.The renaming of
mayBeUserAddedChainColortomaybeUserAddedChainColorimproves code readability and consistency. It's a good practice to use correct spelling in variable names.Consider using camelCase for the entire variable name to follow JavaScript naming conventions:
-const maybeUserAddedChainColor = useUserAddedChainColor(genesisHash); +const maybeUserAddedChainColor = useUserAddedChainColor(genesisHash);This change is optional and doesn't affect functionality, but it would align better with common JavaScript style guidelines.
Also applies to: 63-63
packages/extension-polkagate/src/popup/history/Explorer.tsx (1)
20-20: Approved: Spelling correction improves code readability.The change from
mayBeTheFirstPartOfChaintomaybeTheFirstPartOfChainis a good correction that improves code readability and adheres to common naming conventions. The functionality remains unchanged, and the correction is consistently applied across all occurrences.Consider using camelCase for the entire variable name for even better readability:
-const maybeTheFirstPartOfChain = chainName?.split(' ')?.[0]; +const maybeFirstPartOfChain = chainName?.split(' ')?.[0];This would make the variable name fully adhere to JavaScript naming conventions.
Also applies to: 29-29, 32-32
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (3)
6-10: LGTM! Consider adding JSDoc for the new function.The new import and function declaration look good. The
getAssetsfunction seems to be a well-structured helper for processing both foreign and native assets.Consider adding a JSDoc comment for the
getAssetsfunction to describe its purpose, parameters, and return value. This would improve code documentation and maintainability.
Line range hint
11-59: LGTM! Consider adding error handling.The main logic of the
getAssetsfunction is well-structured and handles both foreign and native assets appropriately. The balance processing, including consideration for frozen assets, is a good touch.Consider adding try-catch blocks around the API calls and data processing to handle potential errors gracefully. This would improve the robustness of the function, especially when dealing with network requests.
63-87: LGTM! Consider documenting the commented-out block.The modifications to
getAssetOnAssetHubimprove the function's structure and leverage the newgetAssetshelper effectively. This refactoring enhances code organization and maintainability.For the commented-out block (lines 76-85), consider adding a brief comment explaining its purpose and why it's currently disabled. This would help other developers understand its potential future use without having to decipher the code.
packages/extension-polkagate/src/util/workers/getAssetOnRelayChain.js (1)
101-101: LGTM! Consider adding a comment for clarity.The change to explicitly use
NATIVE_TOKEN_ASSET_IDfor theassetIdproperty is good for consistency and readability. It aligns well with the comment above about supporting multiple assets per chain.To further improve clarity, consider adding a brief comment explaining why
NATIVE_TOKEN_ASSET_IDis used here. For example:assetId: NATIVE_TOKEN_ASSET_ID, // Use NATIVE_TOKEN_ASSET_ID for the relay chain's native tokenThis would help future developers understand the reasoning behind this specific asset ID.
packages/extension-polkagate/src/fullscreen/sendFund/Confirmation.tsx (1)
Line range hint
29-41: LGTM with a suggestion for additional null checksThe changes to make the
infoprop optional and the use of optional chaining are good improvements. They add flexibility and prevent potential runtime errors.However, consider adding null checks or default values to ensure the component behaves correctly when
infois undefined. For example:const accountName = useAccountDisplay(info?.address ?? '');This ensures that even if
infois undefined, a valid empty string is passed touseAccountDisplay.packages/extension-polkagate/src/hooks/useAccountLocks.ts (1)
144-144: Approve naming convention improvement.The change from
mayBePriorstomaybePriorsimproves consistency with camelCase naming conventions. This is a good practice for maintaining code readability.For even better consistency, consider using
maybePriorsthroughout the codebase. You can use the following command to check for any remaining instances ofmayBePriors:rg --type typescript 'mayBePriors'packages/extension-polkagate/src/popup/staking/solo/fastUnstake/index.tsx (1)
45-45: Improved variable naming convention.The change from
mayBeMyStashBalancestomaybeMyStashBalancesfollows the camelCase convention more accurately, enhancing code readability and consistency.packages/extension-polkagate/src/popup/staking/partial/ShowPool.tsx (1)
53-54: LGTM! Consider adding a comment for clarity.The variable renaming from
mayBeCommissiontomaybeCommissionimproves readability and follows camelCase convention. The logic for calculatingmaybeCommissionandcommissionis correct and unchanged.Consider adding a brief comment explaining the purpose of dividing by 10^7 in the
commissioncalculation. This would improve code maintainability. For example:// Convert commission from parts per billion to percentage const commission = Number(maybeCommission) / (10 ** 7) < 1 ? 0 : Number(maybeCommission) / (10 ** 7);packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx (1)
44-44: Approved: Updated calculation with renamed variable.The calculation has been correctly updated to use the renamed
maybeCommissionvariable. The logic remains unchanged and correct.For improved readability, consider extracting the calculation into a separate function or adding a comment explaining the conversion. For example:
// Convert commission to percentage (minimum 0%) const commissionValue = Math.max(0, Number(maybeCommission) / (10 ** 7));packages/extension-polkagate/src/fullscreen/governance/utils/util.ts (1)
157-172: Approved: Improved naming consistency inblockToXfunction.The changes enhance code readability by adopting a more consistent camelCase naming convention for variables. This improves the overall quality of the code without altering its functionality.
For complete consistency, consider updating the function name from
blockToXtoblockToTimeorblockToTimeStringto better reflect its purpose.packages/extension-polkagate/src/util/constants.tsx (1)
14-15: LGTM! Consider adding a type annotation for clarity.The addition of
NATIVE_TOKEN_ASSET_ID_ON_ASSETHUBand the clarification forNATIVE_TOKEN_ASSET_IDimprove the code's clarity and maintainability.Consider adding type annotations to these constants for additional clarity:
export const NATIVE_TOKEN_ASSET_ID: number = 0; // used for non asset hub chains export const NATIVE_TOKEN_ASSET_ID_ON_ASSETHUB: number = -1; // used only for asset hubspackages/extension-polkagate/src/fullscreen/governance/post/Metadata.tsx (3)
Line range hint
83-91: Approved: Variable renaming improves readability.The change from
mayBeBeneficiarytomaybeBeneficiaryis a good improvement in terms of spelling and readability. The logic within theuseMemohook remains correct and unchanged.Consider adding a comment explaining the purpose of this memoized value for better code documentation:
// Memoize the beneficiary calculation to avoid unnecessary recalculations const maybeBeneficiary = useMemo(() => { // ... (existing code) }, [chain, referendum?.call]);
190-206: Approved: Consistent variable renaming and correct logic.The change from
mayBeBeneficiarytomaybeBeneficiaryis consistent with the earlier renaming. The logic for rendering the beneficiary information, including address validation and component selection, remains correct and unchanged.Consider extracting the conditional rendering logic into a separate function to improve readability:
const renderBeneficiary = (beneficiary: string) => { if (isValidAddress(beneficiary)) { return ( <Identity api={api} chain={chain} formatted={beneficiary} identiconSize={25} showShortAddress showSocial style={{ fontSize: '16px', fontWeight: 500, maxWidth: '100%', minWidth: '35%' }} /> ); } return <ShowValue value={beneficiary} />; }; // Usage in JSX value={renderBeneficiary(maybeBeneficiary)}This would make the JSX more concise and easier to read.
Line range hint
1-283: Summary: Improved readability through consistent variable renaming.The changes in this file primarily involve renaming the
mayBeBeneficiaryvariable tomaybeBeneficiarythroughout the component. This improves code readability and consistency without altering the component's functionality or overall structure. TheMetadatacomponent continues to render referendum metadata correctly, including beneficiary information when available.Consider the following suggestions for future improvements:
- Add comments explaining the purpose of complex logic, such as the
maybeBeneficiarycalculation.- Extract reusable rendering logic into separate functions to enhance code organization and readability.
- Consider adding unit tests for the
getBeneficiaryfunction and theMetadatacomponent to ensure continued correctness as the codebase evolves.packages/extension-polkagate/src/popup/staking/pool/myPool/editPool/index.tsx (2)
54-55: Approved: Variable renaming improves readability.The renaming of
mayBeCommissiontomaybeCommissionenhances code clarity. Consider adding a comment explaining the10 ** 7division in thecommissionValuecalculation for better maintainability.const maybeCommission = ((pool?.bondedPool?.commission?.current as any)?.[0] || 0) as number; - const commissionValue = Number(maybeCommission) / (10 ** 7) < 1 ? 0 : Number(maybeCommission) / (10 ** 7); + // Convert commission from parts per billion to percentage + const commissionValue = Number(maybeCommission) / (10 ** 7) < 1 ? 0 : Number(maybeCommission) / (10 ** 7);
Line range hint
70-80: Consider optimizing the initialization useEffect hook.The current implementation of the initialization
useEffecthook can be improved for better readability and performance. Consider using object destructuring and a singlesetInitialStatefunction to update all state variables at once.Here's a suggested refactoring:
useEffect(() => { const { metadata, bondedPool } = pool || {}; const { roles, commission } = bondedPool || {}; const { current } = commission || {}; setInitialState({ newPoolName: metadata || '', newRootAddress: roles?.root?.toString() || '', newNominatorAddress: roles?.nominator?.toString() || '', newBouncerAddress: roles?.bouncer?.toString() || '', newCommissionPayee: (current as any)?.[1]?.toString() || '', newCommissionValue: commissionValue || 0 }); }, [pool, commissionValue]); // Add this function at the component level const setInitialState = (state: Partial<typeof initialState>) => { Object.entries(state).forEach(([key, value]) => { if (value !== undefined && value !== null) { (setState as any)(key)(value); } }); };This approach reduces redundancy and improves the hook's readability.
packages/extension-polkagate/src/popup/staking/pool/stake/joinPool/partials/PoolsTable.tsx (1)
150-151: Approved: Naming convention improvementThe change from
mayBeCommissiontomaybeCommissionimproves consistency with common JavaScript/TypeScript naming conventions for optional values. This is a good adjustment that enhances code readability without affecting functionality.Consider adding a comment explaining the
maybeCommissioncalculation for better code documentation:// Extract commission value if present, otherwise default to 0 const maybeCommission = (pool.bondedPool as any).commission.current.isSome ? (pool.bondedPool as any).commission.current.value[0] : 0;packages/extension-polkagate/src/fullscreen/governance/post/Chronology.tsx (1)
Line range hint
82-98: Summary: Improved code consistency and readabilityThe changes in this file focus on standardizing the naming convention for variables, changing from
mayBetomaybe. This improves code readability and maintains consistency throughout theChronologycomponent. All modifications are non-functional and do not alter the component's behavior.However, to further enhance the code quality, consider the following suggestions:
- Add comments to explain the purpose of complex logic, such as the
isInTreasuryQueuecomputation.- Consider extracting some of the complex logic into separate functions for better readability and maintainability.
packages/extension-polkagate/src/components/SignArea2.tsx (2)
36-36: Approve renaming ofmayBeApitomaybeApiThe renaming of
mayBeApitomaybeApiin thePropsinterface and the component function signature improves consistency and readability. This change aligns with common TypeScript/JavaScript naming conventions.For complete consistency, consider updating the comment above the
SignAreacomponent to reflect this change ifmayBeApiis mentioned there.Also applies to: 68-68, 74-74
Line range hint
1-424: Consider future refactoring for improved maintainabilityWhile the current changes are appropriate, the
SignAreacomponent is quite large and handles multiple responsibilities (Ledger, QR, Proxy, Password signing). For future improvements, consider breaking this component down into smaller, more focused components. This could enhance maintainability and make the code easier to test and update.Some suggestions for future refactoring:
- Extract the different signing methods (Ledger, QR, Proxy, Password) into separate components.
- Consider using the React Context API or a state management library to handle shared state and reduce prop drilling.
- Move complex logic (like payload creation) into custom hooks or utility functions for better reusability and testability.
packages/extension-polkagate/src/fullscreen/governance/post/castVote/Cast.tsx (1)
143-148: LGTM: Consistent variable usage with a minor improvement suggestionThe variable renaming is consistently applied within the useEffect hook, maintaining the existing logic. However, we can further improve the code by using optional chaining.
Consider updating line 146 to use optional chaining for better null safety:
- previousVote?.standard && setConviction(getConviction(previousVote.standard.vote)); + previousVote?.standard?.vote && setConviction(getConviction(previousVote.standard.vote));🧰 Tools
🪛 Biome
[error] 146-146: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/extension-polkagate/src/fullscreen/manageIdentity/Review.tsx (2)
359-359: LGTM! Consider adding a type annotation formaybeApi.The renaming from
mayBeApitomaybeApiis correct, fixing a typo in the prop name. This change improves code consistency and readability.To further enhance type safety, consider adding a type annotation for the
maybeApiprop. For example:maybeApi?: ApiPromiseThis will ensure that the correct type is always passed to the
SignArea2component.
Line range hint
1-453: Consider refactoring for improved maintainability and type safety.While the specific change is correct, here are some suggestions to improve the overall code quality:
- The file is quite long and complex. Consider splitting it into smaller, more manageable components to improve readability and maintainability.
- Address the disabled TypeScript checks (
// @ts-nocheck) to improve type safety.- The
Reviewcomponent has many responsibilities. Consider refactoring it to achieve better separation of concerns.- Review and address any TODO comments that are still relevant, or remove them if they're no longer needed.
These improvements will enhance the overall code quality, making it easier to maintain and less prone to errors in the future.
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (1)
Line range hint
309-321: RefactorsetInputsuseEffect for improved readabilityThe
setInputsuseEffect hook now handles different asset types and usesparsedAssetId, which is good. However, the nested ternary operators make the code hard to read.Consider refactoring the params assignment for better readability:
let params; if (isCrossChain) { params = crossChainParams; } else if (isNonNativeToken) { if (['currencies', 'tokens'].includes(onChainCall?.section || '')) { params = [recipientAddress, balances.currencyId, amountAsBN]; // for transferring on multiasset chains } else { params = [parsedAssetId, recipientAddress, amountAsBN]; // for transferring on asset hubs } } else if (transferType === 'All') { params = [recipientAddress, false]; // transferAll with keepalive = false } else { params = [recipientAddress, amountAsBN]; } setInputs({ // ... other properties params: params as unknown[], // ... remaining properties });This refactored version uses if-else statements for better clarity and maintainability.
replacements/interfaces.js (1)
Incorrect SS58 'prefix' Value for 'paseoAssetHub'
The
prefixforpaseoAssetHubis currently set to0. To align with testnet conventions, it should be updated to42.
replacements/interfaces.js(Lines: 94-100)🔗 Analysis chain
Line range hint
97-105: Verify the SS58 'prefix' Value for 'paseoAssetHub'The
paseoAssetHubnetwork is assigned aprefixof0. Please verify if this is the correct SS58 address prefix forpaseoAssetHub. IfpaseoAssetHubis a testnet similar towestmint(which uses aprefixof42), it might also require aprefixof42to align with testnet conventions.Run the following script to check the assigned prefixes for
paseoAssetHubin the codebase:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the SS58 prefix for 'paseoAssetHub' # Expected Outcome: Confirm whether 'paseoAssetHub' has the correct 'prefix' assigned. # Command to search for 'paseoAssetHub' entries and display their prefixes rg --no-heading -A 5 '"network": "paseoAssetHub"'Length of output: 404
Script:
#!/bin/bash # Description: Locate and verify the SS58 prefix for 'paseoAssetHub' # Command to search for 'paseoAssetHub' entries along with their 'prefix' values rg -B 1 -A 2 '"network": "paseoAssetHub"' | rg '"prefix":'Length of output: 104
packages/extension-polkagate/src/hooks/useBalances.ts (1)
52-53: Typo in comment: 'chaines' should be 'chains'There's a typo in the comment. The word 'chaines' should be corrected to 'chains'.
-/** to SAVE fetched balance in local storage, first load saved balances of different chaines if any */ +/** To save fetched balance in local storage, first load saved balances of different chains if any */packages/extension-polkagate/src/hooks/useNativeAssetBalances.ts (3)
5-5: Unnecessary usage of//@ts-ignoreThe
//@ts-ignorecomment at line 5 does not appear to be necessary since there is no immediate TypeScript error being suppressed. Consider removing it to keep the code clean.
35-35: Simplify type assertion forexistentialDepositAt line 35, the type casting
as unknown as BNmay be unnecessary or overly complex. Consider simplifying the type assertion to improve code clarity:const ED = api.consts['balances'] ? api.consts['balances']['existentialDeposit'] as BN : BN_ZERO;
129-129: Simplify type assertionAt line 129, the expression
stakingAccount?.stakingLedger?.total as unknown as BNcan be streamlined. Verify the type ofstakingAccount?.stakingLedger?.totaland use a direct type assertion if possible:soloTotal: stakingAccount?.stakingLedger?.total as BNpackages/extension-polkagate/src/partials/QuickAction.tsx (2)
60-64: Useaddressinstead offormattedfor consistencyIn the
goToCrowdLoansfunction, consider usingaddressdirectly in the condition check instead offormatted, asaddressis already available and used consistently in other navigation functions. This enhances readability and consistency.
Line range hint
100-110: Refactor to reduce code duplication in icon stylingThe logic for determining icon colors and the
textDisabledproperty is repeated across multipleHorizontalMenuItemcomponents. Consider creating a helper function or a styled component to handle the conditional styling to improve maintainability and reduce code duplication.Also applies to: 118-131, 142-152, 160-164, 172-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (53)
- packages/extension-polkagate/src/components/Assets.tsx (1 hunks)
- packages/extension-polkagate/src/components/ChainLogo.tsx (2 hunks)
- packages/extension-polkagate/src/components/SignArea2.tsx (2 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx (2 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/index.tsx (3 hunks)
- packages/extension-polkagate/src/fullscreen/governance/FullScreenHeader.tsx (2 hunks)
- packages/extension-polkagate/src/fullscreen/governance/post/Chronology.tsx (3 hunks)
- packages/extension-polkagate/src/fullscreen/governance/post/Description.tsx (3 hunks)
- packages/extension-polkagate/src/fullscreen/governance/post/Metadata.tsx (3 hunks)
- packages/extension-polkagate/src/fullscreen/governance/post/castVote/Cast.tsx (4 hunks)
- packages/extension-polkagate/src/fullscreen/governance/utils/util.ts (1 hunks)
- packages/extension-polkagate/src/fullscreen/manageIdentity/Review.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/sendFund/Confirmation.tsx (4 hunks)
- packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (12 hunks)
- packages/extension-polkagate/src/fullscreen/sendFund/index.tsx (3 hunks)
- packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/stake/pool/partials/PoolsTable.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/stake/solo/commonTasks/configurePayee/index.tsx (1 hunks)
- packages/extension-polkagate/src/hooks/index.ts (3 hunks)
- packages/extension-polkagate/src/hooks/useAccountAssetsOptions.ts (2 hunks)
- packages/extension-polkagate/src/hooks/useAccountLocks.ts (4 hunks)
- packages/extension-polkagate/src/hooks/useAssetsBalances.ts (2 hunks)
- packages/extension-polkagate/src/hooks/useBalances.ts (2 hunks)
- packages/extension-polkagate/src/hooks/useBalancesOnAssethub.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useBalancesOnMultiAssetChain.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useNativeAssetBalances.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useNativeTokenPrice.ts (1 hunks)
- packages/extension-polkagate/src/hooks/usePrices.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useReferendum.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useReservedDetails.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useTokenPrice.ts (2 hunks)
- packages/extension-polkagate/src/hooks/useTokens.ts (1 hunks)
- packages/extension-polkagate/src/partials/QuickAction.tsx (7 hunks)
- packages/extension-polkagate/src/partials/QuickActionFullScreen.tsx (4 hunks)
- packages/extension-polkagate/src/popup/account/index.tsx (1 hunks)
- packages/extension-polkagate/src/popup/history/Explorer.tsx (2 hunks)
- packages/extension-polkagate/src/popup/staking/partial/ShowPool.tsx (1 hunks)
- packages/extension-polkagate/src/popup/staking/partial/ShowRoles.tsx (1 hunks)
- packages/extension-polkagate/src/popup/staking/pool/myPool/editPool/index.tsx (1 hunks)
- packages/extension-polkagate/src/popup/staking/pool/stake/joinPool/index.tsx (1 hunks)
- packages/extension-polkagate/src/popup/staking/pool/stake/joinPool/partials/PoolsTable.tsx (1 hunks)
- packages/extension-polkagate/src/popup/staking/solo/fastUnstake/index.tsx (1 hunks)
- packages/extension-polkagate/src/popup/staking/solo/unstake/Review.tsx (1 hunks)
- packages/extension-polkagate/src/util/api/postData.ts (1 hunks)
- packages/extension-polkagate/src/util/api/signAndSend.ts (1 hunks)
- packages/extension-polkagate/src/util/constants.tsx (5 hunks)
- packages/extension-polkagate/src/util/getLogo.ts (1 hunks)
- packages/extension-polkagate/src/util/getLogo2.ts (1 hunks)
- packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (3 hunks)
- packages/extension-polkagate/src/util/workers/getAssetOnRelayChain.js (1 hunks)
- packages/extension-polkagate/src/util/workers/utils/toGetNativeToken.js (2 hunks)
- packages/extension-ui/src/Popup/index.tsx (1 hunks)
- replacements/interfaces.js (3 hunks)
✅ Files skipped from review due to trivial changes (7)
- packages/extension-polkagate/src/hooks/usePrices.ts
- packages/extension-polkagate/src/hooks/useTokens.ts
- packages/extension-polkagate/src/popup/staking/pool/stake/joinPool/index.tsx
- packages/extension-polkagate/src/popup/staking/solo/unstake/Review.tsx
- packages/extension-polkagate/src/util/api/postData.ts
- packages/extension-polkagate/src/util/api/signAndSend.ts
- packages/extension-ui/src/Popup/index.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx
- packages/extension-polkagate/src/fullscreen/accountDetails/index.tsx
- packages/extension-polkagate/src/fullscreen/sendFund/index.tsx
- packages/extension-polkagate/src/hooks/index.ts
- packages/extension-polkagate/src/hooks/useBalancesOnAssethub.ts
- packages/extension-polkagate/src/hooks/useBalancesOnMultiAssetChain.ts
- packages/extension-polkagate/src/hooks/useTokenPrice.ts
🧰 Additional context used
🪛 Biome
packages/extension-polkagate/src/fullscreen/governance/post/castVote/Cast.tsx
[error] 146-146: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js
[error] 91-93: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
🔇 Additional comments (74)
packages/extension-polkagate/src/hooks/useAccountAssetsOptions.ts (3)
4-4: LGTM: Import statement improvementThe change to directly import
DropdownOptionfrom the types file enhances code clarity without affecting functionality.
24-24: Verify intentionality and impact of removing fallback valueThe fallback value of
-1forvaluehas been removed. Now,assetIdis directly used, which could potentially beundefined. This change might be intentional to support both string and number types forassetId, but it could lead to issues if the consuming code expectsvalueto always be a number.Please confirm:
- Is this change intentional to support multiple types for
assetId?- Have all places where this value is used been updated to handle potentially undefined values?
To check the usage of this value throughout the codebase, you can run:
Please review the results and ensure that all usages of this hook's return value can handle potentially undefined
valueproperties.
11-11: Verify consistency with coding style guideA space has been added before the opening parenthesis in the function signature. While this doesn't affect functionality, it's important to ensure this change aligns with the project's coding style guide for consistency.
To verify the coding style consistency, you can run the following command:
Please review the results and ensure this change is consistent with other function declarations in the project.
✅ Verification successful
Coding style change is consistent with project conventions
The addition of a space before the opening parenthesis in the function signature is consistent with the majority of function declarations in the project. This change aligns with the predominant coding style.
However, it's worth noting that there are some inconsistencies present in the codebase, with a few functions not following this convention. To maintain better overall consistency, it may be beneficial to:
- Continue following the majority style of including a space before the parenthesis in function declarations.
- Consider gradually updating the remaining inconsistent function declarations to match this style in future refactoring efforts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the coding style for function declarations # Expected result: Consistent use of spaces before parentheses in function declarations rg --type typescript "function\s*\w+\s*\(" packages/Length of output: 89
Script:
#!/bin/bash # Description: Check the coding style for function declarations by searching TypeScript files # Expected result: Consistent use of spaces before parentheses in function declarations rg --glob "*.ts" --glob "*.tsx" "function\s+\w+\s*\(" packages/Length of output: 164741
Script:
#!/bin/bash # Description: Verify consistent spacing before parentheses in TypeScript function declarations # Expected result: List of function declarations that do not have a space before the opening parenthesis rg --glob "*.ts" --glob "*.tsx" "function\s+\w+\s*\(" packages/Length of output: 164741
packages/extension-polkagate/src/hooks/useNativeTokenPrice.ts (5)
11-11: LGTM: Minor formatting change in function signature.The addition of a space before the opening parenthesis in the function signature is a minor formatting change that doesn't affect functionality. This change likely aligns with the project's code style guidelines.
16-18: Improved error handling forchainName.The addition of an early return when
chainNameis falsy enhances the function's robustness. This prevents potential runtime errors that could occur when trying to process an undefinedchainName.
22-24: Enhanced error handling forcurrentChainName.The addition of another early return when
currentChainNameis falsy after sanitization further improves the function's reliability. This ensures that only valid chain names are used in subsequent operations.
27-27: Corrected variable naming:maybeTestNetPrice.The variable has been renamed from
mayBeTestNetPricetomaybeTestNetPrice, correcting a minor typo. This change improves code readability and adheres to common naming conventions.
11-29: Overall improvements in error handling and code quality.The changes made to the
useNativeTokenPricefunction significantly enhance its robustness and readability. The introduction of early returns for invalidchainNameandcurrentChainNamevalues prevents potential runtime errors. The variable renaming improves consistency with naming conventions. These modifications collectively result in more reliable and maintainable code.packages/extension-polkagate/src/util/workers/utils/toGetNativeToken.js (2)
6-6: LGTM: Import changes align with PR objectives.The addition of
NATIVE_TOKEN_ASSET_ID_ON_ASSETHUBimport is consistent with the PR's goal of adding foreign assets support on asset hubs. This new constant will likely be used to differentiate asset IDs on asset hubs from other chains.
23-26: Verify the impact of assetId changes across the codebase.The modification to the
assetIdassignment in thetoGetNativeTokenfunction may affect other parts of the codebase that rely on this value. It's important to ensure that all consumers of this function are updated to handle bothNATIVE_TOKEN_ASSET_IDandNATIVE_TOKEN_ASSET_ID_ON_ASSETHUB.Run the following script to identify potential areas that might need updates:
Please review the output of this script and ensure that all identified locations correctly handle the new
assetIdassignment logic.packages/extension-polkagate/src/util/getLogo.ts (2)
37-50: Summary of changes in getLogo functionThe changes in this file are minimal and focused on improving code readability:
- Renaming of
mayBeExternalLogotomaybeExternalLogo.- Consistent usage of the renamed variable throughout the function.
These changes align with best practices in naming conventions and do not alter the functionality of the
getLogofunction. The overall logic and structure of the function remain unchanged, maintaining its original behavior while improving code quality.
43-47: Consistent usage of renamed variableThe renamed variable
maybeExternalLogois correctly used in the conditional logic and later in the function. This change maintains consistency with the earlier variable renaming, ensuring that the new name is used throughout the function without altering its logic or functionality.To ensure the variable renaming is consistent throughout the codebase, run the following script:
Also applies to: 50-50
✅ Verification successful
Consistent usage of renamed variable
The renamed variable
maybeExternalLogois correctly used throughout the function with no remaining instances ofmayBeExternalLogo. This change maintains consistency and ensures that the logic and functionality remain unchanged.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of 'maybeExternalLogo' and check for any remaining instances of 'mayBeExternalLogo' # Test 1: Search for correct usage of 'maybeExternalLogo' echo "Correct usage of 'maybeExternalLogo':" rg --type typescript "maybeExternalLogo" # Test 2: Search for any remaining instances of 'mayBeExternalLogo' echo "Checking for any remaining instances of 'mayBeExternalLogo':" rg --type typescript "mayBeExternalLogo"Length of output: 379
Script:
#!/bin/bash # Description: Verify the usage of 'maybeExternalLogo' and check for any remaining instances of 'mayBeExternalLogo' in TypeScript files # Test 1: Search for correct usage of 'maybeExternalLogo' echo "Correct usage of 'maybeExternalLogo':" rg --type ts "maybeExternalLogo" rg --type tsx "maybeExternalLogo" # Test 2: Search for any remaining instances of 'mayBeExternalLogo' echo "Checking for any remaining instances of 'mayBeExternalLogo':" rg --type ts "mayBeExternalLogo" rg --type tsx "mayBeExternalLogo"Length of output: 1007
packages/extension-polkagate/src/util/getLogo2.ts (1)
57-57: Approved: Consistent variable usageThe usage of the renamed variable
maybeExternalLogois consistent with the earlier change. This ensures that the variable is correctly referenced throughout the function.To ensure that all occurrences of the variable have been updated, let's run the following verification script:
✅ Verification successful
Verification Successful: Variable Renaming Confirmed
All occurrences of
maybeExternalLogohave been properly updated ingetLogo2.ts, ensuring consistent variable usage throughout the function.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all occurrences of the variable have been updated # Test: Search for any remaining instances of 'mayBeExternalLogo' rg --type typescript 'mayBeExternalLogo' packages/extension-polkagate/src/util/getLogo2.ts # Expected result: No output, indicating that all instances have been updatedLength of output: 125
Script:
#!/bin/bash # Description: Verify that all occurrences of the variable have been updated # Test: Search for any remaining instances of 'mayBeExternalLogo' without specifying the type rg 'mayBeExternalLogo' packages/extension-polkagate/src/util/getLogo2.ts # Expected result: No output, indicating that all instances have been updatedLength of output: 72
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (3)
94-100: LGTM! Good defensive programming.The changes to the message handling logic are well-thought-out. Setting a default empty array for
assetsToBeFetchedwhen it's not provided is a good defensive programming practice. The warning message is also helpful for debugging purposes.
91-93: Ignore static analysis warning about global reassignment.The static analysis tool flagged a potential issue with global variable reassignment for
onmessage. However, in the context of a Web Worker, this is the correct way to define the message handler.This is not an actual issue, as reassigning
onmessageis the standard practice for setting up event listeners in Web Workers. The static analysis tool lacks the context to understand this specific use case.🧰 Tools
🪛 Biome
[error] 91-93: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
9-9: Provide explanations for @ts-ignore directives.There are still @ts-ignore directives without explanations on lines 9, 62, and 73. As mentioned in previous reviews, it's important to provide comments explaining why these directives are necessary.
Please add brief explanations for each @ts-ignore directive. This helps maintain code clarity and assists future developers in understanding why TypeScript checks are being suppressed in these specific instances.
Also applies to: 62-62, 73-73
packages/extension-polkagate/src/popup/staking/partial/ShowRoles.tsx (1)
48-53: Approved: Improved variable naming conventionThe change from
mayBeCommissionAddresstomaybeCommissionAddressis a good improvement in the naming convention. The new name better follows the common practice of using "maybe" as a prefix for optional values, enhancing code readability.The functionality remains unchanged, and the update is consistent with the camelCase naming convention used throughout the codebase.
packages/extension-polkagate/src/fullscreen/governance/FullScreenHeader.tsx (3)
14-14: LGTM: Import statement updated correctlyThe addition of
NATIVE_TOKEN_ASSET_IDto the import statement is consistent with its usage in the file and doesn't introduce any issues.
Line range hint
1-138: Overall assessment: Changes look good and align with PR objectivesThe modifications to this file are minimal and focused on supporting the new feature for foreign assets. The changes are consistent with the PR objectives and don't introduce any apparent bugs or inconsistencies. The addition of
NATIVE_TOKEN_ASSET_IDto the import statement and its usage in the URL construction for the 'send' page are appropriate for the intended functionality.
66-66: LGTM: URL updated to include native token asset IDThe modification to include
NATIVE_TOKEN_ASSET_IDin the URL for the 'send' page is consistent with the new import and likely supports the new feature for foreign assets.To ensure consistency across the application, please run the following script to check if other parts of the codebase use similar URL structures for asset-related pages:
This will help verify if the change is consistent with how other parts of the application handle asset IDs in URLs.
✅ Verification successful
Verified: URL structures consistently include native token asset ID
The usage of
NATIVE_TOKEN_ASSET_IDin URL constructions is consistent across the codebase, ensuring uniform handling of asset IDs in 'send' pages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar URL structures in asset-related pages # Search for URL patterns similar to '/send/' followed by parameters rg --type typescript --type javascript -g '!*test*' '/send/.*\$\{.*\}' # Search for other uses of NATIVE_TOKEN_ASSET_ID in URL construction rg --type typescript --type javascript -g '!*test*' 'NATIVE_TOKEN_ASSET_ID.*\`'Length of output: 304
Script:
#!/bin/bash # Description: Check for similar URL structures in asset-related pages # Search for URL patterns similar to '/send/' followed by parameters rg --type typescript --type javascript -g '!*.test.*' '/send/\$\{.+\}' # Search for other uses of NATIVE_TOKEN_ASSET_ID in URL construction rg --type typescript --type javascript -g '!*.test.*' 'NATIVE_TOKEN_ASSET_ID.*`'Length of output: 225
Script:
#!/bin/bash # Description: Check for similar URL structures in asset-related pages # Search for URL patterns similar to '/send/' followed by parameters rg --type ts --type js -g '!*.test.*' '/send/\$\{.+\}' # Search for other uses of NATIVE_TOKEN_ASSET_ID in URL construction rg --type ts --type js -g '!*.test.*' 'NATIVE_TOKEN_ASSET_ID.*\`'Length of output: 1256
Script:
#!/bin/bash # Description: Check for similar URL structures in asset-related pages # Search for URL patterns similar to '/send/' followed by parameters rg --type ts --type js -g '!*.test.*' '/send/\$\{.+\}' # Search for other uses of NATIVE_TOKEN_ASSET_ID in URL construction rg --type ts --type js -g '!*.test.*' 'NATIVE_TOKEN_ASSET_ID.*`'Length of output: 1468
packages/extension-polkagate/src/fullscreen/sendFund/Confirmation.tsx (7)
47-47: LGTM: Improved function signatureThe updated function signature using object destructuring enhances readability and aligns with modern JavaScript best practices. This change is purely syntactical and doesn't introduce any functional modifications.
91-91: LGTM: Consistent prop usageThe direct passing of the
infoprop without type assertion is consistent with the earlier change to make it optional. This simplifies the code and removes unnecessary type assertions, improving overall code clarity.
99-103: LGTM: Simplified translation function usageThe removal of the generic type argument
<string>from thetfunction calls simplifies the code without affecting functionality. The TypeScript compiler can infer the return type oft, making the explicit type argument unnecessary.
108-108: LGTM: Consistent prop usageThe direct passing of the
infoprop to the Account component is consistent with the earlier changes. This maintains code consistency throughout the Confirmation component.
111-119: LGTM: Improved null handling and consistent translation usageThe changes in this section are positive:
- The removal of the generic type argument from the
tfunction calls is consistent with earlier changes.- The use of the nullish coalescing operator (
??) infee?.toHuman() as string ?? '00.00'improves null handling, providing a default value iffeeis null or undefined.These modifications enhance code consistency and robustness.
148-148: LGTM: Consistent translation function usageThe removal of the generic type argument from the
tfunction call is consistent with earlier changes in the file. This maintains code consistency throughout the Confirmation component.
Line range hint
1-151: Overall assessment: Approved with minor suggestionsThe changes in this file primarily focus on improving type handling, null safety, and simplifying translation function usage. These modifications enhance code quality, consistency, and robustness. The key improvements include:
- Making the
infoprop optional in the Account component, improving flexibility.- Consistent use of optional chaining and nullish coalescing for better null handling.
- Removal of unnecessary generic type arguments from translation function calls.
While the changes are generally positive, consider implementing the suggestion for additional null checks in the Account component to further improve robustness.
Great job on these improvements!
packages/extension-polkagate/src/hooks/useAccountLocks.ts (3)
Line range hint
155-161: Approve consistent variable usage.The use of
maybePriorshere is consistent with the earlier renaming. This change maintains the improved naming convention throughout the function.
Line range hint
1-236: Summary of changes: Improved naming convention.The changes in this file consist solely of renaming the variable
mayBePriorstomaybePriors. This improves consistency with JavaScript/TypeScript naming conventions and enhances code readability. No functional changes were made, and the overall logic of theuseAccountLockshook remains intact.These changes are low-risk and improve the quality of the codebase. Ensure that this renaming is consistent across the entire project for maximum benefit.
Line range hint
183-203: Approve consistent variable usage and suggest verification.The use of
maybePriorsin these locations is consistent with the earlier renaming. This change maintains the improved naming convention throughout the function.To ensure that all instances of the variable have been updated correctly, run the following command:
✅ Verification successful
Consistent Variable Renaming Confirmed
All instances of
mayBePriorshave been successfully renamed tomaybePriorsin the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instances of 'mayBePriors' have been updated to 'maybePriors' # Test: Search for any remaining instances of 'mayBePriors'. Expect: No results. rg --type typescript 'mayBePriors' # Test: Confirm the presence of the new 'maybePriors' variable. Expect: Multiple results. rg --type typescript 'maybePriors'Length of output: 139
Script:
#!/bin/bash # Description: Verify all instances of 'mayBePriors' have been updated to 'maybePriors' # Test: Search for any remaining instances of 'mayBePriors'. Expect: No results. rg --type ts 'mayBePriors' rg --type tsx 'mayBePriors' # Test: Confirm the presence of the new 'maybePriors' variable. Expect: Multiple results. rg --type ts 'maybePriors' rg --type tsx 'maybePriors'Length of output: 524
packages/extension-polkagate/src/popup/staking/solo/fastUnstake/index.tsx (2)
51-51: Consistent variable usage.The usage of
maybeMyStashBalancesin theuseMemohook has been updated to reflect the variable renaming from line 45. This change maintains consistency throughout the code.
45-51: Overall assessment: Minor improvements in naming convention.The changes in this file are minimal and focus on improving the naming convention of a variable. These modifications enhance code readability and consistency without altering the functionality of the component. The overall structure and logic remain intact, and no further actions are necessary.
packages/extension-polkagate/src/fullscreen/governance/post/Description.tsx (4)
Line range hint
54-62: Approved: Improved variable naming conventionThe renaming of
mayBeBeneficiarytomaybeBeneficiaryenhances code readability by adhering to standard JavaScript naming conventions. This change maintains consistency throughout the codebase without altering the logic.
63-63: Approved: Consistent variable namingThe renaming of
mayBeTreasuryProposalIdtomaybeTreasuryProposalIdaligns with the previous variable renaming, maintaining consistency in the naming convention throughout the component. This change enhances code readability without affecting the functionality.
85-89: Approved: Consistent use of renamed variableThe update in the JSX to use
maybeTreasuryProposalIdinstead ofmayBeTreasuryProposalIdmaintains consistency with the earlier variable renaming. This change ensures that the component correctly uses the updated variable name without introducing any functional changes.
Line range hint
1-189: Summary: Improved code consistency through variable renamingThe changes in this file focus on renaming variables to improve consistency with JavaScript naming conventions. Specifically:
mayBeBeneficiary→maybeBeneficiarymayBeTreasuryProposalId→maybeTreasuryProposalIdThese changes enhance code readability and maintain a consistent style throughout the component. The functionality of the
ReferendumDescriptioncomponent remains unchanged, and the updates have been applied consistently in both the variable declarations and their usage in the JSX.Overall, these changes represent a positive improvement in code quality without introducing any functional modifications or potential issues.
packages/extension-polkagate/src/hooks/useReservedDetails.ts (3)
72-72: LGTM: Improved variable naming.The change from
mayBeDeposittomaybeDepositis a good correction. It improves code readability and maintains consistency with common naming conventions in JavaScript/TypeScript.
74-76: LGTM: Consistent variable usage.The changes in this segment correctly use the renamed
maybeDepositvariable. The logic remains unchanged, and the modifications are consistent with the previous renaming.
Line range hint
1-324: Overall assessment: Minor improvements, no functional changes.The changes in this file are limited to renaming a variable from
mayBeDeposittomaybeDeposit. This improvement enhances code readability and maintains consistency with common naming conventions. The functionality of theuseReservedDetailshook remains unchanged, and no potential issues were introduced by these modifications.packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/editPool/Edit.tsx (1)
43-43: Approved: Improved variable naming consistency.The change from
mayBeCommissiontomaybeCommissionenhances code readability and maintains consistent naming conventions. This is a good practice that doesn't affect functionality.packages/extension-polkagate/src/util/constants.tsx (4)
50-50: Please verify the updated PASEO_ASSET_HUB_GENESIS_HASH.The genesis hash for the Paseo Asset Hub has been updated. This is a critical change that can affect the entire system.
Could you please confirm that this new hash value is correct and intentional? It's crucial to ensure that this update aligns with the intended changes in the Paseo network.
88-90: LGTM! Improved maintainability of PROXY_CHAINS.The use of spread syntax to include
RELAY_CHAINS_GENESISHASHandASSET_HUBSinPROXY_CHAINSis a good improvement. It makes the code more maintainable and reduces the risk of forgetting to add new chains in the future.
110-111: LGTM! Improved maintainability of STAKING_CHAINS.The use of spread syntax to include
RELAY_CHAINS_GENESISHASHinSTAKING_CHAINSis a good improvement. It ensures that all relay chains are automatically included as staking chains, improving maintainability.
220-220: LGTM! Improved readability of ProxyTypeIndex type.The formatting change in the
ProxyTypeIndextype definition improves readability without affecting functionality.packages/extension-polkagate/src/fullscreen/governance/post/Metadata.tsx (1)
Line range hint
176-190: LGTM: Consistent variable renaming.The change from
mayBeBeneficiarytomaybeBeneficiaryis consistent with the earlier renaming. The conditional rendering logic remains correct and unchanged.packages/extension-polkagate/src/fullscreen/stake/solo/commonTasks/configurePayee/index.tsx (1)
202-203: Improved object comparison and variable namingThe changes in this segment enhance the code in two ways:
The variable name
mayBeNewhas been corrected tomaybeNew, improving readability and adhering to common naming conventions.The condition for setting the
payeehas been updated to useJSON.stringifyfor comparison. This is a more robust method for comparing complex objects, ensuring that the payee is only updated when there's an actual difference in the object structure or values.These changes improve the accuracy of the payee update logic and make the code more maintainable.
packages/extension-polkagate/src/fullscreen/stake/pool/partials/PoolsTable.tsx (1)
150-151: Approved: Improved variable naming conventionThe change from
mayBeCommissiontomaybeCommissionis a positive improvement. It corrects a typo in the variable name, enhancing code readability and maintaining consistency in naming conventions. This type of attention to detail contributes to overall code quality.packages/extension-polkagate/src/fullscreen/governance/post/Chronology.tsx (4)
82-82: Approved: Improved variable namingThe change from
mayBeExecutionBlocktomaybeExecutionBlockenhances code readability and maintains consistency in naming conventions.
83-83: Approved: Consistent variable namingThe renaming of
mayBeBeneficiarytomaybeBeneficiaryaligns with the previous change, maintaining consistency in variable naming throughout the component.
92-95: Approved: Consistent naming convention appliedThe change from
mayBeAwardedDatetomaybeAwardedDatecompletes the set of naming convention improvements in this file. This consistency enhances the overall readability of the code.
98-98: Approved: Consistent use of renamed variableThe update to use
maybeBeneficiaryin theisInTreasuryQueuecomputation is crucial for maintaining the code's functionality after the variable renaming. This change ensures that the renamed variable is used consistently throughout the component.packages/extension-polkagate/src/popup/account/index.tsx (2)
88-89: Improved robustness ingoToSendfunctionThe change in the
goToSendfunction enhances its reliability by using a fallback value forassetId. This ensures that anassetIdis always provided when opening the send window, even if theassetIdstate is undefined.
Line range hint
1-389: Overall assessment of changesThe visible changes in this file improve the robustness of the
AccountDetailscomponent, particularly in handling asset IDs when navigating to the send page. The modification to thegoToSendfunction ensures that anassetIdis always provided, even when the state value is undefined.Note: The AI summary mentioned changes to the
_onChangeAssetfunction, but these changes are not visible in the provided code snippet. Therefore, we cannot provide a comprehensive review of all the reported changes.To ensure all reported changes are accounted for, please run the following command to check for any modifications to the
_onChangeAssetfunction:packages/extension-polkagate/src/components/SignArea2.tsx (1)
Line range hint
1-424: Summary: Approved with suggestions for future improvementsThe changes in this PR, specifically the renaming of
mayBeApitomaybeApi, are approved as they improve naming consistency and readability. While the component is quite large and complex, suggesting major refactoring is out of scope for this PR. However, consider the suggestions for future improvements to enhance maintainability and testability of theSignAreacomponent.packages/extension-polkagate/src/fullscreen/governance/post/castVote/Cast.tsx (3)
104-104: LGTM: Improved variable namingThe renaming of
mayBePreviousVotetomaybePreviousVoteenhances code consistency and readability. This change doesn't affect functionality but improves the overall code quality.
Line range hint
331-339: LGTM: Consistent JSX updateThe condition for rendering the Warning component has been updated to use the renamed variable
maybePreviousVote. This change maintains consistency with the earlier variable renaming while preserving the existing logic.
408-408: LGTM: Consistent prop updateThe height prop of the Grid component has been updated to use the renamed variable
maybePreviousVote. This change maintains consistency with the earlier variable renaming while preserving the existing conditional height setting logic.packages/extension-polkagate/src/hooks/useReferendum.ts (1)
243-245: Improved variable naming for better readabilityThe changes in this section enhance the code's readability by consistently using the prefix "maybe" instead of "mayBe" for optional values. This adjustment aligns with common naming conventions for potentially undefined values and improves overall code clarity.
Here's a summary of the changes:
mayBeUnwrappedResult→maybeUnwrappedResultmayBeOngoingRef→maybeOngoingRefmayBeTally→maybeTallymayBeOrigin→maybeOriginThese modifications maintain the existing functionality while making the code more consistent and easier to understand.
Also applies to: 247-248, 251-251, 254-254
packages/extension-polkagate/src/hooks/useAssetsBalances.ts (2)
77-77: UpdateFetchedBalanceinterface usage to accommodate requiredassetIdThe
assetIdproperty in theFetchedBalanceinterface has been changed from an optional number to a required number or string. This change aligns with the modification made to theMessageBodyinterface, maintaining consistency and increasing flexibility.To ensure this change doesn't introduce any issues, please run the following script to check for any places where
FetchedBalanceis used without providing anassetId:
49-49: UpdateMessageBodyinterface usage to accommodate requiredassetIdThe
assetIdproperty in theMessageBodyinterface has been changed from an optional number to a required number or string. This change increases flexibility but may require updates in code that uses this interface.To ensure this change doesn't introduce any issues, please run the following script to check for any places where
MessageBodyis used without providing anassetId:✅ Verification successful
No instances of
MessageBodyusage withoutassetIdwere found. The update has been verified and does not introduce any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MessageBody usage without assetId rg --type typescript 'MessageBody.*[^assetId]'Length of output: 86
Script:
#!/bin/bash # Search for MessageBody usage without assetId in .ts and .tsx files rg 'MessageBody.*[^assetId]' -g '*.ts' -g '*.tsx'Length of output: 247
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (1)
9-9: LGTM: New imports and type changesThe new imports and type changes are appropriate for supporting foreign assets and improving asset ID handling. The
assetIdtype change in thePropsinterface allows for more flexible asset ID representations, which is beneficial for supporting various asset types.Also applies to: 25-26, 34-34
replacements/interfaces.js (2)
57-57: Addition of Genesis Hash for 'paseoAssetHub' Looks CorrectThe addition of the
paseoAssetHubgenesis hash appears accurate and consistent with the format used for other networks.
Line range hint
88-94: 'statemine' Entry in 'assetHubs' Array Looks GoodThe inclusion of the
stateminenetwork with appropriate properties in theassetHubsarray is correct.packages/extension-polkagate/src/hooks/useBalances.ts (2)
79-81: Verify thatassetBalancereturns correct balance informationWhen
maybeNonNativeAssetIdis truthy, the function returnsassetBalance. Ensure thatassetBalancecorrectly reflects the balances for the specified non-native asset and that all necessary balance information is included.
14-14: Ensure all usages ofuseBalancesaccommodate the updatedassetIdtypeThe
assetIdparameter type has been changed fromnumbertostring | number. Please verify that all calls touseBalancesare updated accordingly to handle both string and number types forassetId.You can run the following script to locate all invocations of
useBalancesand check theassetIdparameter:packages/extension-polkagate/src/partials/QuickAction.tsx (5)
6-6: ImportingAccountIdtype is appropriateThe import of
AccountIdis necessary for defining the type of theaddressprop in thePropsinterface.
18-18: Constants are correctly importedThe constants
CROWDLOANS_CHAINS,GOVERNANCE_CHAINS,NATIVE_TOKEN_ASSET_ID, andSTAKING_CHAINSare appropriately imported from'../util/constants'and used within the component.
32-32: DestructuringuseInfohook return values is appropriateThe destructured values
{ api, formatted, genesisHash }from theuseInfo(address)hook provide necessary information for the component's functionality.
37-39: Feature flags are correctly initializedThe boolean flags
isStakingEnabled,isCrowdLoanEnabled, andisGovernanceEnabledare properly set based on thegenesisHashand the corresponding chain arrays.
42-43: Usage ofwindowOpenwith correct parametersThe
goToSendfunction correctly constructs the URL usingaddressandNATIVE_TOKEN_ASSET_IDand handles any potential errors with.catch(console.error).packages/extension-polkagate/src/partials/QuickActionFullScreen.tsx (4)
6-7: ImportingAccountIdtype is appropriate.The import of
AccountIdfrom@polkadot/types/interfaces/runtimeis necessary to type theaddressprop correctly.
25-25: UpdatingassetIdtype tonumber | string | undefinedis appropriate.Allowing
assetIdto be anumber,string, orundefinedincreases flexibility in handling asset identifiers. Ensure that all usages ofassetIdcan handle bothnumberandstringtypes without issues.
39-39: IncludingassetIdin function parameters aligns with updated props.Adding
assetIdto the function signature matches the updatedPropsinterface and ensures consistency.
28-28: Verify the change fromtypetointerfaceforQuickActionButtonType.Changing
QuickActionButtonTypefrom atypealias to aninterfacemay have implications if there are extensions or implementations relying on it. Ensure this change does not introduce unintended side effects.To check usage of
QuickActionButtonType, run:
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/hooks/useNativeAssetBalances.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Chart.tsx (1)
Line range hint
1-79: Consider modernizing and refactoring for improved maintainabilityWhile the current implementation works well, here are two suggestions for potential improvements:
Replace
React.memowith more modern React patterns for performance optimization. React's built-in optimizations have improved, often makingmemounnecessary.Consider extracting the chart creation logic into a separate function. This could enhance readability and maintainability, especially if the chart configuration grows more complex in the future.
Example refactoring for point 2:
const createChart = (chartRef: React.RefObject<HTMLCanvasElement>, assets: Props['assets'], theme: Theme) => { if (!assets || !chartRef.current) { return null; } return new Chart(chartRef.current, { // ... (chart configuration) }); }; useEffect(() => { const chartInstance = createChart(chartRef, assets, theme); return () => { chartInstance?.destroy(); }; }, [assets, theme.palette.divider]);These suggestions are optional and aimed at long-term maintainability.
packages/extension-polkagate/src/popup/home/news.ts (1)
16-16: LGTM! Consider a minor wording improvement.The addition of DOT to the list of currencies for balance viewing is a valuable enhancement. It aligns well with the PR objective of adding support for foreign assets.
Consider rewording slightly for better clarity:
- 'BTC, ETH and DOT as currency: View your token balance equivalent in BTC, ETH, DOT and other fiat currencies.', + 'BTC, ETH, and DOT as currencies: View your token balance equivalent in BTC, ETH, DOT, and other fiat currencies.',This change:
- Fixes the double space after "ETH".
- Makes "currencies" plural to match the multiple options.
- Adds a comma before "and" in the list for consistency.
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx (1)
195-195: LGTM: Minor UI adjustment in Grid component padding.The horizontal padding has been slightly reduced from 30px to 25px on both sides. This change doesn't affect functionality and might be for aesthetic reasons or to accommodate more content within the same width.
Could you confirm if this padding adjustment is intentional and aligns with the overall design guidelines? If it's part of a broader design update, consider documenting these changes to maintain consistency across the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/extension-polkagate/src/components/FormatPrice.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Chart.tsx (2 hunks)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx (2 hunks)
- packages/extension-polkagate/src/popup/account/ReservedReasons.tsx (1 hunks)
- packages/extension-polkagate/src/popup/home/news.ts (1 hunks)
- packages/extension-polkagate/src/util/types.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/Chart.tsx (2)
19-19: LGTM: Improved type flexibility forassetIdThe change from
number | undefinedtonumber | string | undefinedfor theassetIdproperty type enhances flexibility. This modification aligns well with the PR objective of adding support for foreign assets on asset hubs.
28-28: LGTM: Enhanced robustness inuseEffectand improved formatting
The expanded conditional check
!assets || !chartRef.currentin theuseEffecthook improves the component's robustness. It prevents potential errors by ensuring the effect doesn't execute if eitherassetsis undefined orchartRefis not initialized.The added space after the function name in
ChartTotaldeclaration improves code style consistency.These changes contribute to better error prevention and code readability.
Also applies to: 35-35
packages/extension-polkagate/src/components/FormatPrice.tsx (1)
54-54: Approve change with suggestions for documentation and verification.The reduction of
DECIMAL_POINTS_FOR_CRYPTO_AS_CURRENCYfrom 4 to 3 seems intentional and is consistently applied within this component. However, I have a few suggestions:
Please add a comment explaining the rationale behind this change. This will help future maintainers understand why 3 decimal points were chosen for crypto assets used as currency.
Verify that this change doesn't negatively impact other parts of the application that might rely on the
FormatPricecomponent, expecting 4 decimal points for crypto assets.To ensure this change doesn't unexpectedly affect other parts of the codebase, please run the following script:
packages/extension-polkagate/src/popup/account/ReservedReasons.tsx (3)
Line range hint
1-138: Summary: Good change, verify compatibilityThe change to allow both
numberandstringtypes forassetIdis a positive improvement that increases flexibility without breaking backward compatibility. To ensure the change is fully integrated:
- Verify that the component and its dependencies handle both types correctly.
- Check the
useTokenPricehook for compatibility with the newassetIdtype.- Run the provided verification scripts and address any issues found.
Once these verifications are complete, the change can be confidently merged.
Line range hint
39-39: Verify useTokenPrice hook compatibilityThe
useTokenPricehook is using theassetIdprop. Ensure that this hook can handle bothnumberandstringtypes forassetId.Please check the implementation of the
useTokenPricehook:#!/bin/bash # Description: Check the implementation of useTokenPrice hook echo "Checking useTokenPrice hook implementation:" rg --type typescript -A 10 "export function useTokenPrice" packages/extension-polkagate/srcReview the results to confirm that the
useTokenPricehook can properly handle bothnumberandstringtypes forassetId. If necessary, update the hook to ensure type compatibility.
26-26: LGTM! Verify handling of string asset IDs.The change to allow both
numberandstringtypes forassetIdincreases flexibility without breaking backward compatibility. This is a good improvement.To ensure the component handles both types correctly, please verify:
- The component's logic properly handles both number and string asset IDs.
- Any dependencies or child components expecting
assetIdcan work with both types.Run the following script to check for potential issues:
Review the results to ensure that the handling of
assetIdis type-safe and works correctly for both number and string inputs.packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx (1)
37-37: LGTM: Improved type flexibility for assetId.The change from
numbertonumber | stringforassetIdenhances flexibility and aligns with the PR objective of supporting foreign assets. This modification allows for different types of asset identifiers across various chains or protocols.To ensure consistency, please verify that all usages of
assetIdthroughout the codebase can handle both number and string types. Run the following script to check for potential issues:✅ Verification successful
Verified:
assetIdtype handling is consistent across the codebase.All usages of
assetIdhave been checked and appropriately handle bothnumberandstringtypes, ensuring type safety and alignment with the PR objective of supporting foreign assets.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of assetId and verify type handling # Search for assetId usages echo "Searching for assetId usages:" rg --type typescript --type tsx 'assetId' # Look for type checks or conversions related to assetId echo "\nSearching for type checks or conversions:" rg --type typescript --type tsx 'typeof.*assetId|assetId\s*(===|!==|==|!=)'Length of output: 355
Script:
#!/bin/bash # Description: Find all usages of assetId and verify type handling in .ts and .tsx files # Search for assetId usages in TypeScript and TSX files echo "Searching for assetId usages in .ts and .tsx files:" rg 'assetId' --glob '**/*.ts?(x)' # Look for type checks or conversions related to assetId in TypeScript and TSX files echo "\nSearching for type checks or conversions related to assetId in .ts and .tsx files:" rg 'typeof\s+assetId|assetId\s*(===|!==|==|!=)' --glob '**/*.ts?(x)'Length of output: 694
Script:
#!/bin/bash # Description: Find all usages of assetId and verify type handling in .ts and .tsx files without using --type flag # Search for assetId usages in .ts and .tsx files echo "Searching for assetId usages in .ts and .tsx files:" rg 'assetId' --glob '*.ts' --glob '*.tsx' # Look for type checks or conversions related to assetId in .ts and .tsx files echo "\nSearching for type checks or conversions related to assetId in .ts and .tsx files:" rg 'typeof\s+assetId|assetId\s*(===|!==|==|!=)' --glob '*.ts' --glob '*.tsx'Length of output: 18941
packages/extension-polkagate/src/util/types.ts (1)
636-636: LGTM: Improved flexibility for assetId representation.The change to allow both
numberandstringtypes forassetIdenhances the flexibility of theBalancesInfointerface. This is a good improvement that can accommodate various asset ID representations across different chains or systems.To ensure this change doesn't introduce any unintended side effects, please run the following script to check for any usage of
assetIdthat might need updating:This script will help identify any areas in the codebase that might need attention due to this type change.
✅ Verification successful
LGTM:
assetIdtype change verified with no adverse impacts detected.The modification to allow
assetIdto be anumberorstringhas been thoroughly checked. The search within the codebase revealed no instances whereassetIdis used in a way that could be affected by this type change. This ensures that the update enhances flexibility without introducing any unintended side effects.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of assetId in BalancesInfo to ensure compatibility with the new type # Search for BalancesInfo usage echo "Searching for BalancesInfo usage:" rg --type typescript -A 5 'BalancesInfo' # Search for assetId usage within BalancesInfo context echo "Searching for assetId usage within BalancesInfo context:" rg --type typescript -A 5 -B 5 'BalancesInfo.*assetId'Length of output: 367
Script:
#!/bin/bash # Description: Check for usage of assetId in BalancesInfo to ensure compatibility with the new type # Search for BalancesInfo usage across all files echo "Searching for BalancesInfo usage:" rg -A 5 'BalancesInfo' # Search for assetId usage within BalancesInfo context across all files echo "Searching for assetId usage within BalancesInfo context:" rg -A 5 -B 5 'BalancesInfo.*assetId'Length of output: 55362
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (3)
Line range hint
11-59: ApprovegetAssetsimplementation, but address @ts-ignore commentsThe
getAssetsfunction implementation looks good and correctly handles both foreign and native assets. It aligns well with the PR objectives of adding foreign assets support.However, there are two more
@ts-ignorecomments in this function (lines 15 and 25). Please provide reasons for these directives or consider fixing the underlying TypeScript issues.
Line range hint
90-116: Approveonmessagehandler changes, but address global reassignmentThe updates to the
onmessagehandler improve error handling and provide a fallback for missingassetsToBeFetched. The warning message is helpful for debugging.The static analysis tool flagged a potential issue with global variable reassignment. Consider using a different approach to handle the
onmessageevent, such as:self.onmessage = async (e) => { // ... existing code ... };This avoids reassigning the global
onmessageproperty and resolves the static analysis warning.🧰 Tools
🪛 Biome
[error] 56-56: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 90-92: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
Line range hint
1-116: Overall approval with suggestions for improvementThe changes in this file successfully implement support for foreign assets on asset hubs, aligning well with the PR objectives. The code structure and organization have been improved, enhancing maintainability.
To further improve the code quality:
- Address all
@ts-ignorecomments by either providing explanations or fixing the underlying TypeScript issues.- Consider adding more error handling and logging throughout the functions to improve debugging and resilience.
- If possible, add unit tests for the new
getAssetsfunction and the updatedgetAssetOnAssetHubfunction to ensure their correctness and prevent regressions.🧰 Tools
🪛 Biome
[error] 56-56: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 90-92: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
packages/extension-polkagate/src/hooks/useBalances.ts (1)
52-52: Consider extending balance saving logic for non-native assetsThe TODO comment suggests that the current implementation only saves native assets in local storage. To improve the functionality, consider extending this logic to handle non-native assets as well.
Would you like assistance in implementing the logic for saving non-native asset balances? This could involve:
- Modifying the
savedBalancesstructure to accommodate multiple assets per chain.- Updating the balance saving logic to handle both native and non-native assets.
- Adjusting the balance retrieval logic elsewhere in the codebase to work with the new storage structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/extension-polkagate/src/hooks/useBalances.ts (2 hunks)
- packages/extension-polkagate/src/hooks/useNativeAssetBalances.ts (1 hunks)
- packages/extension-polkagate/src/popup/home/news.ts (1 hunks)
- packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-polkagate/src/hooks/useNativeAssetBalances.ts
- packages/extension-polkagate/src/popup/home/news.ts
🧰 Additional context used
🪛 Biome
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js
[error] 90-92: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
🔇 Additional comments (6)
packages/extension-polkagate/src/hooks/useBalances.ts (6)
5-14: LGTM: Import updates and function signature changeThe changes to the imports and the function signature look good. The addition of new hooks and the change in the
assetIdparameter type tostring | numberimprove the flexibility and modularity of the code.
21-26: LGTM: Improved balance retrieval logicThe introduction of separate hooks for different balance retrieval scenarios (
useNativeAssetBalances,useBalancesOnAssethub,useBalancesOnMultiAssetChain) improves the modularity and clarity of the code. The handling of native and non-native asset balances is well-structured.
79-83: LGTM: Proper handling of non-native asset balancesThe added logic for handling non-native asset balances is concise and correct. It ensures that the appropriate balance is returned based on the
assetIdprovided.
Line range hint
85-89: LGTM: Improved return logicThe updated return logic effectively handles different scenarios, ensuring that the most appropriate balance information is returned. The use of optional chaining and multiple conditions provides a robust and safe way to access and compare properties.
18-19:⚠️ Potential issueHandle
undefinedassetIdto prevent unintended comparisonsWhen
assetIdisundefined,String(assetId)evaluates to'undefined', which may cause incorrect comparisons withNATIVE_TOKEN_ASSET_ID. Consider explicitly checking forundefinedbefore converting toString.Suggested fix:
-const isNativeAssetId = String(assetId) === String(NATIVE_TOKEN_ASSET_ID) || String(assetId) === String(NATIVE_TOKEN_ASSET_ID_ON_ASSETHUB); +const isNativeAssetId = assetId === undefined || String(assetId) === String(NATIVE_TOKEN_ASSET_ID) || String(assetId) === String(NATIVE_TOKEN_ASSET_ID_ON_ASSETHUB);
38-38: 🛠️ Refactor suggestionSimplify type casting for
soloTotalThe casting
stakingAccount?.stakingLedger?.total as unknown as BNmay be unnecessary or can be simplified. Iftotalis already of typeBN, additional casting may not be required.Consider updating the assignment:
-soloTotal: stakingAccount?.stakingLedger?.total as unknown as BN +soloTotal: stakingAccount?.stakingLedger?.totalIf
totalis not of typeBN, adjust the casting appropriately.
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (3)
103-114: LGTM! Clear asset type categorization.The new variables provide a clear and efficient way to categorize and handle different asset types. The use of
useMemoforparsedAssetIdis a good practice for performance optimization.Consider simplifying the
noAssetIdcheck:-const noAssetId = assetId === undefined || assetId === 'undefined'; +const noAssetId = !assetId || assetId === 'undefined';This change maintains the existing behavior while being more concise.
172-199: LGTM! Comprehensive module selection logic.The updated
onChainCalllogic now handles different asset types and modules more effectively, improving the overall functionality of the component.Consider improving the error logging in the catch block:
-console.log('Something wrong while making on chain call!', e); +console.error('Error determining on-chain call module:', e);This provides more context and uses the appropriate console method for errors.
360-374: LGTM! Improved maximum amount calculation.The updated
setWholeAmountfunction now correctly handles both native and non-native tokens when calculating the maximum transferable amount. This ensures more accurate calculations across different token types.Consider adding a safety check to ensure the result is not negative:
-const allAmount = canNotTransfer ? '0' : amountToHuman(transferableBalance.sub(_maxFee).toString(), balances.decimal); +const balanceAfterFee = transferableBalance.sub(_maxFee); +const allAmount = canNotTransfer || balanceAfterFee.isNeg() ? '0' : amountToHuman(balanceAfterFee.toString(), balances.decimal);This prevents potential issues with negative amounts in edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (11 hunks)
- packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (2 hunks)
🧰 Additional context used
🪛 Biome
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js
[error] 59-59: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 96-98: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
🔇 Additional comments (8)
packages/extension-polkagate/src/util/workers/getAssetOnAssetHub.js (4)
6-6: LGTM: New import for foreign asset supportThe addition of
decodeMultiLocationimport aligns with the PR objectives of adding foreign assets support. This function will be useful for processing foreign asset IDs in the new implementation.
99-104: LGTM: Improved error handling inonmessagehandlerThe changes to the
onmessageevent handler improve the robustness of the code by providing a default empty array forassetsToBeFetchedwhen it's not specified. The added warning message is helpful for debugging purposes.
Line range hint
67-92: LGTM: UpdatedgetAssetOnAssetHubfunctionThe changes to
getAssetOnAssetHubfunction effectively incorporate the newgetAssetsfunction for handling non-native assets, aligning with the PR objectives.Query: Purpose of commented-out code
There's a block of commented-out code (lines 81-90) that seems to be for calculating new foreign token asset IDs. Could you provide more context on this code? Is it intended for future use or debugging purposes? If it's not needed, consider removing it to keep the codebase clean.
#!/bin/bash # Check if the commented-out code is used elsewhere in the project rg "const allForeignAssets = await api\.query\.foreignAssets\.asset\.entries\(\)" --type jsThis will help determine if similar functionality exists elsewhere in the project or if this code is unique and potentially valuable for future use.
🧰 Tools
🪛 Biome
[error] 96-98: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
10-65: LGTM: NewgetAssetsfunction implementationThe new
getAssetsfunction effectively implements support for both foreign and native assets, aligning with the PR objectives. It improves code organization by separating asset processing logic from the maingetAssetOnAssetHubfunction.However, there are a few points to consider:
Error Handling: The current error handling is quite broad. Consider adding more specific error messages or handling different types of errors separately to aid in debugging.
TypeScript Ignores: There are multiple
@ts-ignorecomments in the function. It would be beneficial to address these TypeScript issues properly instead of ignoring them. Can you provide more context on why these are necessary or consider fixing the underlying type issues?Consider refactoring the error handling to provide more specific error messages:
try { // ... existing code ... } catch (e) { if (e instanceof ApiError) { console.error('API error while fetching assets:', e.message); } else if (e instanceof NetworkError) { console.error('Network error while fetching assets:', e.message); } else { console.error('Unexpected error while fetching assets:', e); } throw e; // Re-throw the error to be handled by the caller }To address the TypeScript issues, let's investigate the types causing problems:
This will help us understand the correct types to use and potentially remove the
@ts-ignorecomments.🧰 Tools
🪛 Biome
[error] 59-59: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/extension-polkagate/src/fullscreen/sendFund/InputPage.tsx (4)
9-9: LGTM! Improved asset handling capabilities.The new imports and type changes enhance the component's ability to handle various asset types, including foreign assets and native tokens across different chains. This is a positive improvement that increases the flexibility and robustness of the code.
Also applies to: 25-26, 34-34
Line range hint
135-153: LGTM! Improved warning message logic.The updated
warningMessagelogic now correctly handles different scenarios for both native and non-native tokens. This enhancement provides more accurate warnings to users, improving the overall user experience and error handling.
157-163: LGTM! Improved teleport destination handling.The updated logic in
destinationGenesisHashesnow correctly considersisNativeTokenwhen determining teleport destinations. This ensures that teleport options are only provided for native tokens, preventing potential issues with non-native token transfers.
Line range hint
306-318: LGTM! Improved input handling for different token types.The updated logic in the
useEffecthook now correctly handles different scenarios for native and non-native tokens when setting inputs. The use ofparsedAssetIdfor asset hub transfers ensures that the correct asset ID format is used, improving the overall reliability of the transfer process.
# [0.18.0](v0.17.0...v0.18.0) (2024-10-06) ### Features * add foreign assets support on asset hubs, such as MYTH on PAH ([#1577](#1577)) ([a77459a](a77459a))
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor