fix(suite): fixed staking in sidebar #17525
Conversation
WalkthroughThe changes update the logic used to determine if staking details should be displayed in the ✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
const hasEthStaked = useSelector(state => selectEthAccountHasStaked(state, account.key));
const hasSolStaked = useSelector(state => selectSolAccountHasStaked(state, account.key));
Previously it was decided based on this logic which is used also on other places, it would be great to have it unified all over the places so that it does not happen that there is staking in sidebar but you see "staking welcome screen" or vice versa
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
suite-common/wallet-core/src/stake/stakeUtils.ts (2)
17-23: Consider adding JSDoc comments for better documentation.While the code is clear, adding JSDoc comments would improve maintainability by documenting the purpose of the function, its parameters, and return value.
+/** + * Determines if an account is actively participating in staking. + * An account is considered active if: + * - It has a deposited balance, or + * - It has pending stake/unstake/claim operations, or + * - It has funds ready to claim + * + * @param account - The account to check for staking activity + * @param claimTransactions - List of claim transactions to check for pending status + * @returns True if the account is actively participating in staking, false otherwise + */ export const isAccountStakingActive = ( account: Account | null, claimTransactions: WalletAccountTransaction[], ) => {
25-25: Consider adding a comment explaining the purpose of pendingClaimTxs.Adding a brief comment would clarify why we're filtering for pending claim transactions and how they're used in determining if a claim is pending.
- const pendingClaimTxs = claimTransactions.filter(tx => isPending(tx)); + // Filter for pending claim transactions to determine if a claim operation is in progress + const pendingClaimTxs = claimTransactions.filter(tx => isPending(tx));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/suite/src/components/wallet/WalletLayout/AccountsMenu/AccountSection.tsx(2 hunks)packages/suite/src/views/dashboard/AssetsView/AssetCard/AssetCard.tsx(3 hunks)packages/suite/src/views/dashboard/AssetsView/AssetTable/AssetRow.tsx(4 hunks)packages/suite/src/views/dashboard/AssetsView/assetsViewUtils.ts(1 hunks)packages/suite/src/views/wallet/staking/components/EthStakingDashboard/components/EthStakingDashboard.tsx(3 hunks)packages/suite/src/views/wallet/staking/components/SolStakingDashboard/SolStakingDashboard.tsx(2 hunks)packages/suite/src/views/wallet/staking/components/SolStakingDashboard/components/Rewards/RewardsList.tsx(3 hunks)packages/suite/src/views/wallet/staking/components/StakingDashboard/StakingDashboard.tsx(1 hunks)suite-common/wallet-core/src/stake/stakeUtils.ts(1 hunks)suite-common/wallet-core/src/transactions/transactionsReducer.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/suite/src/components/wallet/WalletLayout/AccountsMenu/AccountSection.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (30)
packages/suite/src/views/dashboard/AssetsView/assetsViewUtils.ts (2)
43-45: Improved variable naming for better semantics.The renamed variable
hasAccountsWithStakingBalancebetter reflects its purpose - it indicates the presence of accounts with staking balance rather than making a rendering decision. This enhances code readability and aligns with the PR's goal of clearly identifying when staking should be displayed.
51-51: LGTM - consistent variable name update.The return object properly uses the renamed variable, maintaining consistency throughout the function.
packages/suite/src/views/wallet/staking/components/SolStakingDashboard/components/Rewards/RewardsList.tsx (3)
4-4: Appropriate new selector import.The import of
selectAccountIsStakingActivealigns with the PR's goal of refining staking visibility logic.
53-53: Correctly implementing the staking active check.Using the selector with
useSelectorhook is the right approach to determine if staking is active for the specific account.
55-61: Improved conditional rendering logic.This change properly implements the PR requirement by:
- Only showing the
RewardsEmptycomponent when staking is active but there are no rewards- Returning null when staking is not active
This ensures that staking UI elements are only displayed when relevant to the user.
packages/suite/src/views/wallet/staking/components/StakingDashboard/StakingDashboard.tsx (1)
22-22: Simplified component rendering.Removing conditional logic from this component is appropriate since the responsibility for determining whether to show staking content has been moved to the parent components. This maintains the single responsibility principle and makes the component more focused.
packages/suite/src/views/wallet/staking/components/SolStakingDashboard/SolStakingDashboard.tsx (3)
3-7: Well-organized imports.The imports have been properly reorganized to include the new
selectAccountIsStakingActiveselector.
40-40: Correctly implementing the staking active check.Using the new selector to determine if staking is active for the current account aligns perfectly with the PR objective.
47-81: Well-implemented conditional rendering.This change perfectly implements the PR's goal by:
- Only showing the staking dashboard when staking is active
- Showing an
EmptyStakingCardwhen staking is not activeThe conditional structure is clear and easy to understand, making the code maintainable.
packages/suite/src/views/wallet/staking/components/EthStakingDashboard/components/EthStakingDashboard.tsx (3)
6-6: Good addition of new importsThe imports for
selectAccountIsStakingActiveandEmptyStakingCardare correctly added to support the conditional rendering of staking UI elements.Also applies to: 27-27
72-72: Well-implemented staking status checkGood use of the selector to determine if staking is active for the account, which will help control when staking information is displayed.
79-117: Excellent conditional rendering implementationThe conditional rendering logic improves the user experience by showing either the full staking dashboard when staking is active or an empty state when it's not. This aligns perfectly with the PR objective to only show staking information under specific conditions.
packages/suite/src/views/dashboard/AssetsView/AssetCard/AssetCard.tsx (5)
10-10: Good addition of the selector importThe import of
selectAnyAccountIsStakingActiveis correctly added to determine if any accounts associated with the asset are actively staking.
130-132: Well-implemented staking status checkGood use of the selector to determine if any of the staking accounts for this asset are active, which will help control when staking information is displayed in the asset card.
134-147: Clear renaming of variablesThe renaming from
shouldRenderStakingRowtohasAccountsWithStakingBalancemakes the code more readable and better reflects what the variable represents - a check for accounts with balances rather than a UI rendering decision.
148-148: Good implementation of the rendering conditionThe new
shouldRenderStakingvariable combines both balance check and active staking status, which correctly implements the PR requirement to only show staking information under specific conditions.
192-201: Updated rendering logic improves UXThe conditional rendering of
AssetCardTokensAndStakingInfousing both staking status and presence of tokens enhances the user experience by only showing relevant information, which aligns with the PR objectives.suite-common/wallet-core/src/transactions/transactionsReducer.ts (3)
40-40: Good import of the utility functionThe import of
isAccountStakingActiveutility function is appropriately added to support the new selectors.
386-389: Well-implemented account staking selectorThe new
selectAccountIsStakingActiveselector correctly uses the utility function to determine if a specific account is actively staking, which is essential for the conditional UI rendering in other components.
391-405: Excellent implementation of the aggregated staking selectorThe
selectAnyAccountIsStakingActiveselector efficiently processes multiple accounts to determine if any are actively staking. The implementation follows good practices:
- Correctly maps accounts to their transactions
- Filters for claim transactions
- Uses the utility function to check each account's staking status
- Returns a boolean indicating if any account is active
This implementation enables proper conditional rendering of staking UI elements across multiple components.
packages/suite/src/views/dashboard/AssetsView/AssetTable/AssetRow.tsx (6)
9-9: Good addition of the selector importThe import of
selectAnyAccountIsStakingActiveis correctly added to determine if any accounts associated with the asset are actively staking.
87-89: Well-implemented staking status checkGood use of the selector to determine if any of the staking accounts for this asset are active, which helps control when staking information is displayed in the asset row.
91-104: Clear renaming of variablesThe renaming from
shouldRenderStakingRowtohasAccountsWithStakingBalancemakes the code more readable and better reflects what the variable represents - a check for accounts with balances rather than a UI rendering decision.
105-105: Good implementation of the rendering conditionThe new
shouldRenderStakingvariable combines both balance check and active staking status, which correctly implements the PR requirement to only show staking information under specific conditions.
113-114: Consistent update to dashed line position logicGood update to the
$dashedLinePositionproperty to use the newshouldRenderStakingvariable, maintaining visual consistency with the new rendering logic.
193-199: Updated rendering logic improves UXThe conditional rendering of
AssetStakingRowusing the newshouldRenderStakingcondition enhances the user experience by only showing staking information when it's relevant, which aligns with the PR objectives.suite-common/wallet-core/src/stake/stakeUtils.ts (4)
9-33: Well-implemented staking activity check function.This new utility function
isAccountStakingActiveprovides a comprehensive check for whether an account is active in staking, aligning with the PR objective to display staking in the sidebar only under specific conditions. The function correctly handles various scenarios:
- Properly validates account existence and staking support
- Checks for deposited balance
- Detects pending stake/unstake/claim operations
- Identifies if claiming is available
The use of BigNumber for comparisons is appropriate for financial calculations to avoid floating-point precision issues.
13-15: Good defensive programming with early returns.These early return conditions appropriately handle edge cases (null account or unsupported network) before proceeding with the main logic, improving both code readability and performance.
29-29: Verify logic for determining pending claims.The condition for
isClaimPendingchecks bothclaimableAmount > 0ANDpendingClaimTxs.length > 0. Please confirm this is intentional - a claim is only considered pending when there's a claimable amount AND there are pending claim transactions.
32-32: Return statement aligns well with PR objectives.The return statement effectively implements the requirements specified in the PR objectives, showing staking information only when there's a non-zero staked balance, pending operations, or claim availability.
|
🚀 Expo preview is ready!
|
9bfc7ee to
4456fb2
Compare
Description
Staking in sidebar is shown only if non-zero balance is staked or there is stake/unstake/claim pending or the claim is ready.
Related Issue
Resolve #16864
Screenshots:
Ethereum #1staking is shown because the claim is ready:Ethereum #2staking is hidden because there is no staked balance and no other condition is met: