-
Notifications
You must be signed in to change notification settings - Fork 75
fix: add BigInt-aware template evaluation for slippage calculations #435
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
Conversation
- Add support for mathematical operations in template variables (multiplication, division, addition, subtraction)
- Detect BigInt strings (>15 digits) and perform precision-preserving calculations
- Support common slippage values (0.1%, 0.5%, 1%, 5%) with exact fraction handling
- Add comprehensive unit tests for BigInt template math operations
- Enables direct slippage calculation in template expressions like: {{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}
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.
Pull request overview
This PR adds BigInt-aware template evaluation to support mathematical operations in template expressions, specifically targeting slippage calculations for Uniswap v3 operations. The implementation enables direct calculations like {{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}} while preserving precision for large integer values.
Key changes:
- Added mathematical operation detection and parsing for multiplication, division, addition, and subtraction in template expressions
- Implemented BigInt detection (strings >15 digits) with precision-preserving arithmetic using Go's
math/bigpackage - Added special handling for common slippage values (0.999, 0.995, 0.99, 0.95) to avoid floating-point precision errors
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| core/taskengine/vm.go | Adds core BigInt math evaluation functions (detectMathematicalOperation, parseMathExpression, isBigIntString, evaluateBigIntMath, evaluateVariablePath) and integrates BigInt-aware math into the existing resolveVariablePath method |
| core/taskengine/vm_template_bigint_math_test.go | Comprehensive test suite covering multiplication with various slippage values, division by floats, and edge cases including very large BigInt values and fallback scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // evaluateVariablePath is a helper to evaluate a variable path (used recursively) | ||
| func evaluateVariablePath(jsvm *goja.Runtime, varPath string, currentVars map[string]any) (interface{}, bool) { | ||
| // Similar to resolveVariablePath but without security validation (already validated) | ||
| script := varPath | ||
| if strings.Contains(varPath, ".") { | ||
| parts := strings.Split(varPath, ".") | ||
| if len(parts) > 1 { | ||
| for i := 1; i < len(parts); i++ { | ||
| if strings.Contains(parts[i], "-") { | ||
| basePath := parts[0] | ||
| for j := 1; j < i; j++ { | ||
| basePath += "." + parts[j] | ||
| } | ||
| propertyName := parts[i] | ||
| remainingParts := "" | ||
| if i+1 < len(parts) { | ||
| remainingParts = "." + strings.Join(parts[i+1:], ".") | ||
| if strings.Contains(remainingParts, "-") { | ||
| remainingPartsParts := parts[i+1:] | ||
| remainingParts = "" | ||
| for _, part := range remainingPartsParts { | ||
| if strings.Contains(part, "-") { | ||
| remainingParts += `["` + part + `"]` | ||
| } else { | ||
| remainingParts += "." + part | ||
| } | ||
| } | ||
| } | ||
| } | ||
| script = fmt.Sprintf(`%s["%s"]%s`, basePath, propertyName, remainingParts) | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| script = fmt.Sprintf(`(() => { try { return %s; } catch(e) { return undefined; } })()`, script) | ||
|
|
||
| if evaluated, err := jsvm.RunString(script); err == nil { | ||
| exportedValue := evaluated.Export() | ||
| if exportedValue != nil && fmt.Sprintf("%v", exportedValue) != "undefined" { | ||
| return exportedValue, true | ||
| } | ||
| } | ||
|
|
||
| return nil, false | ||
| } |
Copilot
AI
Dec 10, 2025
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 evaluateVariablePath function bypasses security validation by not calling ValidateCodeInjection on the varPath parameter. This creates a potential security vulnerability where malicious code could be injected through the right operand of a mathematical expression, since this function is called from evaluateBigIntMath in the default case. The comment acknowledges this ("without security validation (already validated)"), but the validation only happens for the initial expression, not for resolved variable paths that might contain injection attempts.
| resultBig := new(big.Int) | ||
| if _, ok1 := originalBig.SetString("3030137700988171", 10); ok1 { | ||
| if _, ok2 := resultBig.SetString(result, 10); ok2 { | ||
| if resultBig.Cmp(originalBig) >= 0 { |
Copilot
AI
Dec 10, 2025
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 variable 'resultBig' is declared here and shadows the 'resultBig' variable declared at line 52 in the same test function. While this works due to the nested scope of the if statement at line 50, it can lead to confusion. Consider using a different variable name like 'resultBigVerify' or 'resultForComparison' to make the code clearer.
| resultBig := new(big.Int) | |
| if _, ok1 := originalBig.SetString("3030137700988171", 10); ok1 { | |
| if _, ok2 := resultBig.SetString(result, 10); ok2 { | |
| if resultBig.Cmp(originalBig) >= 0 { | |
| resultBigVerify := new(big.Int) | |
| if _, ok1 := originalBig.SetString("3030137700988171", 10); ok1 { | |
| if _, ok2 := resultBigVerify.SetString(result, 10); ok2 { | |
| if resultBigVerify.Cmp(originalBig) >= 0 { |
core/taskengine/vm.go
Outdated
| rightValue = float64(val) | ||
| case int: | ||
| rightValue = float64(val) |
Copilot
AI
Dec 10, 2025
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.
Converting int64 and int types to float64 can result in precision loss for large integers (integers larger than 2^53). If the right operand is a large integer that should be treated as BigInt, this conversion will lose precision. The code should preserve integer types when dealing with BigInt operations, or at least check if the integer is too large for safe float64 conversion.
| rightValue = float64(val) | |
| case int: | |
| rightValue = float64(val) | |
| rightValue = strconv.FormatInt(val, 10) | |
| case int: | |
| rightValue = strconv.Itoa(val) |
core/taskengine/vm.go
Outdated
| } | ||
|
|
||
| // Check if string contains only digits (and optional leading minus sign) | ||
| matched, _ := regexp.MatchString(`^-?\d+$`, str) |
Copilot
AI
Dec 10, 2025
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 regex error from MatchString is silently ignored. Consider either handling the error properly or using a pre-compiled regex pattern as a package-level variable.
| multiplier := big.NewInt(9950) // 0.995 * 10000 | ||
| resultBig := new(big.Int).Mul(bigValue, multiplier) | ||
| resultBig.Div(resultBig, big.NewInt(10000)) |
Copilot
AI
Dec 10, 2025
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 test is using an incorrect precision factor compared to the implementation. The test uses a multiplier of 9950 with divisor 10000 (4 decimal places precision), but the actual implementation in vm.go uses 99500 with divisor 100000 (5 decimal places precision) for the 0.995 slippage value. This inconsistency means the test's expected value doesn't match what the implementation actually produces.
| multiplier := big.NewInt(9950) // 0.995 * 10000 | |
| resultBig := new(big.Int).Mul(bigValue, multiplier) | |
| resultBig.Div(resultBig, big.NewInt(10000)) | |
| multiplier := big.NewInt(99500) // 0.995 * 100000 | |
| resultBig := new(big.Int).Mul(bigValue, multiplier) | |
| resultBig.Div(resultBig, big.NewInt(100000)) |
| func TestBigIntTemplateMath_Multiplication(t *testing.T) { | ||
| vm, err := NewVMWithData(&model.Task{ | ||
| Task: &avsproto.Task{ | ||
| Id: "test_task", | ||
| Trigger: &avsproto.TaskTrigger{ | ||
| Id: "trigger1", | ||
| Name: "test_trigger", | ||
| }, | ||
| }, | ||
| }, nil, testutil.GetTestSmartWalletConfig(), nil) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("expect vm initialized, got error: %v", err) | ||
| } | ||
|
|
||
| // Test case 1: 0.1% slippage (multiplier 0.999) | ||
| t.Run("0.1% slippage (0.999 multiplier)", func(t *testing.T) { | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", // Large BigInt string | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "uniswapv3_pool": map[string]interface{}{ | ||
| "slippage": 0.999, // 0.1% slippage | ||
| }, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 * 0.999 = 3027104563287162.829 | ||
| // With integer math (100000 precision): (3030137700988171 * 99900) / 100000 = 3027104563287162 | ||
| // Note: Due to floating point precision, the result may vary slightly | ||
| // The important thing is that it's less than the original (slippage applied) | ||
| expected := "3027104563287162" | ||
| if result != expected { | ||
| // Check if result is within acceptable range (within 0.1% of expected) | ||
| resultBig := new(big.Int) | ||
| expectedBig := new(big.Int) | ||
| if _, ok1 := resultBig.SetString(result, 10); ok1 { | ||
| if _, ok2 := expectedBig.SetString(expected, 10); ok2 { | ||
| diff := new(big.Int).Sub(resultBig, expectedBig) | ||
| diff.Abs(diff) | ||
| // Allow difference up to 0.1% of expected value | ||
| tolerance := new(big.Int).Div(expectedBig, big.NewInt(1000)) | ||
| if diff.Cmp(tolerance) > 0 { | ||
| t.Errorf("expected %s, got %s (difference too large)", expected, result) | ||
| } | ||
| } else { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| } else { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| } | ||
|
|
||
| // Verify result is less than original (slippage applied) | ||
| originalBig := new(big.Int) | ||
| resultBig := new(big.Int) | ||
| if _, ok1 := originalBig.SetString("3030137700988171", 10); ok1 { | ||
| if _, ok2 := resultBig.SetString(result, 10); ok2 { | ||
| if resultBig.Cmp(originalBig) >= 0 { | ||
| t.Errorf("result should be less than original for slippage, got %s >= %s", result, "3030137700988171") | ||
| } | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| // Test case 2: 0.5% slippage (multiplier 0.995) | ||
| t.Run("0.5% slippage (0.995 multiplier)", func(t *testing.T) { | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "uniswapv3_pool": map[string]interface{}{ | ||
| "slippage": 0.995, // 0.5% slippage | ||
| }, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 * 0.995 = 3014987012483230.145 | ||
| // With integer math (100000 precision): (3030137700988171 * 99500) / 100000 = 3014987012483230 | ||
| expected := "3014987012483230" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
|
|
||
| // Test case 3: 1% slippage (multiplier 0.99) | ||
| t.Run("1% slippage (0.99 multiplier)", func(t *testing.T) { | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "uniswapv3_pool": map[string]interface{}{ | ||
| "slippage": 0.99, // 1% slippage | ||
| }, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 * 0.99 = 2999836323978289.29 | ||
| // With integer math (100000 precision): (3030137700988171 * 99000) / 100000 = 2999836323978289 | ||
| expected := "2999836323978289" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
|
|
||
| // Test case 4: 5% slippage (multiplier 0.95) | ||
| t.Run("5% slippage (0.95 multiplier)", func(t *testing.T) { | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "uniswapv3_pool": map[string]interface{}{ | ||
| "slippage": 0.95, // 5% slippage | ||
| }, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 * 0.95 = 2878630815938762.45 | ||
| // With integer math (100000 precision): (3030137700988171 * 95000) / 100000 = 2878630815938762 | ||
| expected := "2878630815938762" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestBigIntTemplateMath_Division(t *testing.T) { | ||
| vm, err := NewVMWithData(&model.Task{ | ||
| Task: &avsproto.Task{ | ||
| Id: "test_task", | ||
| Trigger: &avsproto.TaskTrigger{ | ||
| Id: "trigger1", | ||
| Name: "test_trigger", | ||
| }, | ||
| }, | ||
| }, nil, testutil.GetTestSmartWalletConfig(), nil) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("expect vm initialized, got error: %v", err) | ||
| } | ||
|
|
||
| t.Run("BigInt division by float", func(t *testing.T) { | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "divisor": 1.005, // Divide by 1.005 | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut / settings.divisor}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 / 1.005 ≈ 3015062389042956.22 | ||
| // With integer math (100000 precision): (3030137700988171 * 100000) / 100500 = 3015062389042956 | ||
| expected := "3015062389042956" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestBigIntTemplateMath_EdgeCases(t *testing.T) { | ||
| vm, err := NewVMWithData(&model.Task{ | ||
| Task: &avsproto.Task{ | ||
| Id: "test_task", | ||
| Trigger: &avsproto.TaskTrigger{ | ||
| Id: "trigger1", | ||
| Name: "test_trigger", | ||
| }, | ||
| }, | ||
| }, nil, testutil.GetTestSmartWalletConfig(), nil) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("expect vm initialized, got error: %v", err) | ||
| } | ||
|
|
||
| t.Run("Small BigInt (should still work)", func(t *testing.T) { | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "amountOut": "1000000", // Smaller number | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "slippage": 0.995, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.amountOut * settings.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 1000000 * 0.995 = 995000 | ||
| expected := "995000" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Very large BigInt", func(t *testing.T) { | ||
| // Test with a very large number that exceeds JavaScript safe integer | ||
| largeValue := "999999999999999999999999999999" | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "amountOut": largeValue, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "slippage": 0.995, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.amountOut * settings.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Calculate expected value using big.Int | ||
| bigValue := new(big.Int) | ||
| bigValue.SetString(largeValue, 10) | ||
| multiplier := big.NewInt(9950) // 0.995 * 10000 | ||
| resultBig := new(big.Int).Mul(bigValue, multiplier) | ||
| resultBig.Div(resultBig, big.NewInt(10000)) | ||
| expected := resultBig.String() | ||
|
|
||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Non-BigInt string (should fall back to normal evaluation)", func(t *testing.T) { | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "amountOut": "100", // Small number, not BigInt | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "slippage": 0.995, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.amountOut * settings.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Should still work, but may use JavaScript math | ||
| // Expected: 100 * 0.995 = 99.5 | ||
| if result == "" || result == "undefined" { | ||
| t.Errorf("expected a result, got %s", result) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Invalid expression (no operator)", func(t *testing.T) { | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.amountOut}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Should return the value as-is | ||
| expected := "3030137700988171" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestBigIntTemplateMath_RealWorldScenario(t *testing.T) { | ||
| vm, err := NewVMWithData(&model.Task{ | ||
| Task: &avsproto.Task{ | ||
| Id: "test_task", | ||
| Trigger: &avsproto.TaskTrigger{ | ||
| Id: "trigger1", | ||
| Name: "test_trigger", | ||
| }, | ||
| }, | ||
| }, nil, testutil.GetTestSmartWalletConfig(), nil) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("expect vm initialized, got error: %v", err) | ||
| } | ||
|
|
||
| t.Run("Real Uniswap swap scenario with 0.5% slippage", func(t *testing.T) { | ||
| // Simulate the exact scenario from the user's error | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", // From quote | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "uniswapv3_pool": map[string]interface{}{ | ||
| "slippage": 0.995, // 0.5% slippage tolerance | ||
| }, | ||
| } | ||
|
|
||
| // This is what would be used in amountOutMinimum parameter | ||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 * 0.995 = 3014987012483230 | ||
| // This should be less than the quoted amount, allowing for 0.5% slippage | ||
| expected := "3014987012483230" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
|
|
||
| // Verify it's less than the original quote (slippage applied) | ||
| originalBig := new(big.Int) | ||
| originalBig.SetString("3030137700988171", 10) | ||
| resultBig := new(big.Int) | ||
| resultBig.SetString(result, 10) | ||
| if resultBig.Cmp(originalBig) >= 0 { | ||
| t.Errorf("result should be less than original for slippage, got %s >= %s", result, "3030137700988171") | ||
| } | ||
| }) | ||
| } |
Copilot
AI
Dec 10, 2025
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 test suite is missing test coverage for addition and subtraction operations with BigInt values. While the implementation in vm.go includes support for "+" and "-" operators, there are no corresponding test cases to verify their correctness, especially considering the bug in the float handling logic for these operations.
| t.Run("BigInt division by float", func(t *testing.T) { | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "divisor": 1.005, // Divide by 1.005 | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut / settings.divisor}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 / 1.005 ≈ 3015062389042956.22 | ||
| // With integer math (100000 precision): (3030137700988171 * 100000) / 100500 = 3015062389042956 | ||
| expected := "3015062389042956" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) |
Copilot
AI
Dec 10, 2025
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 division test only covers division by float values. There is no test coverage for BigInt division by another BigInt (integer division), which is an important use case that should be tested to ensure correctness, especially for potential division by zero scenarios.
core/taskengine/vm.go
Outdated
| } | ||
| } | ||
| // Check for subtraction with spaces around it (to avoid matching hyphens in variable names) | ||
| if matched, _ := regexp.MatchString(`\s-\s`, expr); matched { |
Copilot
AI
Dec 10, 2025
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 regex error from MatchString is silently ignored. While this regex pattern is unlikely to fail compilation, it's better practice to either handle the error properly or compile the regex once as a package-level variable to guarantee correctness at startup. The same issue exists at line 1757.
| t.Run("0.1% slippage (0.999 multiplier)", func(t *testing.T) { | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", // Large BigInt string | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "uniswapv3_pool": map[string]interface{}{ | ||
| "slippage": 0.999, // 0.1% slippage | ||
| }, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 * 0.999 = 3027104563287162.829 | ||
| // With integer math (100000 precision): (3030137700988171 * 99900) / 100000 = 3027104563287162 | ||
| // Note: Due to floating point precision, the result may vary slightly | ||
| // The important thing is that it's less than the original (slippage applied) | ||
| expected := "3027104563287162" | ||
| if result != expected { | ||
| // Check if result is within acceptable range (within 0.1% of expected) | ||
| resultBig := new(big.Int) | ||
| expectedBig := new(big.Int) | ||
| if _, ok1 := resultBig.SetString(result, 10); ok1 { | ||
| if _, ok2 := expectedBig.SetString(expected, 10); ok2 { | ||
| diff := new(big.Int).Sub(resultBig, expectedBig) | ||
| diff.Abs(diff) | ||
| // Allow difference up to 0.1% of expected value | ||
| tolerance := new(big.Int).Div(expectedBig, big.NewInt(1000)) | ||
| if diff.Cmp(tolerance) > 0 { | ||
| t.Errorf("expected %s, got %s (difference too large)", expected, result) | ||
| } | ||
| } else { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| } else { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| } | ||
|
|
||
| // Verify result is less than original (slippage applied) | ||
| originalBig := new(big.Int) | ||
| resultBig := new(big.Int) | ||
| if _, ok1 := originalBig.SetString("3030137700988171", 10); ok1 { | ||
| if _, ok2 := resultBig.SetString(result, 10); ok2 { | ||
| if resultBig.Cmp(originalBig) >= 0 { | ||
| t.Errorf("result should be less than original for slippage, got %s >= %s", result, "3030137700988171") | ||
| } | ||
| } | ||
| } | ||
| }) |
Copilot
AI
Dec 10, 2025
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 VM instance is shared across all sub-tests in TestBigIntTemplateMath_Multiplication, which means the sub-tests are not isolated and modify the same vm.vars map. If tests run in parallel or if one test fails and leaves the VM in an unexpected state, it could affect other tests. Consider creating a new VM instance for each sub-test or resetting vm.vars at the beginning of each sub-test.
Critical fixes: - Fix addition/subtraction bug: properly scale both operands to maintain precision - Add security validation to evaluateVariablePath to prevent code injection - Fix operator precedence: handle * and / before + and - correctly Code quality improvements: - Extract magic numbers into named constants (precision scales, rounding factors) - Add depth limit to prevent infinite recursion in resolveVariablePath - Preserve int64/int as BigInt instead of converting to float64 (prevents precision loss) - Use pre-compiled regex patterns for better performance - Add division by zero checks for both float and BigInt operations Test improvements: - Fix test precision mismatch (use 99500/100000 instead of 9950/10000) - Fix variable shadowing (rename resultBig to resultBigVerify) - Fix test isolation (create new VM instance for each test) - Add missing test coverage: * BigInt division by BigInt * BigInt addition/subtraction with floats * BigInt addition/subtraction with BigInt * Division by zero handling All tests passing. Addresses all 13 Copilot review comments.
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.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| } else { | ||
| // For other values, use general calculation with higher precision | ||
| multiplierFloat := rightFloat * float64(bigIntPrecisionScale) | ||
| multiplier = int64(multiplierFloat + bigIntRoundingOffset) // Round to nearest |
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.
Bug: Rounding fails for negative float operands in BigInt math
The rounding logic using + bigIntRoundingOffset (0.5) only works correctly for positive floats. For negative floats, this approach produces off-by-one errors. For example, -0.3 * 100000 + 0.5 = -29999.5 truncates to -29999 instead of the correct -30000. The fix requires checking the sign and subtracting 0.5 for negative values, or using math.Round().
Additional Locations (2)
| // Try parsing as float | ||
| if f, err := strconv.ParseFloat(val, 64); err == nil { | ||
| rightValue = f | ||
| } |
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.
Bug: BigInt string converted to lossy float64 before evaluation
When rightValue is a string in resolveVariablePathWithDepth, the code converts it to float64 via strconv.ParseFloat before passing to evaluateBigIntMath. This loses precision for large integer strings that could be parsed as BigInt. The evaluateBigIntMath function has proper BigInt string handling at lines 1847-1851, but this preprocessing bypasses it by converting to float64 first. Large integer string operands like "99999999999999999999" will have incorrect calculation results.
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // For float division, convert to integer math | ||
| // Use higher precision to avoid rounding errors | ||
| divisorFloat := rightFloat * float64(bigIntPrecisionScale) | ||
| divisor := int64(divisorFloat + bigIntRoundingOffset) // Round to nearest |
Copilot
AI
Dec 11, 2025
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.
Similar to the multiplication case, the rounding offset for division only works correctly for positive divisors. Negative divisors will experience incorrect rounding behavior. Additionally, the same issue exists in the addition (line 1946) and subtraction (line 1957) cases. Consider implementing a sign-aware rounding function.
| t.Run("BigInt division by zero (should handle gracefully)", func(t *testing.T) { | ||
| vm := createTestVM(t) | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "divisor": "0", // Zero divisor | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.amountOut / settings.divisor}}" | ||
|
|
||
| // Use recover to catch panic from division by zero | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| // Division by zero should be caught and handled, not panic | ||
| // If we get here, the error handling needs improvement | ||
| t.Logf("Division by zero caused panic (expected to be handled): %v", r) | ||
| } | ||
| }() | ||
|
|
||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Should handle gracefully (either error or fallback to normal evaluation) | ||
| // The result should not be a valid number or should be empty/undefined | ||
| if result != "" && result != "undefined" { | ||
| // If it returns something, verify it's not a valid calculation | ||
| resultBig := new(big.Int) | ||
| if _, ok := resultBig.SetString(result, 10); ok { | ||
| // If it parsed as a number, that's unexpected for division by zero | ||
| t.Logf("Division by zero returned a number: %s (this may be expected if falling back to JS evaluation)", result) | ||
| } | ||
| } | ||
| }) |
Copilot
AI
Dec 11, 2025
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 division by zero test expects graceful handling but accepts any non-error result, including valid numbers (line 247). This is too permissive and doesn't verify that the function actually handles division by zero correctly. The test should assert specific expected behavior (e.g., return empty string, "undefined", or verify no panic occurs with a specific fallback value).
| func TestBigIntTemplateMath_AdditionSubtraction(t *testing.T) { | ||
| t.Run("BigInt addition with float", func(t *testing.T) { | ||
| vm := createTestVM(t) | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "addend": 1000.5, // Add 1000.5 | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.amountOut + settings.addend}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 + 1000.5 = 3030137700989171 (rounded) | ||
| // With integer math (100000 precision): (3030137700988171 * 100000 + 100050) / 100000 = 3030137700989171 | ||
| expected := "3030137700989171" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("BigInt subtraction with float", func(t *testing.T) { | ||
| vm := createTestVM(t) | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "subtrahend": 1000.5, // Subtract 1000.5 | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.amountOut - settings.subtrahend}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 - 1000.5 = 3030137700987170 (rounded) | ||
| // With integer math (100000 precision): (3030137700988171 * 100000 - 100050) / 100000 = 3030137700987170 | ||
| expected := "3030137700987170" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("BigInt addition with BigInt", func(t *testing.T) { | ||
| vm := createTestVM(t) | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "addend": "1000000000000", // BigInt addend | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.amountOut + settings.addend}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 + 1000000000000 = 3031137700988171 | ||
| expected := "3031137700988171" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("BigInt subtraction with BigInt", func(t *testing.T) { | ||
| vm := createTestVM(t) | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "subtrahend": "1000000000000", // BigInt subtrahend | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.amountOut - settings.subtrahend}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 - 1000000000000 = 3029137700988171 | ||
| expected := "3029137700988171" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
| } |
Copilot
AI
Dec 11, 2025
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.
Missing test coverage for negative BigInt values. The implementation supports negative numbers (regex includes optional minus sign), but there are no tests verifying operations with negative BigInts (e.g., negative * positive, negative + negative, etc.).
| } else { | ||
| // For other values, use general calculation with higher precision | ||
| multiplierFloat := rightFloat * float64(bigIntPrecisionScale) | ||
| multiplier = int64(multiplierFloat + bigIntRoundingOffset) // Round to nearest |
Copilot
AI
Dec 11, 2025
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 rounding offset bigIntRoundingOffset (0.5) is added to perform rounding to nearest integer, but this only works correctly for positive numbers. For negative multipliers, adding 0.5 will round in the wrong direction (e.g., -1.6 + 0.5 = -1.1, truncates to -1 instead of -2). Consider using a conditional rounding approach or Math.Round equivalent that handles negative numbers correctly.
| rightValue, rightResolved := evaluateVariablePath(jsvm, rightOperand, currentVars) | ||
| if !rightResolved { | ||
| // Try parsing right operand as a literal number | ||
| if f, err := strconv.ParseFloat(rightOperand, 64); err == nil { | ||
| rightValue = f | ||
| rightResolved = true | ||
| } else { | ||
| return nil, false | ||
| } | ||
| } |
Copilot
AI
Dec 11, 2025
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 rightOperand is not validated for code injection before being passed to evaluateVariablePath. While evaluateVariablePath does perform validation, the right operand could also be parsed as a literal number (line 2066), which bypasses validation. If the parsing fails, a malicious input could potentially exploit the unvalidated path. Ensure validation occurs before any evaluation or parsing attempt.
| func TestBigIntTemplateMath_Multiplication(t *testing.T) { | ||
| // Test case 1: 0.1% slippage (multiplier 0.999) | ||
| t.Run("0.1% slippage (0.999 multiplier)", func(t *testing.T) { | ||
| vm := createTestVM(t) | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", // Large BigInt string | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "uniswapv3_pool": map[string]interface{}{ | ||
| "slippage": 0.999, // 0.1% slippage | ||
| }, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 * 0.999 = 3027104563287162.829 | ||
| // With integer math (100000 precision): (3030137700988171 * 99900) / 100000 = 3027104563287162 | ||
| // Note: Due to floating point precision, the result may vary slightly | ||
| // The important thing is that it's less than the original (slippage applied) | ||
| expected := "3027104563287162" | ||
| if result != expected { | ||
| // Check if result is within acceptable range (within 0.1% of expected) | ||
| resultBig := new(big.Int) | ||
| expectedBig := new(big.Int) | ||
| if _, ok1 := resultBig.SetString(result, 10); ok1 { | ||
| if _, ok2 := expectedBig.SetString(expected, 10); ok2 { | ||
| diff := new(big.Int).Sub(resultBig, expectedBig) | ||
| diff.Abs(diff) | ||
| // Allow difference up to 0.1% of expected value | ||
| tolerance := new(big.Int).Div(expectedBig, big.NewInt(1000)) | ||
| if diff.Cmp(tolerance) > 0 { | ||
| t.Errorf("expected %s, got %s (difference too large)", expected, result) | ||
| } | ||
| } else { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| } else { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| } | ||
|
|
||
| // Verify result is less than original (slippage applied) | ||
| originalBig := new(big.Int) | ||
| resultBigVerify := new(big.Int) | ||
| if _, ok1 := originalBig.SetString("3030137700988171", 10); ok1 { | ||
| if _, ok2 := resultBigVerify.SetString(result, 10); ok2 { | ||
| if resultBigVerify.Cmp(originalBig) >= 0 { | ||
| t.Errorf("result should be less than original for slippage, got %s >= %s", result, "3030137700988171") | ||
| } | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| // Test case 2: 0.5% slippage (multiplier 0.995) | ||
| t.Run("0.5% slippage (0.995 multiplier)", func(t *testing.T) { | ||
| vm := createTestVM(t) | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "uniswapv3_pool": map[string]interface{}{ | ||
| "slippage": 0.995, // 0.5% slippage | ||
| }, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 * 0.995 = 3014987012483230.145 | ||
| // With integer math (100000 precision): (3030137700988171 * 99500) / 100000 = 3014987012483230 | ||
| expected := "3014987012483230" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
|
|
||
| // Test case 3: 1% slippage (multiplier 0.99) | ||
| t.Run("1% slippage (0.99 multiplier)", func(t *testing.T) { | ||
| vm := createTestVM(t) | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "uniswapv3_pool": map[string]interface{}{ | ||
| "slippage": 0.99, // 1% slippage | ||
| }, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 * 0.99 = 2999836323978289.29 | ||
| // With integer math (100000 precision): (3030137700988171 * 99000) / 100000 = 2999836323978289 | ||
| expected := "2999836323978289" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
|
|
||
| // Test case 4: 5% slippage (multiplier 0.95) | ||
| t.Run("5% slippage (0.95 multiplier)", func(t *testing.T) { | ||
| vm := createTestVM(t) | ||
| vm.vars["contractRead1"] = map[string]interface{}{ | ||
| "data": map[string]interface{}{ | ||
| "quoteExactInputSingle": map[string]interface{}{ | ||
| "amountOut": "3030137700988171", | ||
| }, | ||
| }, | ||
| } | ||
| vm.vars["settings"] = map[string]interface{}{ | ||
| "uniswapv3_pool": map[string]interface{}{ | ||
| "slippage": 0.95, // 5% slippage | ||
| }, | ||
| } | ||
|
|
||
| expr := "{{contractRead1.data.quoteExactInputSingle.amountOut * settings.uniswapv3_pool.slippage}}" | ||
| result := vm.preprocessTextWithVariableMapping(expr) | ||
|
|
||
| // Expected: 3030137700988171 * 0.95 = 2878630815938762.45 | ||
| // With integer math (100000 precision): (3030137700988171 * 95000) / 100000 = 2878630815938762 | ||
| expected := "2878630815938762" | ||
| if result != expected { | ||
| t.Errorf("expected %s, got %s", expected, result) | ||
| } | ||
| }) | ||
| } |
Copilot
AI
Dec 11, 2025
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.
Missing test coverage for operator precedence (e.g., "{{a + b * c}}" should evaluate multiplication before addition) and chained operations (e.g., "{{a - b - c}}"). These are important edge cases for the mathematical expression parser, especially given the right-to-left parsing in the implementation.
core/taskengine/vm.go
Outdated
| if resolved, ok := evaluateVariablePath(jsvm, fmt.Sprintf("%v", rightValue), currentVars); ok { | ||
| return evaluateBigIntMath(leftStr, operator, resolved, jsvm, currentVars) |
Copilot
AI
Dec 11, 2025
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 recursive call to evaluateBigIntMath inside the default case could lead to infinite recursion if the resolved value is neither a recognized type nor resolves to a valid type. There is no depth tracking for this recursion, which differs from the depth tracking in resolveVariablePathWithDepth. Consider adding a depth parameter to evaluateBigIntMath or removing this recursive call to prevent potential stack overflow.
core/taskengine/vm.go
Outdated
| MaxExecutionDepth = 50 // Maximum depth for nested workflow execution | ||
|
|
||
| // BigInt math precision constants | ||
| bigIntPrecisionScale = int64(100000) // 5 decimal places precision |
Copilot
AI
Dec 11, 2025
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 constant name bigIntPrecisionScale could be more descriptive. Consider renaming it to something like bigIntDecimalPrecisionScale or bigIntFixedPointScale to clarify that it represents a fixed-point scaling factor for decimal precision (5 decimal places).
core/taskengine/vm.go
Outdated
| bigIntMathSubtractionRegex = regexp.MustCompile(`\s-\s`) | ||
| bigIntStringRegex = regexp.MustCompile(`^-?\d+$`) |
Copilot
AI
Dec 11, 2025
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 bigIntMathSubtractionRegex is compiled at package initialization, but bigIntStringRegex is compiled at the same time. While regex compilation is efficient, consider whether these patterns could be simplified. The subtraction regex \s-\s is quite simple and could be replaced with a string-based check using strings.Contains for better performance, especially if this code is called frequently.
Co-authored-by: Copilot <[email protected]>
Security fixes: - Add ValidateCodeInjection for rightOperand before evaluation * Prevents code injection vulnerabilities in template expressions * Validates before passing to evaluateVariablePath or parsing as literal Bug fixes: - Fix BigInt string precision loss: check isBigIntString before float64 conversion * Prevents precision loss for large integer strings (e.g., "99999999999999999999") * Preserves BigInt strings for evaluateBigIntMath to handle correctly Recursion safety: - Add depth tracking to evaluateBigIntMath to prevent infinite recursion * Created evaluateBigIntMathWithDepth with depth parameter * Enforces maxBigIntMathDepth limit to prevent stack overflow * Updates recursive calls to track and increment depth Code quality improvements: - Rename bigIntPrecisionScale to bigIntDecimalPrecisionScale * More descriptive name clarifies fixed-point scaling factor purpose * Updated all 6 occurrences throughout the codebase - Replace regex with string-based check for subtraction detection * Use strings.Contains(expr, " - ") instead of regex for better performance * Removed unused bigIntMathSubtractionRegex variable Test improvements: - Improve division by zero test with specific assertions * Verify no panic occurs * Assert result is empty string, "undefined", or unchanged expression * Fail if valid number is returned (unexpected behavior) All tests passing. Addresses Copilot and Cursor bot review comments.
|
This approach is too specialized and should not be included in the general contract_write node. Discarding it. |
Note
Adds BigInt-precise math to template evaluation in VM (supporting */+- and slippage), with comprehensive unit tests.
core/taskengine/vm.go):*,/,+,-in template variable resolution with precision/rounding constants and recursion depth limits.strconvusage.core/taskengine/vm_template_bigint_math_test.go):Written by Cursor Bugbot for commit 241fe13. Configure here.