Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support Aleph Zero #1576

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: support Aleph Zero #1576

wants to merge 3 commits into from

Conversation

Nick-1979
Copy link
Member

@Nick-1979 Nick-1979 commented Oct 1, 2024

plus its staking

Summary by CodeRabbit

  • New Features
    • Enhanced handling of recipient addresses in the Send component for improved type clarity.
    • Introduced support for the Aleph Zero blockchain, including new constants and network handling.
  • Improvements
    • Streamlined prop handling in various components for better type safety and clarity.
    • Improved UI logic for network selection and validator display components.
  • Bug Fixes
    • Enhanced conditional rendering in the ShowValidator component to prevent runtime errors.
  • Chores
    • Code organization improvements, including removal of TypeScript checks and reordering of import statements across multiple files.

plus its staking
@Nick-1979 Nick-1979 added the WIP Work In Progress label Oct 1, 2024
@Nick-1979 Nick-1979 self-assigned this Oct 1, 2024
Copy link
Contributor

coderabbitai bot commented Oct 1, 2024

Warning

Rate limit exceeded

@Nick-1979 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 39 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Files that changed from the base of the PR and between 350d1af and ab04780.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (23)
packages/extension-polkagate/src/popup/staking/solo/stake/partials/util.tsx (1)

6-6: Consider adhering to consistent function declaration style.

The function signature has been updated to include a space between the function name and the opening parenthesis. While this change doesn't affect functionality, it's important to maintain consistent coding style throughout the project.

Please verify if this style change aligns with the project's coding guidelines. If not, consider reverting to the previous style:

-export default function getPayee (settings: SoloSettings): string | undefined {
+export default function getPayee(settings: SoloSettings): string | undefined {
packages/extension-polkagate/src/util/api/getPrices.ts (2)

20-22: Consider the implications of using toLocaleLowerCase()

The change from toLowerCase() to toLocaleLowerCase() in the revisedPriceIds calculation is notable. While toLocaleLowerCase() is more locale-sensitive, which can be beneficial in some cases, it's worth considering that:

  1. Chain names and price IDs are typically in ASCII, where toLowerCase() would suffice.
  2. toLowerCase() is generally faster for simple ASCII strings.

Given the context, the performance difference is likely negligible, but it's worth ensuring that this change is intentional and aligns with the project's localization strategy.

Would you like to discuss the rationale behind this change or revert to toLowerCase() for consistency with other parts of the codebase?


Line range hint 5-5: Consider addressing TypeScript issues

The presence of @ts-nocheck at the top of the file suggests that there are TypeScript type-checking issues that have been suppressed. While this doesn't directly relate to the current changes, it's generally a good practice to address these issues to improve code quality and catch potential errors early.

Consider investigating and resolving the TypeScript issues in this file. If you need assistance in identifying or fixing these issues, please let me know.

packages/extension-polkagate/src/popup/staking/solo/stake/Settings.tsx (1)

6-7: LGTM! Consider grouping related imports.

The relocation of the import statement for SoloSettings and StakingConsts improves code organization. To further enhance readability, consider grouping related imports together.

You could group the imports as follows:

import type { SoloSettings, StakingConsts } from '../../../../util/types';

import React, { useCallback } from 'react';
import { Close as CloseIcon } from '@mui/icons-material';
import { Grid, IconButton } from '@mui/material';

import { SlidePopUp } from '../../../../components';
import SetPayeeController from './partials/SetPayeeController';
import SettingsHeader from './partials/SettingsHeader';
packages/extension-polkagate/src/util/workers/getStakingConsts.js (3)

7-13: LGTM! Consider adding a comment explaining the variations.

The MAX_NOMINATIONS constant is a good addition, providing clear maximum nomination values for different tokens, including AZERO (Aleph Zero) as per the PR objective.

Consider adding a brief comment explaining why the maximum nominations vary significantly between tokens (e.g., 16 for DOT, 1 for AZERO). This would help other developers understand the rationale behind these differences.


23-31: LGTM! Consider adding error handling for unexpected tokens.

The changes improve the function's robustness and accuracy:

  1. The new logic for maxNominations correctly uses the MAX_NOMINATIONS constant with a fallback.
  2. Null checks for epochDuration and expectedBlockTime enhance error handling.
  3. The unbondingDuration calculation is now more precise.

Consider adding a warning log when an unexpected token is encountered. This could help identify issues with new or unsupported chains:

if (!MAX_NOMINATIONS[token]) {
  console.warn(`Unexpected token: ${token}. Using default max nominations.`);
}

Line range hint 1-62: Overall, excellent changes that meet the PR objectives.

The modifications successfully introduce support for Aleph Zero (AZERO) while improving the overall flexibility and robustness of the getStakingConsts function. The new constants and logic changes align well with the PR's goals and maintain consistency with the existing codebase.

As the number of supported chains grows, consider refactoring the chain-specific constants (like MAX_NOMINATIONS) into a separate configuration file. This would improve maintainability and make it easier to add support for new chains in the future.

packages/extension-polkagate/src/hooks/useEndpoints.ts (2)

40-42: Improved endpoint matching logic looks good!

The addition of an exact match condition after removing whitespace enhances the accuracy of endpoint selection. This change will help prevent false positive matches for chain names that are substrings of others.

Consider extracting the text normalization into a separate function for improved readability:

const normalizeText = (text: string) => text.replace(/\s/g, '').toLowerCase();

// Then use it in the filter:
normalizeText(String(e.text)) === normalizeText(chainName)

This would make the logic more explicit and easier to maintain.


Line range hint 1-68: Overall, the changes improve endpoint matching accuracy.

The modifications to the filtering logic in the useEndpoints hook enhance the precision of endpoint selection based on chain names. The addition of an exact match condition after whitespace removal complements the existing partial match logic, reducing the likelihood of false positive matches for similar chain names.

Consider adding a unit test for this hook to verify the behavior with various chain names, including edge cases with whitespace and similar names. This would ensure the robustness of the matching logic across different scenarios.

packages/extension-polkagate/src/components/AccountInputWithIdentity.tsx (2)

21-21: Improved prop type for setAddress

The updated type for setAddress is more aligned with React's state management practices and provides better flexibility by making it optional. This change improves type safety and component reusability.

Consider adding a comment explaining the purpose of this prop and when it might be omitted, to improve code documentation:

/** Function to update the address state. If omitted, the component will be read-only. */
setAddress?: React.Dispatch<React.SetStateAction<string | undefined | null>>;

39-41: Improved type safety and readability

The removal of type assertions enhances type safety, aligning with the updated prop types. The formatting changes in the Identity component improve code readability.

Consider extracting the inline styles for the Identity component into a separate constant or using a custom theme to improve maintainability:

const identityStyles = {
  height: '51px',
  maxWidth: '100%',
  width: 'fit-content'
};

// In the JSX
<Identity
  api={api}
  chain={chain}
  formatted={address}
  identiconSize={31}
  name={name}
  style={identityStyles}
/>

Also applies to: 53-57

packages/extension-polkagate/src/fullscreen/stake/solo/partials/ShowValidator.tsx (2)

Line range hint 82-102: Improved robustness in handling staked amount display.

The changes enhance the component's ability to handle potentially undefined values, preventing runtime errors. The fallback mechanism is well-implemented.

For consistency, consider using the same isHexToBn().gt(BN_ZERO) check for both v.exposure.total and v.stakingLedger.total. This would make the logic more uniform:

{isHexToBn(v.exposure?.total?.toString() ?? '0').gt(BN_ZERO)
  ? <ShowBalance
      api={api}
      balance={v.exposure.total}
      decimal={decimal}
      decimalPoint={1}
      height={15}
      skeletonWidth={50}
      token={token}
    />
  : isHexToBn(v.stakingLedger?.total?.toString() ?? '0').gt(BN_ZERO)
    ? <ShowBalance
        api={api}
        balance={isHexToBn(v.stakingLedger.total.toString())}
        decimal={decimal}
        decimalPoint={1}
        height={15}
        skeletonWidth={50}
        token={token}
      />
    : t('waiting')
}

This approach uses optional chaining and nullish coalescing to safely access nested properties and provide a default value for the isHexToBn function.


Line range hint 1-146: Overall improvement in component robustness and error handling.

The changes in this file significantly enhance the ShowValidator component's ability to handle various data scenarios. The improved conditional rendering and safe property access make the component more resilient to potential runtime errors. These modifications align well with React best practices and contribute to a more stable user experience.

Consider implementing a custom hook or utility function to handle the complex logic for determining the staked amount and nominator count. This could improve code reusability and make the component easier to maintain in the future.

packages/extension-polkagate/src/fullscreen/stake/solo/partials/ValidatorsTableFS.tsx (1)

63-63: Consistent use of optional chaining and potential refactor

The optional chaining (?.) is correctly applied to v.exposure?.others?.length, maintaining consistency with previous changes. This improves null safety throughout the function.

Consider refactoring to reduce code duplication:

const othersLength = v.exposure?.others?.length;
return {
  notSafe: othersLength > threshold && (maybeMyIndex > threshold || maybeMyIndex === -1),
  safe: othersLength > threshold && (maybeMyIndex < threshold || maybeMyIndex === -1)
};

This refactoring would make the code more DRY and easier to maintain.

packages/extension-polkagate/src/util/constants.tsx (2)

54-55: LGTM: Addition of Aleph Zero genesis hashes

The addition of ALEPH_ZERO_GENESIS_HASH and ALEPH_ZERO_TESTNET_GENESIS_HASH constants is crucial for supporting Aleph Zero networks. These constants will be used to identify and interact with both the mainnet and testnet of Aleph Zero.

Consider adding a comment above these constants to briefly explain their purpose, similar to other sections in this file. For example:

/** Aleph Zero network info */
export const ALEPH_ZERO_GENESIS_HASH = '0x70255b4d28de0fc4e1a193d7e175ad1ccef431598211c55538f1018651a0344e';
export const ALEPH_ZERO_TESTNET_GENESIS_HASH = '0x05d5279c52c484cc80396535a316add7d47b1c5b9e0398dd1f584149341460c5';

114-116: LGTM: Update to STAKING_CHAINS array

The update to the STAKING_CHAINS array appropriately includes all relay chains and both Aleph Zero networks (mainnet and testnet). This change aligns with the PR objective of supporting Aleph Zero and its staking functionality.

Consider adding a comment to explain the inclusion of both Aleph Zero networks, especially the testnet, in the staking chains. For example:

export const STAKING_CHAINS = [
  ...RELAY_CHAINS_GENESISHASH,
  ALEPH_ZERO_GENESIS_HASH,
  ALEPH_ZERO_TESTNET_GENESIS_HASH // Including testnet for development and testing purposes
];
packages/extension-polkagate/src/partials/FullScreenChainSwitch.tsx (6)

Line range hint 66-70: Disable onClick handler for non-selectable testnets

Currently, the onClick handler is attached to all network items, including those that are disabled (i.e., testnets when testnet usage is not enabled). This may lead to confusing user experience because clicking on a disabled item still triggers the onClick event, even though it performs no action due to the early return in selectNetwork.

To improve user experience and prevent unintended behavior, conditionally attach the onClick handler only to selectable networks.

Apply this diff to update the code:

- <Grid container justifyContent='space-between' key={index} onClick={() => selectNetwork(network)} sx={{ ':hover': { bgcolor: theme.palette.mode === 'light' ? 'rgba(24, 7, 16, 0.1)' : 'rgba(255, 255, 255, 0.1)' }, bgcolor: selectedNetwork ? 'rgba(186, 40, 130, 0.2)' : 'transparent', cursor: isTestnetDisabled(network.value as string) ? 'not-allowed' : 'pointer', height: '45px', opacity: isTestnetDisabled(network.value as string) ? 0.3 : 1, px: '15px' }}>
+ <Grid container justifyContent='space-between' key={index} onClick={!isTestnetDisabled(network.value as string) ? () => selectNetwork(network) : undefined} sx={{ ':hover': { bgcolor: theme.palette.mode === 'light' ? 'rgba(24, 7, 16, 0.1)' : 'rgba(255, 255, 255, 0.1)' }, bgcolor: selectedNetwork ? 'rgba(186, 40, 130, 0.2)' : 'transparent', cursor: isTestnetDisabled(network.value as string) ? 'not-allowed' : 'pointer', height: '45px', opacity: isTestnetDisabled(network.value as string) ? 0.3 : 1, px: '15px' }}>

Line range hint 99-102: Clean up setTimeout to prevent potential memory leaks

In the useEffect, the setTimeout is not being cleared when the component unmounts or before the next effect runs, which could lead to memory leaks or unexpected behavior if the component unmounts before the timeout completes.

To prevent this, store the timeout ID and clear it in a cleanup function.

Apply this diff to modify the code:

 useEffect(() => {
     setFlip(true);
-    setTimeout(() => setFlip(false), 1000);
+    const timeoutId = setTimeout(() => setFlip(false), 1000);
+    return () => clearTimeout(timeoutId);
 }, [selectedChainName]);

Line range hint 41-45: Include sanitizeChainName in useCallback dependencies

The sanitizedLowercase function uses sanitizeChainName, which is imported from '../util/utils'. While it's unlikely to change, for correctness, it should be included in the dependency array of the useCallback hook.

Apply this diff to update the dependencies:

 const sanitizedLowercase = useCallback((text: string) => sanitizeChainName(text)?.toLowerCase(), [
+  sanitizeChainName
 ]);

Line range hint 125-154: Enhance accessibility by using appropriate HTML elements

The Grid component is being used with component='button' to render a button element. However, additional accessibility attributes might be needed to ensure proper semantics, such as type="button" and handling keyboard interactions.

Apply this diff to include the type attribute and ensure accessibility:

 <Grid
   aria-describedby={id}
-  component='button'
+  component='button'
+  type='button'
   container
   item
   onClick={handleClick}
   sx={{ bgcolor: 'transparent', border: '1px solid', borderColor: 'secondary.main', borderRadius: '50%', cursor: 'pointer', height: '40px', p: 0, width: '40px' }}
 >

Additionally, consider verifying that all interactive elements are keyboard accessible and have appropriate focus styles.


Line range hint 12-14: Update year in copyright

The file header comments specify the year range as 2019-2024. Since the current year is 2024, consider updating it if necessary.


Line range hint 131-139: Simplify conditional rendering of ChainLogo

The ChainLogo component is conditionally rendered based on selectedChainName. Since there's an explicit check, you can simplify the JSX by using conditional rendering directly.

Apply this diff to simplify the code:

-{selectedChainName &&
-  <Box
-    sx={{
-      ...
-    }}
-  >
-    <ChainLogo chainName={selectedChainName} genesisHash={genesisHash} size={34} />
-  </Box>}
+<Box
+  sx={{
+    display: selectedChainName ? 'block' : 'none',
+    ...
+  }}
+>
+  <ChainLogo chainName={selectedChainName} genesisHash={genesisHash} size={34} />
+</Box>

Alternatively, ensure that selectedChainName is always defined to avoid unnecessary checks.

replacements/interfaces.js (1)

139-149: Standardize the URL casing in the website field

The website field for the aleph network uses mixed casing in the URL (https://alephZero.org). For consistency and to prevent potential issues, it's recommended to use lowercase letters in URLs.

Apply this diff to standardize the URL:

{
    "prefix": 42,
    "network": "aleph",
    "displayName": "Aleph Zero",
    "symbols": ["AZERO"],
    "decimals": [12],
    "standardAccount": "*25519",
-   "website": "https://alephZero.org"
+   "website": "https://alephzero.org"
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 350d1af and 10bc136.

📒 Files selected for processing (28)
  • packages/extension-polkagate/src/components/AccountInputWithIdentity.tsx (3 hunks)
  • packages/extension-polkagate/src/fullscreen/stake/partials/DisplayBalance.tsx (4 hunks)
  • packages/extension-polkagate/src/fullscreen/stake/solo/StakedSolo.tsx (2 hunks)
  • packages/extension-polkagate/src/fullscreen/stake/solo/commonTasks/configurePayee/index.tsx (1 hunks)
  • packages/extension-polkagate/src/fullscreen/stake/solo/partials/SetPayee.tsx (5 hunks)
  • packages/extension-polkagate/src/fullscreen/stake/solo/partials/ShowValidator.tsx (3 hunks)
  • packages/extension-polkagate/src/fullscreen/stake/solo/partials/ValidatorsTableFS.tsx (1 hunks)
  • packages/extension-polkagate/src/hooks/index.ts (0 hunks)
  • packages/extension-polkagate/src/hooks/useApiWithChain2.ts (2 hunks)
  • packages/extension-polkagate/src/hooks/useEndpoints.ts (1 hunks)
  • packages/extension-polkagate/src/hooks/useIdentity.ts (1 hunks)
  • packages/extension-polkagate/src/hooks/useNativeTokenPrice.ts (0 hunks)
  • packages/extension-polkagate/src/hooks/usePeopleChain.ts (3 hunks)
  • packages/extension-polkagate/src/hooks/useValidatorSuggestion.ts (1 hunks)
  • packages/extension-polkagate/src/partials/FullScreenChainSwitch.tsx (1 hunks)
  • packages/extension-polkagate/src/popup/send/index.tsx (2 hunks)
  • packages/extension-polkagate/src/popup/staking/solo/stake/Settings.tsx (1 hunks)
  • packages/extension-polkagate/src/popup/staking/solo/stake/partials/RewardDestination.tsx (1 hunks)
  • packages/extension-polkagate/src/popup/staking/solo/stake/partials/SetPayeeController.tsx (6 hunks)
  • packages/extension-polkagate/src/popup/staking/solo/stake/partials/util.tsx (2 hunks)
  • packages/extension-polkagate/src/util/api/getPrices.ts (1 hunks)
  • packages/extension-polkagate/src/util/constants.tsx (4 hunks)
  • packages/extension-polkagate/src/util/getLogo2.ts (1 hunks)
  • packages/extension-polkagate/src/util/types.ts (1 hunks)
  • packages/extension-polkagate/src/util/workers/getStakingConsts.js (1 hunks)
  • packages/extension-polkagate/src/util/workers/getValidatorsInfo.js (1 hunks)
  • packages/extension-polkagate/src/util/workers/utils/getChainEndpoints.js (1 hunks)
  • replacements/interfaces.js (3 hunks)
💤 Files with no reviewable changes (2)
  • packages/extension-polkagate/src/hooks/index.ts
  • packages/extension-polkagate/src/hooks/useNativeTokenPrice.ts
✅ Files skipped from review due to trivial changes (2)
  • packages/extension-polkagate/src/popup/staking/solo/stake/partials/RewardDestination.tsx
  • packages/extension-polkagate/src/util/workers/getValidatorsInfo.js
🧰 Additional context used
🪛 GitHub Check: build
packages/extension-polkagate/src/util/getLogo2.ts

[failure] 47-47:
Property 'replace' does not exist on type 'string | number | boolean | ReactElement<any, string | JSXElementConstructor> | Iterable | ReactPortal'.

packages/extension-polkagate/src/util/workers/utils/getChainEndpoints.js

[failure] 20-20:
Property 'replace' does not exist on type 'string | number | boolean | ReactElement<any, string | JSXElementConstructor> | Iterable | ReactPortal'.

🔇 Additional comments (60)
packages/extension-polkagate/src/popup/staking/solo/stake/partials/util.tsx (1)

Line range hint 1-18: Overall assessment: Function logic is sound, but minor improvements suggested.

The core functionality of the getPayee function appears to be correct and unchanged. However, there are two areas for potential improvement:

  1. Ensure consistent function declaration style across the project.
  2. Address the use of @ts-ignore by resolving the underlying type issue.

These changes will enhance code quality, maintainability, and type safety without altering the function's behavior.

packages/extension-polkagate/src/util/workers/utils/getChainEndpoints.js (1)

16-23: ⚠️ Potential issue

Approve changes with suggestions for improvement

The changes to the filtering logic enhance the flexibility of endpoint matching, which is beneficial for supporting various endpoint configurations. However, there are a few points to address:

  1. The additional check for the text property is a good addition, but it introduces a potential type safety issue.
  2. The complex condition could be refactored for improved readability.

To address the type safety issue and improve readability, consider the following refactoring:

-    .filter((endpoint) =>
-      endpoint.info &&
-      (
-        endpoint.info.toLowerCase() === chainName.toLowerCase() ||
-        endpoint.text?.replace(/\s/g, '')?.toLowerCase() === chainName.toLowerCase()
-      ) &&
-      !endpoint.isDisabled && !endpoint?.isLightClient
-    );
+    .filter((endpoint) => {
+      const normalizedChainName = chainName.toLowerCase();
+      const matchesInfo = endpoint.info?.toLowerCase() === normalizedChainName;
+      const matchesText = typeof endpoint.text === 'string' && endpoint.text.replace(/\s/g, '').toLowerCase() === normalizedChainName;
+      
+      return (matchesInfo || matchesText) && !endpoint.isDisabled && !endpoint.isLightClient;
+    });

This refactoring:

  1. Addresses the type safety issue by explicitly checking if endpoint.text is a string.
  2. Improves readability by breaking down the condition into smaller, named parts.
  3. Removes the unnecessary optional chaining on isLightClient.

To ensure that this change doesn't affect other parts of the codebase, please run the following script:

This will help identify if there are other places in the codebase where endpoint.text is used, allowing us to maintain consistency in type handling.

✅ Verification successful

Changes Approved with Verified Type Safety

The additional check for the text property is only present in getChainEndpoints.js. The verification confirms that there are no other usages of endpoint.text in the codebase, ensuring type safety is maintained. The proposed refactoring improves readability and safeguards against potential type issues without affecting other parts of the code.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other usages of 'endpoint.text' to ensure consistency

rg --type js --type ts 'endpoint\.text' -C 3

Length of output: 1507

🧰 Tools
🪛 GitHub Check: build

[failure] 20-20:
Property 'replace' does not exist on type 'string | number | boolean | ReactElement<any, string | JSXElementConstructor> | Iterable | ReactPortal'.

packages/extension-polkagate/src/util/api/getPrices.ts (2)

13-13: LGTM: Addition of Aleph Zero mapping

The addition of alephzero: 'aleph-zero' to the EXTRA_PRICE_IDS object is correct and aligns with the PR objective of supporting Aleph Zero. This mapping will ensure that the price data for Aleph Zero can be correctly fetched and processed.


Line range hint 1-43: Overall assessment: Changes look good

The modifications to add support for Aleph Zero are minimal, focused, and align well with the PR objectives. The changes effectively integrate Aleph Zero into the existing price fetching mechanism without introducing any apparent issues.

However, consider addressing the following points for further improvement:

  1. The rationale behind using toLocaleLowerCase() instead of toLowerCase().
  2. The TypeScript issues that led to the use of @ts-nocheck.

These suggestions aim to enhance code consistency and type safety but don't impede the main objective of supporting Aleph Zero.

packages/extension-polkagate/src/util/getLogo2.ts (1)

47-47: ⚠️ Potential issue

Address potential type issue with text.replace()

The current implementation assumes that text is always a string when defined, which may not be the case according to the type definition. This could lead to runtime errors if text is of a different type.

To resolve this issue and improve type safety, consider the following options:

  1. Add a type guard:
(typeof text === 'string' && text.replace(/\s/g, '').toLowerCase() === iconName)
  1. Use a type assertion (if you're certain text will always be a string when defined):
(text as string)?.replace(/\s/g, '').toLowerCase() === iconName
  1. Convert text to a string explicitly:
String(text).replace(/\s/g, '').toLowerCase() === iconName

Option 1 is the safest approach as it ensures text is a string before calling replace. Choose the most appropriate solution based on your knowledge of the text property in the endpoints array.

To help determine the most appropriate solution, let's verify the type and usage of text in the codebase:

🧰 Tools
🪛 GitHub Check: build

[failure] 47-47:
Property 'replace' does not exist on type 'string | number | boolean | ReactElement<any, string | JSXElementConstructor> | Iterable | ReactPortal'.

packages/extension-polkagate/src/popup/staking/solo/stake/Settings.tsx (2)

Line range hint 1-63: Overall assessment: Good improvements, but limited scope.

The changes in this file are positive, improving code organization and enabling TypeScript checks. However, they don't directly contribute to the PR's main objective of supporting Aleph Zero and its staking. Consider reviewing other files in the PR that might contain more substantial changes related to Aleph Zero integration.

To ensure we haven't missed any Aleph Zero-related changes in this file, let's run a quick check:

#!/bin/bash
# Description: Check for any mentions of Aleph Zero in the Settings.tsx file

# Test: Search for "Aleph" or "AZERO" (Aleph Zero's token) in the file
rg -i "aleph|azero" packages/extension-polkagate/src/popup/staking/solo/stake/Settings.tsx

# If no results are found, it confirms that this file doesn't contain direct references to Aleph Zero.

1-5: Great improvement! Enable TypeScript checks.

Removing the @ts-nocheck directive is a positive step towards improving code quality by enabling TypeScript type checking for this file.

To ensure this change doesn't introduce any new type errors, please run the following command:

If any type errors are found, please address them to fully benefit from TypeScript's type checking capabilities.

packages/extension-polkagate/src/util/workers/getStakingConsts.js (2)

15-15: LGTM! Good fallback value.

The DEFAULT_MAX_NOMINATIONS constant provides a sensible fallback value, matching the maximum nominations for DOT and WND.


Line range hint 41-41: LGTM! Improved clarity in unbondingDuration calculation.

The return object structure is maintained, ensuring compatibility with existing code. The unbondingDuration calculation now uses epochDurationInHours, which improves clarity and accuracy.

packages/extension-polkagate/src/components/AccountInputWithIdentity.tsx (3)

6-7: LGTM: Improved imports and readability

The addition of the Chain type import and the empty line for separation improves type checking and code readability.


28-28: LGTM: Minor formatting improvement

The slight formatting change in the function signature improves readability without affecting functionality.


Line range hint 1-63: Summary: Improved type safety and component flexibility

The changes in this file enhance type safety, improve readability, and increase the flexibility of the AccountInputWithIdentity component. While there are no direct references to Aleph Zero, these improvements support better integration of different chains, aligning with the PR's objectives.

Key improvements:

  1. More specific and flexible prop types
  2. Removal of unnecessary type assertions
  3. Better code formatting and readability

These changes contribute to a more robust and maintainable codebase.

packages/extension-polkagate/src/hooks/useValidatorSuggestion.ts (2)

67-67: Improved null safety in validator filtering logic

The modification to use optional chaining (v.exposure?.others?.length) is a good improvement. This change makes the code more robust by safely handling cases where v.exposure might be undefined, preventing potential runtime errors.

This enhancement aligns with best practices for handling potentially undefined properties in JavaScript/TypeScript.


Line range hint 1-95: Summary of changes in useValidatorSuggestion hook

The modification in this file is minimal but valuable. The core functionality of the useValidatorSuggestion hook remains intact, with the only change being an improvement in the null safety of the validator filtering logic. This enhancement makes the code more robust without altering its primary purpose or behavior.

To ensure the change doesn't introduce any unintended side effects, consider adding or updating unit tests to cover scenarios where v.exposure might be undefined.

packages/extension-polkagate/src/fullscreen/stake/solo/partials/SetPayee.tsx (8)

6-7: LGTM: Improved type imports

The addition of specific type imports from '@mui/material' and the custom types file enhances type safety and code clarity. This change aligns with best practices for TypeScript usage in React components.


23-23: LGTM: Consistent function signature style

The adjustment in the function signature improves code consistency by adding spaces around the destructured props. This change aligns with common JavaScript/TypeScript style guidelines.


69-69: LGTM: Improved component flexibility

The changes to the Warn component enhance its usability:

  1. Making the style parameter optional with a default value increases flexibility.
  2. Reordering the parameters is a minor style improvement that maintains consistency with common React patterns.

These changes make the component more adaptable to different use cases without affecting its core functionality.


95-95: LGTM: Improved style property organization

The rearrangement of style properties in the Skeleton component improves code readability. This change maintains the same visual output while organizing the properties in a more logical order.


43-43: LGTM: Consistent nullable type usage

The change to allow null values for the account parameter in makePayee function aligns with the earlier modification to the rewardDestinationAccount state. This consistency improves type safety and allows for more explicit handling of cases where no account is selected.

Please verify that the function's logic correctly handles the case when account is null. Run the following command to check the makePayee function implementation:

#!/bin/bash
# Check makePayee function implementation
rg --type typescript -A 10 'const makePayee' packages/extension-polkagate/src/fullscreen/stake/solo/partials/SetPayee.tsx

104-104: LGTM: Added chain prop to AccountInputWithIdentity

The addition of the chain prop to the AccountInputWithIdentity component suggests an improvement in chain-specific account handling. This change likely enhances the component's ability to validate or display accounts based on the specific blockchain being used.

Please verify that the AccountInputWithIdentity component correctly utilizes the chain prop. Run the following command to check its implementation:

#!/bin/bash
# Check AccountInputWithIdentity component implementation
rg --type typescript 'function AccountInputWithIdentity' packages/extension-polkagate/src/components

35-36: LGTM: Improved event handling and type casting

The changes in the onSelectionMethodChange function improve code quality:

  1. Prefixing the unused event parameter with an underscore follows best practices.
  2. Accepting a string value and then casting it to the union type allows for more flexible input handling.

Please verify that the RadioGroup component in the render method is correctly passing string values that match the expected union type ('Staked' | 'Others'). Run the following command to check the RadioGroup implementation:

#!/bin/bash
# Check RadioGroup implementation
rg --type typescript '<RadioGroup' packages/extension-polkagate/src/fullscreen/stake/solo/partials/SetPayee.tsx

31-31: LGTM: Improved state type definition

The change to allow null values for rewardDestinationAccount enables more explicit handling of the unset state. This improvement aligns with TypeScript best practices for nullable values.

Please verify that this change is correctly handled throughout the component, especially in the makePayee function and any conditional rendering that depends on this state.

✅ Verification successful

LGTM: Improved state type definition

The change to allow null values for rewardDestinationAccount enables more explicit handling of the unset state. This improvement aligns with TypeScript best practices for nullable values.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the usage of rewardDestinationAccount
rg --type typescript 'rewardDestinationAccount' packages/extension-polkagate/src/fullscreen/stake/solo/partials/SetPayee.tsx

Length of output: 415


Script:

#!/bin/bash
# Verify the usage of rewardDestinationAccount in SetPayee.tsx
rg --type tsx 'rewardDestinationAccount' packages/extension-polkagate/src/fullscreen/stake/solo/partials/SetPayee.tsx

Length of output: 145


Script:

#!/bin/bash
# Verify the usage of rewardDestinationAccount in SetPayee.tsx
rg 'rewardDestinationAccount' packages/extension-polkagate/src/fullscreen/stake/solo/partials/SetPayee.tsx

Length of output: 402

packages/extension-polkagate/src/fullscreen/stake/solo/StakedSolo.tsx (3)

6-7: LGTM: Improved import organization

The reorganization of import statements, particularly moving type imports to the top, enhances code readability and follows best practices. This change may also positively impact build times through better tree-shaking.


35-35: LGTM: Improved function signature formatting

The addition of a space after the function name in the signature enhances code readability and aligns with common coding style conventions.


Line range hint 1-180: Consider further improvements and clarify PR scope

While the changes in this file are minimal and improve code quality, there might be opportunities for further enhancements:

  1. Consider adding prop types validation using PropTypes or TypeScript interfaces for better type safety.
  2. The component seems to have many responsibilities. Consider breaking it down into smaller, more focused components for better maintainability.
  3. Some of the useCallback dependencies might be incomplete. Review and ensure all necessary dependencies are included.

Additionally, could you please clarify how this file's changes relate to the PR's main objective of supporting Aleph Zero? The current changes don't seem to directly address this goal.

To verify the PR's scope and its relation to Aleph Zero support, you can run:

packages/extension-polkagate/src/fullscreen/stake/solo/partials/ShowValidator.tsx (1)

121-121: Safe access to nominator count implemented.

The use of optional chaining (?.) to access v.exposure.others.length is a good practice. It prevents potential runtime errors if v.exposure or v.exposure.others is undefined. The fallback to 'N/A' when the length is unavailable is appropriate.

packages/extension-polkagate/src/fullscreen/stake/solo/partials/ValidatorsTableFS.tsx (3)

58-58: Improved null safety with optional chaining

The addition of optional chaining (?.) when accessing v.exposure.others is a good practice. It prevents potential runtime errors if v.exposure is undefined, making the code more robust.


62-62: Consistent use of optional chaining

The optional chaining (?.) is correctly applied to v.exposure?.others?.length. This change is consistent with the previous modification and maintains the improved null safety throughout the function.


58-63: Summary: Improved null safety in validator handling

The changes in this file consistently enhance null safety by introducing optional chaining when accessing v.exposure and its properties. This improvement aligns well with the PR's objective of supporting Aleph Zero and its staking functionality by making the validator handling more robust.

The modifications reduce the risk of runtime errors and improve the overall reliability of the ValidatorsTableFS component. Consider the suggested refactoring to further improve code maintainability.

Overall, these changes contribute positively to the integration of Aleph Zero support in the PolkaGate extension.

packages/extension-polkagate/src/fullscreen/stake/partials/DisplayBalance.tsx (5)

69-69: LGTM: Price retrieval update and minor layout adjustment.

The changes in price retrieval align with the updated import statement, and the minor layout adjustment should improve the UI alignment.

Also applies to: 99-99


Line range hint 103-103: Verify the impact of setting a fixed decimal point.

Setting decimalPoint={3} standardizes the balance display, which can improve UI consistency. However, please ensure this doesn't lead to loss of precision for any supported tokens, especially for Aleph Zero.

Consider reviewing the decimal places used by Aleph Zero and other supported tokens to confirm this change is appropriate for all cases.


119-119: LGTM: Improved grid layout balance.

The adjustment of the grid item width from 38% to 36% results in a more balanced layout distribution (28% + 36% + 36% = 100%). This change should improve the overall visual consistency of the component.


Line range hint 1-190: Summary: Changes align with Aleph Zero integration, minor verifications needed.

The modifications in this file, including the update to token price retrieval and UI adjustments, align well with the PR objective of supporting Aleph Zero. The changes appear to make the component more flexible for different token types. However, it's important to verify that these changes don't negatively impact existing functionality, especially regarding price retrieval and balance display precision.

Please ensure that all suggested verifications are performed, particularly regarding the useTokenPrice usage across the codebase and the appropriateness of the 3 decimal point standardization for all supported tokens, including Aleph Zero.


16-16: Verify the impact of switching to useTokenPrice.

The change from useNativeTokenPrice to useTokenPrice suggests a more generic approach to fetching token prices. This aligns with the PR objective of supporting Aleph Zero, but we should ensure this change doesn't negatively impact existing functionality.

Run the following script to check for any remaining usage of useNativeTokenPrice:

✅ Verification successful

Re-attempting to verify the usage of useNativeTokenPrice.

The previous rg command failed due to an unrecognized file type. I'll run a corrected command to search for any remaining usages.


Re-attempting to verify the usage of useNativeTokenPrice.

The previous rg command failed due to an unrecognized file type. I'll adjust the command to target .ts and .tsx files using glob patterns.


Verified: No remaining usages of useNativeTokenPrice found.

The switch to useTokenPrice has been successfully implemented without leaving any residual references to useNativeTokenPrice.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining usage of useNativeTokenPrice

# Test: Search for useNativeTokenPrice usage
rg --type typescript --type tsx 'useNativeTokenPrice'

Length of output: 88


Script:

#!/bin/bash
# Description: Check for any remaining usage of useNativeTokenPrice

# Test: Search for useNativeTokenPrice usage
rg --type ts --type tsx 'useNativeTokenPrice'

Length of output: 73


Script:

#!/bin/bash
# Description: Check for any remaining usage of useNativeTokenPrice

# Test: Search for useNativeTokenPrice usage in .ts and .tsx files
rg 'useNativeTokenPrice' --glob '*.ts' --glob '*.tsx'

Length of output: 53

packages/extension-polkagate/src/popup/send/index.tsx (4)

185-185: LGTM: Simplified setAddress prop passing

Directly passing setRecipientAddress as the setAddress prop is a good simplification. This change is consistent with the new type of recipientAddress and improves code clarity.


Line range hint 1-228: Overall assessment: Improvements in type handling and code simplification

The changes in this file consistently improve type handling and simplify the code, particularly around the recipientAddress state and its usage. These modifications align well with the PR objectives of supporting Aleph Zero and its staking functionality.

Key improvements:

  1. Simplified type for recipientAddress
  2. Improved prop passing in AccountInputWithIdentity
  3. Simplified setAddress prop passing

These changes enhance code clarity and maintainability without introducing apparent issues. Good job on the refactoring!


181-182: LGTM: Improved prop passing in AccountInputWithIdentity

The changes in passing props to AccountInputWithIdentity are good improvements:

  1. Passing recipientAddress directly as address prop is consistent with the earlier type change.
  2. Removing the any type casting for the chain prop potentially improves type safety.

To ensure type safety for the chain prop:

  1. Verify that the chain object matches the expected type in the AccountInputWithIdentity component.
  2. If a specific type is expected, consider adding an explicit type annotation.

Run the following script to check the chain prop type in the AccountInputWithIdentity component:

#!/bin/bash
# Check the chain prop type in AccountInputWithIdentity
ast-grep --lang typescript --pattern 'interface $props {
  $$$
  chain: $_
  $$$
}' packages/extension-polkagate/src/components/AccountInputWithIdentity.tsx

43-43: LGTM: Simplified type for recipientAddress

The change from AccountId | string | null | undefined to string | null | undefined simplifies the type handling for recipientAddress. This aligns with the goal of streamlining the code.

To ensure this change doesn't introduce any issues, please verify that:

  1. All usages of recipientAddress in this file are compatible with the new type.
  2. Any external components or functions expecting the old type are updated accordingly.

Run the following script to check for any remaining AccountId references related to recipientAddress:

✅ Verification successful

Verification Successful: No AccountId References Found for recipientAddress

The recipientAddress type change to string | null | undefined has been successfully verified. No remaining AccountId references related to recipientAddress were found in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for AccountId references related to recipientAddress
rg --type typescript "AccountId.*recipientAddress" packages/extension-polkagate/src/

Length of output: 158

packages/extension-polkagate/src/util/constants.tsx (3)

39-39: LGTM: Addition of Aleph Zero to chains with black logos

The addition of 'AlephZero' to the CHAINS_WITH_BLACK_LOGO array is consistent with the PR objective of adding support for Aleph Zero. This change ensures that the UI will display the correct logo style for the Aleph Zero chain.


84-85: LGTM: Inclusion of Aleph Zero testnet in TEST_NETS array

The addition of ALEPH_ZERO_TESTNET_GENESIS_HASH to the TEST_NETS array is appropriate and consistent with the PR objective. This change ensures that the Aleph Zero testnet is properly recognized and treated as a test network within the application.


39-39: Summary: Successful integration of Aleph Zero support

The changes made to this file effectively integrate support for the Aleph Zero network:

  1. Added 'AlephZero' to CHAINS_WITH_BLACK_LOGO for correct logo display.
  2. Introduced genesis hashes for both Aleph Zero mainnet and testnet.
  3. Included Aleph Zero testnet in the TEST_NETS array.
  4. Updated STAKING_CHAINS to include Aleph Zero networks, enabling staking functionality.

These modifications are consistent, focused, and well-integrated with the existing code structure. They successfully implement the PR objective of adding support for Aleph Zero and its staking functionality.

Also applies to: 54-55, 84-85, 114-116

packages/extension-polkagate/src/hooks/useApiWithChain2.ts (3)

10-10: Import statement for 'useUserAddedEndpoint' is correct

The import of useUserAddedEndpoint from '../fullscreen/addNewChain/utils' is appropriate and ensures that user-added endpoints can be accessed within this hook.


19-19: Assignment of 'userAddedEndpoint' is appropriate

Retrieving userAddedEndpoint using the genesisHash ensures that any user-defined endpoints associated with the specific chain are correctly obtained.


35-35: Dependencies array in 'useMemo' is updated correctly

Including userAddedEndpoint in the dependencies array ensures that maybeEndpoint recalculates whenever userAddedEndpoint changes, maintaining the consistency of the hook's output.

packages/extension-polkagate/src/hooks/usePeopleChain.ts (5)

9-9: Importing useUserAddedEndpoint is correctly implemented

The import statement appropriately includes useUserAddedEndpoint, which is utilized later to handle user-added endpoints.


35-35: Correct return of POLKADOT_PEOPLE_GENESIS_HASH

The function getPeopleChainGenesisHash correctly returns POLKADOT_PEOPLE_GENESIS_HASH when the chain name starts with 'Polkadot'.


47-48: Using useUserAddedEndpoint with genesisHash

The hook useUserAddedEndpoint is correctly invoked with genesisHash to fetch user-added endpoints relevant to the specified chain.


54-54: Retrieving identityChain using useMetadata

The identityChain is obtained using useMetadata, utilizing peopleChainGenesisHash || genesisHash to ensure it falls back appropriately when peopleChainGenesisHash is undefined.


57-57: Sanitizing the identity chain name

Sanitizing identityChain?.name with sanitizeChainName ensures consistency in chain name formatting.

packages/extension-polkagate/src/partials/FullScreenChainSwitch.tsx (2)

Line range hint 27-29: Check for empty options before filtering

In the computation of selectableNetworks, if options is empty or undefined, it may lead to errors. Although it's unlikely, adding a safety check can prevent potential runtime errors.

Consider verifying that options is always defined and populated. If there's a possibility it could be empty, add a default value or handle the empty state appropriately.


Line range hint 27-32: Ensure options context has consistent data

The options obtained from GenesisHashOptionsContext are used directly. Ensure that the context provider always supplies the expected data structure to avoid potential runtime errors.

Consider adding validation or default values to handle cases where options might be undefined or malformed.

packages/extension-polkagate/src/popup/staking/solo/stake/partials/SetPayeeController.tsx (6)

6-6: Import statement is correct.

The required types Payee, SoloSettings, and StakingConsts are correctly imported from the relative path.


27-27: Component definition is appropriately structured.

The SetPayeeController function component is defined with properly destructured props, enhancing readability and maintainability.


32-34: State initialization with appropriate types.

The useState hooks for controllerId, rewardDestinationValue, and rewardDestinationAccount are initialized with suitable types and default values. The inclusion of string | undefined | null accurately represents all possible states of these variables.


71-71: Explicitly returning undefined enhances clarity.

By explicitly returning undefined in the makePayee function when no conditions are met, the code clearly communicates the intended outcome, improving readability.


128-132: Conditional handling of setAddress prop is appropriate.

The AccountInputWithIdentity component correctly handles the setAddress prop based on the isControllerDeprecated flag. This ensures that when the controller account is deprecated, the input is disabled, preventing unintended user interactions.


163-164: Reward destination account input is properly configured.

The AccountInputWithIdentity component for the reward destination account is set up correctly, allowing users to specify an alternative account when the reward destination is set to 'Others'.

replacements/interfaces.js (3)

13-13: Assigning genesisHash using supportedGenesis mapping is appropriate

The assignment correctly updates n.genesisHash to use the new supportedGenesis object, ensuring the genesis hashes include the newly supported networks.


41-51: Expansion of supportedGenesis with additional networks

The supportedGenesis object is appropriately extended with new network entries, enhancing the support for networks like aleph and others.


139-149: Verify the prefix value for the aleph network

The prefix for the aleph network is currently set to 42, which is generally used for testnets or generic accounts. It's important to ensure that this value accurately reflects Aleph Zero's official SS58 prefix.

Run the following script to confirm the correct SS58 prefix for Aleph Zero:

Comment on lines +15 to 17
// @ts-ignore
return settings.payee?.Account ? String(settings.payee.Account) : undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Investigate and resolve the underlying type issue.

The @ts-ignore comment is used to suppress TypeScript errors for the fallback case. While this allows the code to compile, it can hide potential type-related issues and reduce type safety.

Consider investigating the underlying type issue and resolving it properly. Here are some suggestions:

  1. Check the type definition of SoloSettings and ensure it accurately represents all possible shapes of the settings object.
  2. Use type narrowing or type guards to handle different shapes of the settings object without suppressing TypeScript errors.
  3. If the type of settings.payee is indeed unpredictable, consider using a more specific type like unknown and then narrowing it down with type checks.

Example of a possible type-safe implementation:

export default function getPayee(settings: SoloSettings): string | undefined {
  if (settings.payee === 'Stash') {
    return settings.stashId?.toString();
  }

  if (settings.payee === 'Controller') {
    return settings.controllerId?.toString();
  }

  if (typeof settings.payee === 'object' && settings.payee !== null && 'Account' in settings.payee) {
    return settings.payee.Account?.toString();
  }

  return undefined;
}

This approach eliminates the need for @ts-ignore while maintaining type safety.

Comment on lines +799 to +801
controllerId?: string | undefined,
payee: Payee,
stashId?: AccountId | string | undefined,
stashId?: string | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Issues Found with Remaining AccountId Usage in controllerId and stashId.

The verification script identified the following instances where AccountId is still used with controllerId or stashId:

  • packages/extension-polkagate/src/hooks/useStashId.ts:
    const [stashId, setStashId] = useState<AccountId | string>();
  • packages/extension-polkagate/src/hooks/useValidators.ts:
    stashId: v as unknown as AccountId,
  • packages/extension-polkagate/src/hooks/useIsExposed.ts:
    const checkIsExposed = useCallback(async (stashId: AccountId | string): Promise<undefined> => {
  • packages/extension-polkagate/src/popup/staking/partial/SelectValidators.tsx:
    stashId: AccountId | string;
  • packages/extension-polkagate/src/fullscreen/stake/solo/partials/SelectValidatorsFs.tsx:
    stashId: AccountId | string | undefined;
  • packages/extension-polkagate/src/fullscreen/stake/pool/commonTasks/manageValidators/InputPage.tsx:
    stashId={pool?.accounts?.stashId as unknown as AccountId}

Please address these instances by updating the types to string | undefined to ensure consistency across the codebase. Additionally, review any related logic that may depend on the AccountId type to accommodate the type changes.

🔗 Analysis chain

Approve changes to SoloSettings interface with a note on potential impacts.

The simplification of controllerId and stashId types from AccountId | string | undefined to string | undefined is a good change that aligns with the PR objective of supporting Aleph Zero. This change makes the interface more flexible and easier to work with across different blockchain implementations.

However, please ensure the following:

  1. Verify that all code interacting with SoloSettings.controllerId and SoloSettings.stashId has been updated to handle the string type instead of AccountId.
  2. Check if there are any type-specific operations or validations that were previously relying on the AccountId type, and update them accordingly.
  3. Consider adding a comment explaining the rationale behind this change, especially if it's part of a larger refactoring effort to standardize account ID representations.

To ensure that these changes don't introduce any regressions, please run the following verification script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining usage of AccountId type with controllerId or stashId

# Search for potential issues in TypeScript files
echo "Searching for potential issues in TypeScript files:"
rg --type typescript 'controllerId.*AccountId|stashId.*AccountId' -g '!**/types.ts'

# Search for potential issues in JavaScript files
echo "Searching for potential issues in JavaScript files:"
rg --type javascript 'controllerId.*AccountId|stashId.*AccountId'

echo "If no results are shown above, it means no potential issues were found."

Length of output: 602


Script:

#!/bin/bash
# Description: Corrected script to check for any remaining usage of AccountId type with controllerId or stashId

# Check available file types
echo "Available file types in rg:"
rg --type-list

# Search for potential issues in TypeScript files
echo "Searching for potential issues in TypeScript files:"
rg --type=typescript 'controllerId.*AccountId|stashId.*AccountId' -g '!**/types.ts'

# Search for potential issues in JavaScript files
echo "Searching for potential issues in JavaScript files:"
rg --type=javascript 'controllerId.*AccountId|stashId.*AccountId'

echo "If no results are shown above, it means no potential issues were found."

Length of output: 5489


Script:

#!/bin/bash
# Description: Corrected script to check for any remaining usage of AccountId type with controllerId or stashId

# Check available file types
echo "Available file types in rg:"
rg --type-list

# Search for potential issues in TypeScript files using correct type identifier
echo "Searching for potential issues in TypeScript files using type identifier 'ts':"
rg --type=ts 'controllerId.*AccountId|stashId.*AccountId' -g '!**/types.ts'

# If the above search fails, search using file extensions as a fallback
echo "Searching for potential issues in TypeScript files using file extensions (*.ts, *.tsx):"
rg 'controllerId.*AccountId|stashId.*AccountId' -g '*.ts' -g '*.tsx' -g '!**/types.ts'

# Search for potential issues in JavaScript files using type identifier 'js'
echo "Searching for potential issues in JavaScript files using type identifier 'js':"
rg --type=js 'controllerId.*AccountId|stashId.*AccountId'

# If the above search fails, search using file extensions as a fallback
echo "Searching for potential issues in JavaScript files using file extensions (*.js, *.jsx, *.vue):"
rg 'controllerId.*AccountId|stashId.*AccountId' -g '*.js' -g '*.jsx' -g '*.vue'

echo "If no results are shown above, it means no potential issues were found."

Length of output: 7848

Comment on lines +24 to +28
const endpoints = allEndpoints?.filter((e) =>
String(e.text)?.toLowerCase() === chainName?.toLowerCase() ||
String(e.info)?.toLowerCase() === chainName?.toLowerCase() ||
String(e.text)?.replace(/\s/g, '')?.toLowerCase() === chainName?.toLowerCase()
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential issue with converting undefined to string

Using String(e.text) and String(e.info) may result in the string "undefined" when e.text or e.info are undefined, leading to incorrect comparisons. This could cause valid endpoints to be missed in the filter.

Consider updating the filter logic to handle undefined values safely:

const endpoints = allEndpoints?.filter((e) =>
  e.text?.toLowerCase() === chainName?.toLowerCase() ||
  e.info?.toLowerCase() === chainName?.toLowerCase() ||
  e.text?.replace(/\s/g, '').toLowerCase() === chainName?.toLowerCase()
);

Comment on lines +30 to +32
if (!endpoints?.length && userAddedEndpoint) {
return userAddedEndpoint[0].value as string;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure 'userAddedEndpoint' is not empty before accessing

When userAddedEndpoint is defined but empty, accessing userAddedEndpoint[0] may lead to an undefined value or runtime error. To prevent this, check that the array has at least one element:

if (!endpoints?.length && userAddedEndpoint?.length > 0) {
  return userAddedEndpoint[0].value as string;
}

Comment on lines +65 to +70
if (!endpoints?.length && userAddedEndpoint) {
return userAddedEndpoint[0].value as string;
}

return endpoints?.length ? endpoints[0].value : undefined;
}, [peopleChain]);
}, [identityChain?.name, userAddedEndpoint]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance clarity in endpoint selection logic

The current logic in selecting the endpoint could be made clearer. Specifically, the return conditions might benefit from explicit checks.

Consider refactoring the maybeEndpoint logic for better readability and maintenance:

-  if (!endpoints?.length && userAddedEndpoint) {
-    return userAddedEndpoint[0].value as string;
-  }
-
-  return endpoints?.length ? endpoints[0].value : undefined;
+  if (endpoints?.length > 0) {
+    return endpoints[0].value;
+  } else if (userAddedEndpoint && userAddedEndpoint.length > 0) {
+    return userAddedEndpoint[0].value as string;
+  } else {
+    return undefined;
+  }

This refactoring explicitly handles each condition, making the code easier to understand.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!endpoints?.length && userAddedEndpoint) {
return userAddedEndpoint[0].value as string;
}
return endpoints?.length ? endpoints[0].value : undefined;
}, [peopleChain]);
}, [identityChain?.name, userAddedEndpoint]);
if (endpoints?.length > 0) {
return endpoints[0].value;
} else if (userAddedEndpoint && userAddedEndpoint.length > 0) {
return userAddedEndpoint[0].value as string;
} else {
return undefined;
}
}, [identityChain?.name, userAddedEndpoint]);


return {
endpoint: maybeEndpoint,
peopleChain
peopleChain: identityChain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Inconsistent naming between peopleChain and identityChain

Renaming identityChain to peopleChain will impact multiple parts of the codebase. The following files reference peopleChain and will require updates to ensure consistency:

  • packages/extension-polkagate/src/hooks/useIdentity.ts
  • packages/extension-polkagate/src/hooks/usePeopleChain.ts
  • packages/extension-polkagate/src/hooks/useAccountsInfo.ts
  • packages/extension-polkagate/src/fullscreen/manageIdentity/index.tsx

Ensure all these references are updated accordingly to prevent potential runtime errors.

🔗 Analysis chain

Inconsistent naming between peopleChain and identityChain

The returned object uses peopleChain: identityChain, which may cause confusion due to the mismatch in naming conventions. The interface PeopleChainInfo defines peopleChain, but the variable used is identityChain.

To maintain consistency, consider renaming either the property in the returned object or the variable itself. For example, you could rename identityChain back to peopleChain, or update the interface and all associated references to use identityChain.

Suggested Change:

// Update the interface to match the new naming
-interface PeopleChainInfo {
-  peopleChain: Chain | null | undefined;
+interface PeopleChainInfo {
+  identityChain: Chain | null | undefined;
  endpoint: string | undefined;
}

// Update the return statement
return {
  endpoint: maybeEndpoint,
- peopleChain: identityChain
+ identityChain: identityChain
};

Verification Script:

Run the following script to identify all usages of peopleChain in the codebase to assess the impact of renaming:

This will help ensure that all references are updated accordingly, preventing potential runtime errors.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all occurrences of 'peopleChain' in TypeScript files.

# Expected: List of files and lines where 'peopleChain' is used.
rg --type ts 'peopleChain' -n

Length of output: 1743

@@ -28,7 +28,7 @@ export default function useIdentity (genesisHash: string | undefined, formatted:
}

const i = await api.query['identity']['identityOf'](accountId) as any;
const id = i.isSome ? i.unwrap()[0] as PalletIdentityRegistration : null;
const id = i.isSome ? i.unwrap()[0] || i.unwrap() as PalletIdentityRegistration : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix potential type casting issue by adding parentheses

The type assertion may not apply as intended due to operator precedence. To ensure that the type casting applies to the entire expression (i.unwrap()[0] || i.unwrap()), please add parentheses around it.

Apply this diff to fix the issue:

-    const id = i.isSome ? i.unwrap()[0] || i.unwrap() as PalletIdentityRegistration : null;
+    const id = i.isSome ? (i.unwrap()[0] || i.unwrap()) as PalletIdentityRegistration : null;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const id = i.isSome ? i.unwrap()[0] || i.unwrap() as PalletIdentityRegistration : null;
const id = i.isSome ? (i.unwrap()[0] || i.unwrap()) as PalletIdentityRegistration : null;

@@ -110,7 +109,7 @@ export default function SetPayeeController({ address, buttonLabel, newSettings,
!setShowReview && setShow(false); // can be left open when settings accessed from home
}, [controllerId, isSettingAtBonding, makePayee, rewardDestinationAccount, rewardDestinationValue, set, setShow, setShowReview, settings]);

const Warn = ({ text, style = {} }: { text: string, style?: SxProps }) => (
const Warn = ({ style = {}, text }: { text: string, style?: SxProps }) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Inconsistent parameter order in Warn component definition.

The order of parameters in the destructuring assignment does not match the order in the type annotation. This inconsistency could lead to confusion or errors if the parameter order is significant.

Consider reordering the parameters to align the destructuring with the type annotation:

-const Warn = ({ style = {}, text }: { text: string, style?: SxProps }) => (
+const Warn = ({ text, style = {} }: { text: string, style?: SxProps }) => (

This adjustment ensures consistency between the parameter destructuring and the type definition.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const Warn = ({ style = {}, text }: { text: string, style?: SxProps }) => (
const Warn = ({ text, style = {} }: { text: string, style?: SxProps }) => (

@@ -128,7 +128,7 @@ export default function ConfigurePayee ({ address, setRefresh, setShow, show }:

setRewardDestinationValue(payee === 'Staked' ? 'Staked' : 'Others');

return ({ payee, stashId: parsedStakingAccount.stashId });
return ({ payee, stashId: parsedStakingAccount.stashId as unknown as string });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid casting 'stashId' using 'as unknown as string'; use '.toString()' method instead

Casting parsedStakingAccount.stashId with as unknown as string may conceal underlying type issues and reduce type safety. It's clearer and more type-safe to convert stashId to a string using the .toString() method.

Apply this diff to fix the issue:

-        return ({ payee, stashId: parsedStakingAccount.stashId as unknown as string });
+        return ({ payee, stashId: parsedStakingAccount.stashId.toString() });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return ({ payee, stashId: parsedStakingAccount.stashId as unknown as string });
return ({ payee, stashId: parsedStakingAccount.stashId.toString() });


knownSubstrate.push(...assetHubs, ...peopleChains, ...testnets, ...liveChains);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Avoid mutating the imported knownSubstrate array to prevent side effects

Modifying the imported knownSubstrate array directly can lead to unintended side effects elsewhere in the codebase where knownSubstrate is used. It's better to create a new array that combines the original data without altering the imported module.

Apply this diff to create a new array without mutating knownSubstrate:

- knownSubstrate.push(...assetHubs, ...peopleChains, ...testnets, ...liveChains);

- export const allNetworks = knownSubstrate.map(toExpanded);
+ const extendedKnownSubstrate = [
+   ...knownSubstrate,
+   ...assetHubs,
+   ...peopleChains,
+   ...testnets,
+   ...liveChains
+ ];

+ export const allNetworks = extendedKnownSubstrate.map(toExpanded);

This change ensures that the original knownSubstrate array remains unmodified, preventing potential side effects in other parts of the application.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
knownSubstrate.push(...assetHubs, ...peopleChains, ...testnets, ...liveChains);
const extendedKnownSubstrate = [
...knownSubstrate,
...assetHubs,
...peopleChains,
...testnets,
...liveChains
];
export const allNetworks = extendedKnownSubstrate.map(toExpanded);

@Nick-1979 Nick-1979 added this to the Milestone 4 milestone Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant