-
Notifications
You must be signed in to change notification settings - Fork 14
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
docs: Add Rise/Fall business specification document #71
base: master
Are you sure you want to change the base?
docs: Add Rise/Fall business specification document #71
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new business specification document for the Rise/Fall trade type. The document details the business logic, user journey, technical implementation, and UI/UX considerations for this feature. No diagrams generated as the changes look simple and do not need a visual representation. 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 @ashkan-deriv - I've reviewed your changes - here's some feedback:
Overall Comments:
- This is a good start, but consider adding a section on security considerations, such as input validation and protection against replay attacks.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
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.
I've reviewed the Rise/Fall Business Specification document and found it to be well-structured and comprehensive. I've provided several suggestions to improve clarity, consistency, and completeness. Feel free to reach out with any questions or instructions related to this or future reviews, and I'll be happy to assist.
Follow-up suggestion: @devloai help implement the suggested changes to the business specification document
- Configurable minimum and maximum values | ||
- Default: 10 units of the selected currency | ||
|
||
3. **Allow Equals** (Optional): When enabled, the user also wins if: |
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.
There seems to be a duplication in the 'Allow Equals' description. Both Rise and Fall conditions state 'the exit price is equal to the entry price'. Please clarify if this is intended or if there should be different conditions for Rise vs Fall trades when using 'Allow Equals'.
The Rise/Fall trade type integrates with the trading API through: | ||
|
||
1. **Product Configuration API**: | ||
- Endpoint: `/v1/market/products/rise_fall/config` |
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.
The API endpoint path shown here (/v1/market/products/rise_fall/config
) seems to be specific to Rise/Fall, but our API structure typically uses a pattern where the product ID is a parameter, not part of the path itself. Consider updating to reflect the actual endpoint format, such as /v1/market/products/{product_id}/config
where product_id
would be 'rise_fall'.
|
||
3. **Trade Execution API**: | ||
- Endpoint: `/buy` | ||
- Request Parameters: |
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.
The Trade Execution API section lists trade_type
with values 'CALL' and 'PUT', but in the Configuration Structure section (line 128-136), these are mapped to 'contractType'. Please ensure consistent terminology throughout the document to avoid confusion - either standardize on trade_type
or contractType
.
1. **Trade Store**: Manages trade parameters (stake, duration, trade type) | ||
2. **Client Store**: Manages user-specific data (currency, account) | ||
3. **Market Store**: Manages selected market information | ||
4. **Product Config Store**: Manages market-specific configuration |
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.
There's a reference to a 'Product Config Store', but this doesn't match our current store structure based on the repository insights. According to our repository structure, configurations are typically handled through the tradeStore which includes product configurations. Consider updating this to align with the actual implementation.
@@ -0,0 +1,230 @@ | |||
# Rise/Fall Trade Type - Business Specification | |||
|
|||
## Overview |
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.
Consider adding a table of contents at the beginning of the document to improve navigation. This would be helpful given the length and detailed structure of the specification.
- Rise animation shows an upward price movement | ||
- Fall animation shows a downward price movement | ||
|
||
## Error Handling |
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.
The Error Handling section is quite brief compared to other sections. Consider expanding this with more specific error codes, validation rules, and user-facing error messages that would be shown in different scenarios. This would help both developers and QA team understand the expected behavior in error conditions.
Summary by Sourcery
Documentation: