refactor(web): min-stake-check #1806
Conversation
WalkthroughThe pull request enhances the staking functionality within the Court Details section of the web application. Key modifications are made to the Changes
Sequence DiagramsequenceDiagram
participant User
participant InputDisplay
participant StakeWithdrawButton
participant CourtDetails
User->>InputDisplay: Enter stake amount
InputDisplay->>CourtDetails: Fetch court details
CourtDetails-->>InputDisplay: Return minimum stake
InputDisplay->>InputDisplay: Validate stake amount
alt Stake amount < Minimum stake
InputDisplay->>User: Display minimum stake error
else Stake amount >= Minimum stake
InputDisplay->>StakeWithdrawButton: Enable stake button
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx (1)
266-273: Consider adding user feedback for minimum stake requirement.While the minimum stake validation is correctly implemented, users might not understand why the stake button is disabled when their stake amount is below the minimum requirement.
Consider setting an error message in the
useEffecthook when the stake amount is below the minimum:useEffect(() => { if (isPopupOpen) return; if (setStakeError || allowanceError) { setErrorMsg(parseWagmiError(setStakeError || allowanceError)); + } else if ( + action === ActionType.stake && + targetStake !== 0n && + courtDetails?.court?.minStake && + targetStake < BigInt(courtDetails.court.minStake) + ) { + setErrorMsg(`Minimum stake required: ${courtDetails.court.minStake} PNK`); } - }, [setStakeError, setErrorMsg, targetStake, allowanceError, isPopupOpen]); + }, [setStakeError, setErrorMsg, targetStake, allowanceError, isPopupOpen, action, courtDetails]);web/src/pages/Courts/CourtDetails/StakePanel/InputDisplay.tsx (2)
10-10: Consider organizing imports by typeThe imports could be better organized by grouping them into:
- External dependencies (React, styled-components)
- Internal utilities
- Components
- Hooks
- Styles
import React, { useState, useMemo, useEffect } from "react"; import styled from "styled-components"; import { useParams } from "react-router-dom"; import { useDebounce } from "react-use"; + // Internal hooks import { useCourtDetails } from "queries/useCourtDetails"; import { useParsedAmount } from "hooks/useParsedAmount"; import { usePnkData } from "hooks/usePNKData"; + // Utilities import { commify, uncommify } from "utils/commify"; import { formatETH, formatPNK, roundNumberDown } from "utils/format"; import { isUndefined } from "utils/index"; + // Styles import { hoverShortTransitionTiming } from "styles/commonStyles";Also applies to: 13-16
94-101: Consider refactoring complex validation logicThe validation logic could be simplified and made more maintainable by:
- Extracting validation logic into separate functions
- Adding null checks for courtDetails.court
- Using early returns instead of nested conditions
+ const isMinStakeValid = ( + action: ActionType, + courtDetails?: { court?: { minStake: string } }, + jurorBalance?: bigint[], + parsedAmount: bigint + ) => { + if ( + action !== ActionType.stake || + !courtDetails?.court?.minStake || + !jurorBalance || + parsedAmount === 0n + ) return true; + + return jurorBalance[2] + parsedAmount >= BigInt(courtDetails.court.minStake); + }; useEffect(() => { if (parsedAmount > 0n && balance === 0n && isStaking) { setErrorMsg("You need a non-zero PNK balance to stake"); return; } if (isStaking && balance && parsedAmount > balance) { setErrorMsg("Insufficient balance to stake this amount"); return; } if (!isStaking && jurorBalance && parsedAmount > jurorBalance[2]) { setErrorMsg("Insufficient staked amount to withdraw this amount"); return; } - if ( - action === ActionType.stake && - courtDetails && - jurorBalance && - parsedAmount !== 0n && - jurorBalance[2] + parsedAmount < BigInt(courtDetails?.court?.minStake) - ) { + if (!isMinStakeValid(action, courtDetails, jurorBalance, parsedAmount)) { setErrorMsg(`Min Stake in court is: ${formatETH(courtDetails?.court?.minStake)}`); return; } setErrorMsg(undefined); }, [parsedAmount, isStaking, balance, jurorBalance, action, courtDetails]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/pages/Courts/CourtDetails/StakePanel/InputDisplay.tsx(3 hunks)web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx(5 hunks)
🧰 Additional context used
📓 Learnings (1)
web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1775
File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0
Timestamp: 2024-12-09T12:36:59.441Z
Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.
🔇 Additional comments (5)
web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx (4)
71-71: LGTM: Court details integration for minimum stake validation.
The useCourtDetails hook is properly integrated using the court ID from URL parameters.
183-192: LGTM: Clear error handling for simulation failure.
The error handling for simulation failure is well-implemented with a clear, actionable message and proper abort signal handling.
260-263: LGTM: Streamlined error handling logic.
The error handling has been simplified to focus on stake and allowance errors, with minimum stake validation moved to the disabled state check.
287-289: LGTM: Proper dependency management.
The dependencies array has been correctly updated to include all variables used in the isDisabled calculation.
web/src/pages/Courts/CourtDetails/StakePanel/InputDisplay.tsx (1)
80-80: LGTM: Proper hook usage
The useCourtDetails hook is correctly implemented with the court id from URL parameters, following React's data fetching best practices.
|
Code Climate has analyzed commit efedc56 and detected 3 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/src/pages/Courts/CourtDetails/StakePanel/InputDisplay.tsx (1)
94-105: Consider extracting validation logic for better readabilityThe useEffect hook has grown complex with multiple conditions. Consider extracting the validation logic into separate functions:
+ const isInsufficientMinStake = ( + action: ActionType, + courtDetails?: any, + jurorBalance?: any, + parsedAmount?: bigint + ) => { + return ( + action === ActionType.stake && + courtDetails && + jurorBalance && + parsedAmount !== 0n && + jurorBalance[2] + parsedAmount < BigInt(courtDetails?.court?.minStake) + ); + }; useEffect(() => { if (parsedAmount > 0n && balance === 0n && isStaking) { setErrorMsg("You need a non-zero PNK balance to stake"); } else if (isStaking && balance && parsedAmount > balance) { setErrorMsg("Insufficient balance to stake this amount"); } else if (!isStaking && jurorBalance && parsedAmount > jurorBalance[2]) { setErrorMsg("Insufficient staked amount to withdraw this amount"); - } else if ( - action === ActionType.stake && - courtDetails && - jurorBalance && - parsedAmount !== 0n && - jurorBalance[2] + parsedAmount < BigInt(courtDetails?.court?.minStake) - ) { + } else if (isInsufficientMinStake(action, courtDetails, jurorBalance, parsedAmount)) { setErrorMsg(`Min Stake in court is: ${formatPNK(courtDetails?.court?.minStake)} PNK`); } else { setErrorMsg(undefined); } }, [parsedAmount, isStaking, balance, jurorBalance, action, courtDetails]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/lib/forge-std(1 hunks)web/src/pages/Courts/CourtDetails/StakePanel/InputDisplay.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/lib/forge-std
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1775
File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0
Timestamp: 2024-12-09T12:36:59.441Z
Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.
🔇 Additional comments (3)
web/src/pages/Courts/CourtDetails/StakePanel/InputDisplay.tsx (3)
13-16: LGTM: Import statements are clean and well-organized
The new imports are properly structured and used within the component.
101-101: LGTM: Consistent token formatting
The error message now correctly uses formatPNK for formatting the minimum stake amount, which is consistent with the rest of the component.
80-80: Verify court details fetching behavior
The hook usage looks correct, but let's verify the error handling for cases where court details might be undefined.
✅ Verification successful
Proper error handling is in place for undefined court details
The verification shows that the useCourtDetails hook is properly implemented and its usage in InputDisplay.tsx has appropriate error handling:
- The hook itself is enabled conditionally based on the ID's existence
- Error cases are handled in the component through conditional checks before accessing
courtDetailsproperties - The component uses proper null-safe access with optional chaining (
courtDetails?.court?.minStake) - The
courtDetailsdependency is correctly included in the dependency arrays of hooks and callbacks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's proper error handling for undefined court details
ast-grep --pattern 'useCourtDetails($id)' -A 10
Length of output: 49
Script:
#!/bin/bash
# Let's try a different pattern to find the hook usage and its context
ast-grep --pattern 'const { $_ } = useCourtDetails($_)'
# Also search for the hook definition
ast-grep --pattern 'function useCourtDetails($_) {
$$$
}'
# Search for error handling patterns around court details
rg "courtDetails" -A 5 -B 5
Length of output: 13343
PR-Codex overview
This PR enhances the
InputDisplayandStakeWithdrawButtoncomponents by integratingcourtDetailsfor improved validation and error messaging related to staking and withdrawing actions.Detailed summary
useCourtDetailshook to fetch court details in bothInputDisplayandStakeWithdrawButton.courtDetails.court?.minStake.useEffecthooks to includecourtDetails.courtDetails.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
useCourtDetailsto reflect new structure.