feat(prophet): pricepred model handler #1168
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThe pull request introduces a new price prediction handler in the Prophet system. A new package Changes
Sequence DiagramsequenceDiagram
participant Client
participant PricePredictorSolidity
participant PredictionService
Client->>PricePredictorSolidity: Execute(input)
PricePredictorSolidity->>PricePredictorSolidity: decodeInput
PricePredictorSolidity->>PredictionService: Predict(request)
PredictionService-->>PricePredictorSolidity: Response
PricePredictorSolidity->>PricePredictorSolidity: encodeOutput
PricePredictorSolidity-->>Client: Return encoded prediction
✨ 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 @Pitasi 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. |
mn13
previously approved these changes
Jan 16, 2025
|
@Pitasi can you pls merge with main so we can merge this and deploy in devnet? |
This is a vary bare future handler for the price prediction model we have today, and that essentially is an experiment to showcase the tech architecture (in fact, the date for the prediction in the request is hardcoded). We lack some essential features such as a way of configuring the prediction endpoint (again, hardcoded for now). This is also our first tentative of accepting a bytes array coming from solidity, mapping it into a json, and then doing the other way around for the response.
mn13
force-pushed
the
pricepred-future-handler
branch
from
2b458ed to
4792f38
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
prophet/handlers/pricepred/pricepred.go(1 hunks)prophet/handlers/pricepred/pricepred_test.go(1 hunks)warden/app/app.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
warden/app/app.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/pricepred.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
warden/app/app.go (1)
92-92: LGTM!The changes correctly integrate the price prediction handler:
- Import the pricepred package
- Register the handler with the prophet system
Also applies to: 250-251
Comment on lines
+36
to
+73
| func TestEncodeOutput(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| request []string | ||
| expected string | ||
| }{ | ||
| { | ||
| name: "single element list", | ||
| request: []string{"bitcoin", "tether", "uniswap"}, | ||
| expected: "00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000018d6e7f01865e90000000000000000000000000000000000000000000000000000002387ffdba3cf9c000000000000000000000000000000000000000000000000026b6b97cf726620", | ||
| }, | ||
| } | ||
|
|
||
| for _, c := range cases { | ||
| t.Run(c.name, func(t *testing.T) { | ||
| req := Request{ | ||
| SolverInput: RequestSolverInput{ | ||
| Tokens: c.request, | ||
| TargetDate: "2022-01-01", | ||
| AdversaryMode: false, | ||
| }, | ||
| FalsePositiveRate: 0.01, | ||
| } | ||
| res := Response{ | ||
| SolverOutput: map[string]float64{ | ||
| "uniswap": 17.435131034851075, | ||
| "tether": 1.000115715622902, | ||
| "bitcoin": 45820.74676003456, | ||
| }, | ||
| } | ||
|
|
||
| actual, err := encodeOutput(req, res) | ||
| require.NoError(t, err) | ||
|
|
||
| require.Equal(t, c.expected, hex.EncodeToString(actual)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update test with dynamic date and add error scenarios.
The test has two issues:
- Uses a hardcoded date from 2022, which is in the past
- Missing test cases for error scenarios
Consider:
- Using a dynamic future date
- Adding test cases for:
- Empty token list
- Invalid token names
- Zero/negative predictions
- Missing predictions for requested tokens
func TestEncodeOutput(t *testing.T) {
+ futureDate := time.Now().AddDate(1, 0, 0).Format("2006-01-02")
cases := []struct {
name string
request []string
+ date string
expected string
+ wantErr bool
}{
{
name: "single element list",
request: []string{"bitcoin", "tether", "uniswap"},
+ date: futureDate,
expected: "00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000018d6e7f01865e90000000000000000000000000000000000000000000000000000002387ffdba3cf9c000000000000000000000000000000000000000000000000026b6b97cf726620",
+ wantErr: false,
},
+ {
+ name: "empty token list",
+ request: []string{},
+ date: futureDate,
+ expected: "",
+ wantErr: true,
+ },
}Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+12
to
+34
| func TestDecodeInput(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| input string | ||
| expected []string | ||
| }{ | ||
| { | ||
| name: "single element list", | ||
| input: "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAANFVEgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==", | ||
| expected: []string{"ETH"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, c := range cases { | ||
| t.Run(c.name, func(t *testing.T) { | ||
| bz, err := base64.StdEncoding.DecodeString(c.input) | ||
| require.NoError(t, err) | ||
| actual, err := decodeInput(bz) | ||
| require.NoError(t, err) | ||
| require.Equal(t, c.expected, actual) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
The current test suite only covers a single token. Consider adding test cases for:
- Multiple tokens
- Empty input
- Invalid base64 input
- Invalid token format
func TestDecodeInput(t *testing.T) {
cases := []struct {
name string
input string
expected []string
}{
{
name: "single element list",
input: "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAANFVEgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==",
expected: []string{"ETH"},
},
+ {
+ name: "multiple tokens",
+ input: "...", // Add base64 encoded input for multiple tokens
+ expected: []string{"BTC", "ETH", "USDT"},
+ },
+ {
+ name: "empty input",
+ input: "",
+ expected: nil,
+ },
+ {
+ name: "invalid base64",
+ input: "invalid-base64",
+ expected: nil,
+ },
}Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+75
to
+93
| func TestPredict(t *testing.T) { | ||
| //t.Skip("this test relies on external HTTP call") | ||
|
|
||
| ctx := context.Background() | ||
| res, err := Predict(ctx, Request{ | ||
| SolverInput: RequestSolverInput{ | ||
| Tokens: []string{"bitcoin", "tether", "uniswap"}, | ||
| TargetDate: "2022-01-01", | ||
| AdversaryMode: false, | ||
| }, | ||
| FalsePositiveRate: 0.01, | ||
| }) | ||
| require.NoError(t, err) | ||
| require.Len(t, res.SolverOutput, 3) | ||
|
|
||
| for token, pred := range res.SolverOutput { | ||
| require.NotZero(t, pred, "prediction for %t is zero", token) | ||
| } | ||
| } |
There was a problem hiding this comment.
Mock external HTTP calls and update the test date.
The test has several issues:
- Makes real HTTP calls which can make tests flaky and slow
- Uses a hardcoded date from 2022
- Has a commented-out skip line instead of actively managing the test execution
Consider:
- Using a mock HTTP client
- Using a dynamic future date
- Properly managing test execution with environment variables
Here's a suggested implementation:
+type mockHTTPClient struct {
+ DoFunc func(req *http.Request) (*http.Response, error)
+}
+
+func (m *mockHTTPClient) Do(req *http.Request) (*http.Response, error) {
+ return m.DoFunc(req)
+}
+
func TestPredict(t *testing.T) {
- //t.Skip("this test relies on external HTTP call")
+ if testing.Short() {
+ t.Skip("skipping test in short mode")
+ }
+
+ futureDate := time.Now().AddDate(1, 0, 0).Format("2006-01-02")
+ originalClient := client
+ defer func() { client = originalClient }()
+
+ mockResp := &http.Response{
+ StatusCode: http.StatusOK,
+ Body: io.NopCloser(strings.NewReader(`{
+ "solverOutput": {
+ "bitcoin": 50000,
+ "tether": 1,
+ "uniswap": 20
+ }
+ }`)),
+ }
+
+ client = &mockHTTPClient{
+ DoFunc: func(req *http.Request) (*http.Response, error) {
+ return mockResp, nil
+ },
+ }
ctx := context.Background()
res, err := Predict(ctx, Request{
SolverInput: RequestSolverInput{
Tokens: []string{"bitcoin", "tether", "uniswap"},
- TargetDate: "2022-01-01",
+ TargetDate: futureDate,
AdversaryMode: false,
},
FalsePositiveRate: 0.01,
})Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+106
to
+109
| func (s PricePredictorSolidity) Verify(ctx context.Context, input []byte, output []byte) error { | ||
| // todo: verify output | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Implement the Verify method.
The Verify method is a critical security feature that is currently not implemented. This could lead to acceptance of invalid or malicious responses.
The method should verify:
- Response signature if available
- Response format and data types
- Prediction values are within reasonable bounds
- All requested tokens are present in the response
Would you like me to generate an implementation for the Verify method?
Comment on lines
+18
to
+22
| var URL = "https://prediction.devnet.wardenprotocol.org/task/inference/solve" | ||
|
|
||
| var client = http.Client{ | ||
| Timeout: 3 * time.Second, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make URL configurable and review timeout duration.
- The prediction service URL is hardcoded to a devnet endpoint, which limits flexibility across different environments.
- The 3-second timeout might be too short for some network conditions.
Consider:
- Making the URL configurable through environment variables or configuration
- Increasing the timeout or making it configurable
- Adding retries for transient failures
-var URL = "https://prediction.devnet.wardenprotocol.org/task/inference/solve"
+var (
+ URL = getEnvOrDefault(
+ "PREDICTION_SERVICE_URL",
+ "https://prediction.devnet.wardenprotocol.org/task/inference/solve",
+ )
+ timeout = getEnvDurationOrDefault("PREDICTION_SERVICE_TIMEOUT", 10*time.Second)
+)
var client = http.Client{
- Timeout: 3 * time.Second,
+ Timeout: timeout,
}
+
+func getEnvOrDefault(key, defaultValue string) string {
+ if value := os.Getenv(key); value != "" {
+ return value
+ }
+ return defaultValue
+}
+
+func getEnvDurationOrDefault(key string, defaultValue time.Duration) time.Duration {
+ if value := os.Getenv(key); value != "" {
+ if duration, err := time.ParseDuration(value); err == nil {
+ return duration
+ }
+ }
+ return defaultValue
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var URL = "https://prediction.devnet.wardenprotocol.org/task/inference/solve" | |
| var client = http.Client{ | |
| Timeout: 3 * time.Second, | |
| } | |
| var ( | |
| URL = getEnvOrDefault( | |
| "PREDICTION_SERVICE_URL", | |
| "https://prediction.devnet.wardenprotocol.org/task/inference/solve", | |
| ) | |
| timeout = getEnvDurationOrDefault("PREDICTION_SERVICE_TIMEOUT", 10*time.Second) | |
| ) | |
| var client = http.Client{ | |
| Timeout: timeout, | |
| } | |
| func getEnvOrDefault(key, defaultValue string) string { | |
| if value := os.Getenv(key); value != "" { | |
| return value | |
| } | |
| return defaultValue | |
| } | |
| func getEnvDurationOrDefault(key string, defaultValue time.Duration) time.Duration { | |
| if value := os.Getenv(key); value != "" { | |
| if duration, err := time.ParseDuration(value); err == nil { | |
| return duration | |
| } | |
| } | |
| return defaultValue | |
| } |
Comment on lines
+56
to
+76
| func decodeInput(input []byte) ([]string, error) { | ||
| typ, err := abi.NewType("string[]", "string[]", nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| args := abi.Arguments{ | ||
| {Type: typ}, | ||
| } | ||
|
|
||
| unpackArgs, err := args.Unpack(input) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| tokens, ok := unpackArgs[0].([]string) | ||
| if !ok { | ||
| return nil, fmt.Errorf("failed to unpack input") | ||
| } | ||
|
|
||
| return tokens, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add token validation.
The function needs better error handling and input validation:
- Type assertion error message is not descriptive enough
- Missing validation for token strings
- No maximum limit on number of tokens
Consider:
func decodeInput(input []byte) ([]string, error) {
+ const maxTokens = 100 // Prevent DoS attacks
+
typ, err := abi.NewType("string[]", "string[]", nil)
if err != nil {
return nil, err
}
args := abi.Arguments{
{Type: typ},
}
unpackArgs, err := args.Unpack(input)
if err != nil {
return nil, err
}
tokens, ok := unpackArgs[0].([]string)
if !ok {
- return nil, fmt.Errorf("failed to unpack input")
+ return nil, fmt.Errorf("expected []string, got %T", unpackArgs[0])
+ }
+
+ if len(tokens) > maxTokens {
+ return nil, fmt.Errorf("too many tokens: %d (max: %d)", len(tokens), maxTokens)
+ }
+
+ for i, token := range tokens {
+ if !isValidToken(token) {
+ return nil, fmt.Errorf("invalid token at index %d: %s", i, token)
+
}
return tokens, nil
}
+
+func isValidToken(token string) bool {
+ // Add validation logic for token strings
+ return len(token) > 0 && len(token) <= 10 && !strings.ContainsAny(token, " \t\n")
+}Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+28
to
+54
| func (s PricePredictorSolidity) Execute(ctx context.Context, input []byte) ([]byte, error) { | ||
| tokens, err := decodeInput(input) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| req := Request{ | ||
| SolverInput: RequestSolverInput{ | ||
| Tokens: tokens, | ||
| TargetDate: "2022-01-01", | ||
| AdversaryMode: false, | ||
| }, | ||
| FalsePositiveRate: 0.01, | ||
| } | ||
|
|
||
| res, err := Predict(ctx, req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| encodedRes, err := encodeOutput(req, res) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return encodedRes, nil | ||
| } |
There was a problem hiding this comment.
Remove hardcoded date and add input validation.
The Execute method has several issues:
- Uses a hardcoded date from 2022
- Lacks input validation
- Missing error handling for empty token list
Consider:
func (s PricePredictorSolidity) Execute(ctx context.Context, input []byte) ([]byte, error) {
tokens, err := decodeInput(input)
if err != nil {
return nil, err
}
+ if len(tokens) == 0 {
+ return nil, fmt.Errorf("empty token list")
+ }
+
+ futureDate := time.Now().AddDate(1, 0, 0).Format("2006-01-02")
+
req := Request{
SolverInput: RequestSolverInput{
Tokens: tokens,
- TargetDate: "2022-01-01",
+ TargetDate: futureDate,
AdversaryMode: false,
},
FalsePositiveRate: 0.01,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s PricePredictorSolidity) Execute(ctx context.Context, input []byte) ([]byte, error) { | |
| tokens, err := decodeInput(input) | |
| if err != nil { | |
| return nil, err | |
| } | |
| req := Request{ | |
| SolverInput: RequestSolverInput{ | |
| Tokens: tokens, | |
| TargetDate: "2022-01-01", | |
| AdversaryMode: false, | |
| }, | |
| FalsePositiveRate: 0.01, | |
| } | |
| res, err := Predict(ctx, req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| encodedRes, err := encodeOutput(req, res) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return encodedRes, nil | |
| } | |
| func (s PricePredictorSolidity) Execute(ctx context.Context, input []byte) ([]byte, error) { | |
| tokens, err := decodeInput(input) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if len(tokens) == 0 { | |
| return nil, fmt.Errorf("empty token list") | |
| } | |
| futureDate := time.Now().AddDate(1, 0, 0).Format("2006-01-02") | |
| req := Request{ | |
| SolverInput: RequestSolverInput{ | |
| Tokens: tokens, | |
| TargetDate: futureDate, | |
| AdversaryMode: false, | |
| }, | |
| FalsePositiveRate: 0.01, | |
| } | |
| res, err := Predict(ctx, req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| encodedRes, err := encodeOutput(req, res) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return encodedRes, nil | |
| } |
Comment on lines
+78
to
+104
| func encodeOutput(req Request, res Response) ([]byte, error) { | ||
| typ, err := abi.NewType("uint256[]", "", nil) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
| args := abi.Arguments{ | ||
| { | ||
| Type: typ, | ||
| Name: "SolverOutput", | ||
| Indexed: false, | ||
| }, | ||
| } | ||
|
|
||
| tokenPreds := make([]*big.Int, len(req.SolverInput.Tokens)) | ||
| for i, v := range req.SolverInput.Tokens { | ||
| decimals := big.NewFloat(1e16) | ||
| pred := big.NewFloat(res.SolverOutput[v]) | ||
| tokenPreds[i], _ = big.NewFloat(0).Mul(pred, decimals).Int(nil) | ||
| } | ||
|
|
||
| enc, err := args.Pack(tokenPreds) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return enc, nil | ||
| } |
There was a problem hiding this comment.
Fix error handling and make decimal places configurable.
Critical issues in the function:
- Uses
log.Fatalwhich is inappropriate for a library function - Hardcoded decimal places (1e16)
- Missing validation of response data
- No error handling for big.Float operations
Consider:
+const (
+ defaultDecimals = 16
+ maxDecimals = 18
+)
+
func encodeOutput(req Request, res Response) ([]byte, error) {
typ, err := abi.NewType("uint256[]", "", nil)
if err != nil {
- log.Fatal(err)
+ return nil, fmt.Errorf("failed to create ABI type: %w", err)
}
args := abi.Arguments{
{
Type: typ,
Name: "SolverOutput",
Indexed: false,
},
}
tokenPreds := make([]*big.Int, len(req.SolverInput.Tokens))
for i, v := range req.SolverInput.Tokens {
- decimals := big.NewFloat(1e16)
+ pred, ok := res.SolverOutput[v]
+ if !ok {
+ return nil, fmt.Errorf("missing prediction for token: %s", v)
+ }
+ if pred < 0 {
+ return nil, fmt.Errorf("negative prediction for token %s: %f", v, pred)
+ }
+
+ decimals := big.NewFloat(math.Pow10(defaultDecimals))
pred := big.NewFloat(res.SolverOutput[v])
- tokenPreds[i], _ = big.NewFloat(0).Mul(pred, decimals).Int(nil)
+ result, accuracy := big.NewFloat(0).Mul(pred, decimals).Int(nil)
+ if accuracy != big.Exact {
+ return nil, fmt.Errorf("loss of precision for token %s", v)
+ }
+ tokenPreds[i] = result
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func encodeOutput(req Request, res Response) ([]byte, error) { | |
| typ, err := abi.NewType("uint256[]", "", nil) | |
| if err != nil { | |
| log.Fatal(err) | |
| } | |
| args := abi.Arguments{ | |
| { | |
| Type: typ, | |
| Name: "SolverOutput", | |
| Indexed: false, | |
| }, | |
| } | |
| tokenPreds := make([]*big.Int, len(req.SolverInput.Tokens)) | |
| for i, v := range req.SolverInput.Tokens { | |
| decimals := big.NewFloat(1e16) | |
| pred := big.NewFloat(res.SolverOutput[v]) | |
| tokenPreds[i], _ = big.NewFloat(0).Mul(pred, decimals).Int(nil) | |
| } | |
| enc, err := args.Pack(tokenPreds) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return enc, nil | |
| } | |
| const ( | |
| defaultDecimals = 16 | |
| maxDecimals = 18 | |
| ) | |
| func encodeOutput(req Request, res Response) ([]byte, error) { | |
| typ, err := abi.NewType("uint256[]", "", nil) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create ABI type: %w", err) | |
| } | |
| args := abi.Arguments{ | |
| { | |
| Type: typ, | |
| Name: "SolverOutput", | |
| Indexed: false, | |
| }, | |
| } | |
| tokenPreds := make([]*big.Int, len(req.SolverInput.Tokens)) | |
| for i, v := range req.SolverInput.Tokens { | |
| pred, ok := res.SolverOutput[v] | |
| if !ok { | |
| return nil, fmt.Errorf("missing prediction for token: %s", v) | |
| } | |
| if pred < 0 { | |
| return nil, fmt.Errorf("negative prediction for token %s: %f", v, pred) | |
| } | |
| decimals := big.NewFloat(math.Pow10(defaultDecimals)) | |
| pred := big.NewFloat(res.SolverOutput[v]) | |
| result, accuracy := big.NewFloat(0).Mul(pred, decimals).Int(nil) | |
| if accuracy != big.Exact { | |
| return nil, fmt.Errorf("loss of precision for token %s", v) | |
| } | |
| tokenPreds[i] = result | |
| } | |
| enc, err := args.Pack(tokenPreds) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return enc, nil | |
| } |
Comment on lines
+130
to
+168
| func Predict(ctx context.Context, req Request) (Response, error) { | ||
| reqCtx, cancel := context.WithTimeout(ctx, 3*time.Second) | ||
| defer cancel() | ||
|
|
||
| reqBody, err := json.Marshal(req) | ||
| if err != nil { | ||
| return Response{}, err | ||
| } | ||
|
|
||
| httpReq, err := http.NewRequestWithContext(reqCtx, "POST", URL, bytes.NewReader(reqBody)) | ||
| if err != nil { | ||
| return Response{}, err | ||
| } | ||
|
|
||
| httpReq.Header.Set("Accept", "application/json") | ||
| httpReq.Header.Set("Content-Type", "application/json") | ||
|
|
||
| res, err := client.Do(httpReq) | ||
| if err != nil { | ||
| return Response{}, err | ||
| } | ||
| defer res.Body.Close() | ||
| response, err := io.ReadAll(res.Body) | ||
| if err != nil { | ||
| return Response{}, err | ||
| } | ||
|
|
||
| if res.StatusCode != http.StatusOK { | ||
| return Response{}, fmt.Errorf("unexpected status code: %d. Server returned error: %s", res.StatusCode, response) | ||
| } | ||
|
|
||
| var resResp Response | ||
| err = json.Unmarshal(response, &resResp) | ||
| if err != nil { | ||
| return Response{}, err | ||
| } | ||
|
|
||
| return resResp, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add retry mechanism.
The function has several issues:
- Duplicate timeout configuration
- No retry mechanism for transient failures
- Basic error handling
- Response body not limited in size
Consider:
+const (
+ maxResponseSize = 1 << 20 // 1MB
+ maxRetries = 3
+)
+
func Predict(ctx context.Context, req Request) (Response, error) {
- reqCtx, cancel := context.WithTimeout(ctx, 3*time.Second)
- defer cancel()
+ var resp Response
+ var lastErr error
reqBody, err := json.Marshal(req)
if err != nil {
return Response{}, err
}
- httpReq, err := http.NewRequestWithContext(reqCtx, "POST", URL, bytes.NewReader(reqBody))
- if err != nil {
- return Response{}, err
- }
+ for attempt := 0; attempt <= maxRetries; attempt++ {
+ if attempt > 0 {
+ // Exponential backoff
+ backoff := time.Duration(math.Pow(2, float64(attempt-1))) * time.Second
+ timer := time.NewTimer(backoff)
+ select {
+ case <-ctx.Done():
+ timer.Stop()
+ return Response{}, ctx.Err()
+ case <-timer.C:
+ }
+ }
- httpReq.Header.Set("Accept", "application/json")
- httpReq.Header.Set("Content-Type", "application/json")
+ httpReq, err := http.NewRequestWithContext(ctx, "POST", URL, bytes.NewReader(reqBody))
+ if err != nil {
+ return Response{}, err
+ }
- res, err := client.Do(httpReq)
- if err != nil {
- return Response{}, err
- }
- defer res.Body.Close()
- response, err := io.ReadAll(res.Body)
- if err != nil {
- return Response{}, err
- }
+ httpReq.Header.Set("Accept", "application/json")
+ httpReq.Header.Set("Content-Type", "application/json")
- if res.StatusCode != http.StatusOK {
- return Response{}, fmt.Errorf("unexpected status code: %d. Server returned error: %s", res.StatusCode, response)
- }
+ res, err := client.Do(httpReq)
+ if err != nil {
+ lastErr = err
+ continue
+ }
+ defer res.Body.Close()
- var resResp Response
- err = json.Unmarshal(response, &resResp)
- if err != nil {
- return Response{}, err
+ // Limit response size
+ limitedReader := io.LimitReader(res.Body, maxResponseSize)
+ response, err := io.ReadAll(limitedReader)
+ if err != nil {
+ lastErr = err
+ continue
+ }
+
+ if res.StatusCode != http.StatusOK {
+ lastErr = fmt.Errorf("unexpected status code: %d. Server returned error: %s", res.StatusCode, response)
+ // Don't retry on 4xx errors
+ if res.StatusCode/100 == 4 {
+ break
+ }
+ continue
+ }
+
+ if err := json.Unmarshal(response, &resp); err != nil {
+ return Response{}, fmt.Errorf("failed to unmarshal response: %w", err)
+ }
+
+ return resp, nil
}
- return resResp, nil
+ return Response{}, fmt.Errorf("max retries exceeded: %w", lastErr)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func Predict(ctx context.Context, req Request) (Response, error) { | |
| reqCtx, cancel := context.WithTimeout(ctx, 3*time.Second) | |
| defer cancel() | |
| reqBody, err := json.Marshal(req) | |
| if err != nil { | |
| return Response{}, err | |
| } | |
| httpReq, err := http.NewRequestWithContext(reqCtx, "POST", URL, bytes.NewReader(reqBody)) | |
| if err != nil { | |
| return Response{}, err | |
| } | |
| httpReq.Header.Set("Accept", "application/json") | |
| httpReq.Header.Set("Content-Type", "application/json") | |
| res, err := client.Do(httpReq) | |
| if err != nil { | |
| return Response{}, err | |
| } | |
| defer res.Body.Close() | |
| response, err := io.ReadAll(res.Body) | |
| if err != nil { | |
| return Response{}, err | |
| } | |
| if res.StatusCode != http.StatusOK { | |
| return Response{}, fmt.Errorf("unexpected status code: %d. Server returned error: %s", res.StatusCode, response) | |
| } | |
| var resResp Response | |
| err = json.Unmarshal(response, &resResp) | |
| if err != nil { | |
| return Response{}, err | |
| } | |
| return resResp, nil | |
| } | |
| const ( | |
| maxResponseSize = 1 << 20 // 1MB | |
| maxRetries = 3 | |
| ) | |
| func Predict(ctx context.Context, req Request) (Response, error) { | |
| var resp Response | |
| var lastErr error | |
| reqBody, err := json.Marshal(req) | |
| if err != nil { | |
| return Response{}, err | |
| } | |
| for attempt := 0; attempt <= maxRetries; attempt++ { | |
| if attempt > 0 { | |
| // Exponential backoff | |
| backoff := time.Duration(math.Pow(2, float64(attempt-1))) * time.Second | |
| timer := time.NewTimer(backoff) | |
| select { | |
| case <-ctx.Done(): | |
| timer.Stop() | |
| return Response{}, ctx.Err() | |
| case <-timer.C: | |
| } | |
| } | |
| httpReq, err := http.NewRequestWithContext(ctx, "POST", URL, bytes.NewReader(reqBody)) | |
| if err != nil { | |
| return Response{}, err | |
| } | |
| httpReq.Header.Set("Accept", "application/json") | |
| httpReq.Header.Set("Content-Type", "application/json") | |
| res, err := client.Do(httpReq) | |
| if err != nil { | |
| lastErr = err | |
| continue | |
| } | |
| defer res.Body.Close() | |
| // Limit response size | |
| limitedReader := io.LimitReader(res.Body, maxResponseSize) | |
| response, err := io.ReadAll(limitedReader) | |
| if err != nil { | |
| lastErr = err | |
| continue | |
| } | |
| if res.StatusCode != http.StatusOK { | |
| lastErr = fmt.Errorf("unexpected status code: %d. Server returned error: %s", res.StatusCode, response) | |
| // Don't retry on 4xx errors | |
| if res.StatusCode/100 == 4 { | |
| break | |
| } | |
| continue | |
| } | |
| if err := json.Unmarshal(response, &resp); err != nil { | |
| return Response{}, fmt.Errorf("failed to unmarshal response: %w", err) | |
| } | |
| return resp, nil | |
| } | |
| return Response{}, fmt.Errorf("max retries exceeded: %w", lastErr) | |
| } |
Svetomech
approved these changes
Jan 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a very bare future handler for the price prediction model we have today, and that essentially is an experiment to showcase the tech architecture (in fact, the date for the prediction in the request is hardcoded).
We lack some essential features, such as a way of configuring the prediction endpoint (again, hardcoded for now).
This is also our first tentative attempt at accepting a bytes array coming from solidity, mapping it into a json, and then doing the other way around for the response.
A sample Solidity smart contract used when testing:
which can be tested using forge, like this:
Summary by CodeRabbit
New Features
Tests
Improvements