-
Notifications
You must be signed in to change notification settings - Fork 3
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
Revert "fix: rise and fall contracts" #28
Revert "fix: rise and fall contracts" #28
Conversation
Reviewer's Guide by SourceryThis PR reverts the changes introduced in #27. It removes the buy contract functionality and the open contracts modal from the trading panel. The price proposal logic is moved back into the useEffect hook. Sequence diagram for price proposal flow after revertsequenceDiagram
participant Component
participant usePriceProposal
participant DerivAPI
Component->>usePriceProposal: Initialize with parameters
activate usePriceProposal
Note over usePriceProposal: useEffect triggers
usePriceProposal->>DerivAPI: Subscribe to RISE proposal
usePriceProposal->>DerivAPI: Subscribe to FALL proposal
DerivAPI-->>usePriceProposal: Proposal updates
usePriceProposal-->>Component: Return proposal data
deactivate usePriceProposal
Class diagram showing removed functionalityclassDiagram
class TradingPanelStore {
-duration: number
-price: number
-selectedStakeTab: string
-durationError: string
-priceError: string
-is_rise_fall_valid: boolean
+setDuration()
+setPrice()
+setSelectedStakeTab()
}
class usePriceProposal {
+proposal: object
+clearProposal()
+isLoading: object
}
note for TradingPanelStore "Removed:
- riseContractId
- fallContractId
- openContracts
- isOpenContractsModalVisible"
note for usePriceProposal "Moved proposal logic
back into useEffect"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @vinu-deriv - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please provide context in the PR description explaining why this feature is being reverted. Understanding the motivation will help reviewers assess if this is the right approach.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -37,75 +37,67 @@ export const usePriceProposal = ( | |||
setProposal({}); | |||
}; | |||
|
|||
const handleProposal = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider extracting the price proposal subscription logic into a custom hook to improve code organization, testability, and maintainability.
The current implementation could be simplified by extracting the subscription logic into a custom hook while maintaining proper cleanup. Here's a suggested approach:
// useProposalSubscription.ts
const useProposalSubscription = (config: ProposalConfig) => {
const [proposal, setProposal] = useState({});
const [isLoading, setIsLoading] = useState({ rise: false, fall: false });
const derivAPI = getDerivAPI();
useEffect(() => {
if (!config.isValid) return;
const subscribeToType = async (type: "rise" | "fall") => {
const contractConfig = {
proposal: 1,
subscribe: 1,
amount: config.price,
basis: config.basis,
contract_type: type === "rise" ? "CALL" : "PUT",
currency: "USD",
duration: config.duration,
duration_unit: "m",
symbol: config.symbol,
};
try {
await derivAPI.subscribeStream(
contractConfig,
handleProposalResponse(type),
`PROPOSAL_${type.toUpperCase()}`
);
} catch (err) {
handleError(err, type);
}
};
setIsLoading({ rise: true, fall: true });
Promise.all([subscribeToType("rise"), subscribeToType("fall")]);
return () => derivAPI.unsubscribeByPrefix("PROPOSAL_");
}, [config]);
return { proposal, isLoading };
};
// Original component
const useTradingPanel = () => {
const { proposal, isLoading } = useProposalSubscription({
price: debouncedPrice,
duration: debouncedDuration,
basis,
symbol,
isValid: !durationError && !priceError && is_rise_fall_valid
});
return { proposal, clearProposal, isLoading };
};
This approach:
- Separates subscription logic into a focused custom hook
- Maintains proper cleanup via useEffect
- Reduces nesting and improves testability
- Keeps all functionality intact while being more maintainable
Reverts #27
Summary by Sourcery
Bug Fixes: