1183 implement verify for pricepredict future handler #1204
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive enhancement to the price prediction client and handler in the Prophet system. The changes focus on adding a verification mechanism, refactoring the HTTP request handling, and improving data structures. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant PricePredictorService
participant VerificationService
Client->>PricePredictorService: Predict Request
PricePredictorService-->>Client: Prediction Response
Client->>VerificationService: Verify Request
VerificationService-->>Client: Verification Response
Possibly related PRs
Suggested Labels
✨ Finishing Touches
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? 🪧 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
Documentation and Community
|
|
Hey @mn13 and thank you for opening this pull request! 👋🏼 It looks like you forgot to add a changelog entry for your changes. Make sure to add a changelog entry in the 'CHANGELOG.md' file. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
prophet/handlers/pricepred/pricepred.go (3)
91-100: Avoid overshadowing theresvariable for readability.
InsideExecute, you reassign theresvariable to hold backtesting responses, potentially confusing future readers. Consider renaming one of these variables (e.g.,btRes) to keep logic clear.- var backtestingRes *BacktestingResponse ... - res, err := s.c.Backtesting(ctx, ...) - backtestingRes = &res + var btRes *BacktestingResponse ... + btResData, err := s.c.Backtesting(ctx, ...) + btRes = &btResData
165-170: Add a short doc-comment to clarify usage.
Consider adding a brief summary of eachOutputDatafield to assist maintainers in understanding the data structure's purpose.
288-320: Make verification ratio configurable.
The verification ratio is currently hardcoded to0.01. Allowing dynamic configuration might make this more flexible for different confidence levels.prophet/handlers/pricepred/client_test.go (1)
60-90: Consider using local mock server to reduce external call dependency.
Presently,TestVerifyis skipped because it relies on external HTTP calls. Mocking the endpoint or injecting an interface can make tests more consistent and independent of external environments.prophet/handlers/pricepred/client.go (2)
18-18: Add a brief docstring forverifyURLfield.
Clarity on the expected endpoint and usage helps future maintainers.
125-164: Enhance error handling for non-JSON responses.
Currently, a non-JSON response from the server yields an unmarshal error. Consider logging server-provided content to help with debugging.if res.StatusCode != http.StatusOK { return empty, fmt.Errorf("unexpected status code: %d. Server returned error: %s", res.StatusCode, response) } var resResp Resp +// Optionally log or store `response` in logs for debugging if needed err = json.Unmarshal(response, &resResp) ...prophet/handlers/pricepred/pricepred_test.go (1)
Line range hint
41-126: Enhance test coverage for SolverReceipt scenarios.The TestBuildOutput function would benefit from additional test cases to ensure robust handling of SolverReceipt:
- Nil/empty SolverReceipt
- Invalid BloomFilter data
- Zero/negative CountItems
- Edge cases for large CountItems values
This will help validate error handling and edge cases for the new fields.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
prophet/handlers/pricepred/client.go(3 hunks)prophet/handlers/pricepred/client_test.go(2 hunks)prophet/handlers/pricepred/pricepred.go(4 hunks)prophet/handlers/pricepred/pricepred_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
prophet/handlers/pricepred/client_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
prophet/handlers/pricepred/pricepred.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/handlers/pricepred/pricepred_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
prophet/handlers/pricepred/client.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (13)
prophet/handlers/pricepred/pricepred.go (5)
72-89: Consider additional validation for fixture edge cases.
Currently, ifi.FalsePositiveRate[0]exceedsi.FalsePositiveRate[1], thefprcan be > 1, which may be an unintended edge case. Also, negative or extremely large timestamps fori.Datemight affect time-based logic.Would you like me to create a script to locate and check all calls to
ToPredictRequestto confirm whether these edge cases are handled or tested?
174-174: No issues found.
The invocation ofgetOutputABIType()is clear and consistent.
271-278: Validateres.SolverReceiptbefore building the output.
Ensureres.SolverReceiptis always non-nil. If there's a scenario where it may be nil, consider adding a safety check to prevent a nil pointer reference.
324-349: Tuple decoding logic looks good.
ThedecodeOutputfunction is straightforward, and argument mapping appears correct.
350-359: Local struct definitions match ABI fields.
This approach ensures accurate decoding of complex types. No issues found.prophet/handlers/pricepred/client_test.go (1)
5-5: Import statement is appropriate for BloomFilter decoding.
The addition of"encoding/base64"is correctly used inTestVerify.prophet/handlers/pricepred/client.go (6)
24-30: Confirm correctness of the verify endpoint.
Double-check that the path"/api/task/inference/verify"is accurate and functional.
56-56: Generic post usage is clean and straightforward.
The refactoring to usepost[PredictRequest, PredictResponse]is good for DRY principles.
107-108: Solid extension for backtesting.
The samepostlogic is consistently applied, improving maintainability.
110-114: Definition ofVerifyRequestis clear.
The struct fields are well-labeled, aiding readability.
116-119:VerifyResponsestruct looks good.
The fields and types match the verification scenario.
121-123: Introduction ofVerifymethod is consistent.
The method mirrors existing patterns inPredictandBacktesting.prophet/handlers/pricepred/pricepred_test.go (1)
77-80: LGTM! Test data structure updated with SolverReceipt field.The test case correctly initializes the new SolverReceipt field with appropriate test data.
| SolverReceipt: struct { | ||
| BloomFilter []byte | ||
| CountItems *big.Int | ||
| }{ | ||
| BloomFilter: []byte("BgAAAAAAAAApt1DE"), | ||
| CountItems: big.NewInt(3), | ||
| }, |
There was a problem hiding this comment.
Fix type mismatch in CountItems between input and expected output.
There's a type mismatch in the CountItems field:
- Input:
CountItems: 3(int) - Expected:
CountItems: big.NewInt(3)(*big.Int)
This could lead to subtle bugs if the actual implementation expects consistency in types.
Consider using the same type in both structures. If *big.Int is the correct type, update the ResponseSolverReceipt struct to use *big.Int for CountItems.
SolverReceipt: ResponseSolverReceipt{
BloomFilter: []byte("BgAAAAAAAAApt1DE"),
- CountItems: 3,
+ CountItems: big.NewInt(3),
},Committable suggestion skipped: line range outside the PR's diff.
Implementation of verify for pricepredict handler.
First make a bit refactoring of client to make common generic
postfunction that used in all client public methods.Then I've added a Verify to client.
I've extended
OutputDatawith SolverReceipt for using it inVerifyinPricePredictorSolidity.Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates
The updates provide more robust and detailed price prediction verification capabilities.