fix: add validation for lookbackPeriod in ParseGetTwapsArgs#210
Conversation
WalkthroughThe PR adds validation to ParseGetTwapsArgs in precompiles/oracle/types.go: it checks the lookback argument is a non-nil *big.Int, enforces the lookback is positive, and verifies it fits in uint64 before conversion; public function signatures are unchanged. A new test file precompiles/oracle/types_test.go adds TestParseTwapsArgsValidation covering valid input, nil, zero/negative, overflow, wrong type, and incorrect argument count. CHANGELOG.md is updated with a Fixed entry noting the added validation for lookbackPeriod. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive validation to the ParseGetTwapsArgs function in the oracle precompile to prevent runtime panics and data corruption. The fix addresses security and stability issues where nil pointers, negative values, or values exceeding uint64 could cause panics or silent overflow.
Key changes:
- Added nil pointer check to prevent panic when
lookbackPeriodis nil - Added positive value validation to reject zero and negative
lookbackPeriodvalues - Added overflow protection to ensure
lookbackPeriodfits within uint64 bounds
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| precompiles/oracle/types.go | Enhanced ParseGetTwapsArgs with nil, sign, and overflow validations for the lookbackPeriod parameter |
| precompiles/oracle/types_test.go | Added comprehensive unit tests covering all validation scenarios including nil, zero, negative, overflow, type mismatch, and argument count edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Thaleszh
left a comment
There was a problem hiding this comment.
Very good, just missing two things:
- A text on changelog on the part 'Fixed'
- run
make lint-fix
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @precompiles/oracle/types_test.go:
- Around line 41-46: Update the failing test case "invalid - overflow value
larger than uint64" in types_test.go so its errContains matches the actual error
string returned by the implementation in types.go; change errContains from
"lookback period overflow" to "lookback period must fit in uint64" (or
alternatively change the error text in types.go to "lookback period overflow" if
you prefer to keep the test as-is).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdprecompiles/oracle/types.goprecompiles/oracle/types_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- precompiles/oracle/types.go
🧰 Additional context used
🧬 Code graph analysis (1)
precompiles/oracle/types_test.go (1)
precompiles/oracle/types.go (1)
ParseGetTwapsArgs(43-69)
🔇 Additional comments (4)
precompiles/oracle/types_test.go (3)
1-8: LGTM!Package declaration and imports are appropriate for the test file.
67-80: LGTM!The test execution loop is well-structured with proper use of subtests and appropriate assertions for both error and success cases.
10-80: Excellent test coverage for validation requirements.The test comprehensively covers all validation paths specified in the PR objectives:
- Nil pointer handling
- Type validation
- Positivity checks (rejecting zero and negative values)
- Uint64 overflow prevention
- Argument count validation
This aligns well with the goal of bringing precompile behavior in line with wasmbinding validation.
CHANGELOG.md (1)
20-20: LGTM!The changelog entry accurately documents the fix and properly references issue #204.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
precompiles/oracle/types_test.go (1)
18-22: Consider verifying the actual result value in the valid test case.The valid test case confirms success but doesn't verify that
result.LookbackSecondsequals the expected value of100. This would strengthen the assertion.🔎 Proposed enhancement
{ name: "valid - positive lookback period", args: []interface{}{big.NewInt(100)}, expectError: false, },Then in the test execution block (around line 75), you could add:
} else { require.NoError(t, err) require.NotNil(t, result) + // Verify the actual value for the valid case + if tc.name == "valid - positive lookback period" { + require.Equal(t, uint64(100), result.LookbackSeconds) + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
precompiles/oracle/types_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
precompiles/oracle/types_test.go (1)
precompiles/oracle/types.go (1)
ParseGetTwapsArgs(43-69)
🔇 Additional comments (4)
precompiles/oracle/types_test.go (4)
1-8: LGTM! Clean imports and package structure.The package declaration and imports are appropriate for the test file. Using
testify/requirefor assertions is idiomatic and provides clear error messages.
10-17: LGTM! Well-structured table-driven test.The test function uses the table-driven pattern effectively, which is a Go best practice. The test case structure clearly captures inputs and expected outcomes.
18-64: Comprehensive test coverage aligns with PR objectives.The test cases thoroughly validate all failure modes outlined in the PR objectives:
- Nil pointer prevention (line 23-28)
- Non-positive value rejection (lines 29-40)
- Overflow prevention (lines 41-46)
Additionally covers edge cases for type safety and argument count validation. Error message assertions correctly match the implementation in
types.go.
67-79: LGTM! Proper test execution with appropriate assertions.The test execution correctly:
- Uses
t.Runfor subtests, enabling better output and potential parallelization- Applies appropriate assertions for both error and success paths
- Validates error messages with
require.Containsfor flexibility- Ensures result state consistency (nil on error, non-nil on success)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
precompiles/oracle/types_test.go (1)
67-83: Test execution is correct, with one optional improvement.The test loop properly validates both success and failure cases. The assertions are appropriate and thorough.
Optional improvement: Lines 78-80 use string matching on
tc.nameto determine when to verifyLookbackSeconds. Consider adding anexpectedLookbackSeconds *uint64field to the test case struct for more maintainable value assertions.🔎 Proposed refactor
testCases := []struct { name string args []interface{} expectError bool errContains string + expectedLookbackSeconds *uint64 }{ { name: "valid - positive lookback period", args: []interface{}{big.NewInt(100)}, expectError: false, + expectedLookbackSeconds: func() *uint64 { v := uint64(100); return &v }(), }, // ... other test cases ... } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { result, err := ParseGetTwapsArgs(tc.args) if tc.expectError { require.Error(t, err) require.Contains(t, err.Error(), tc.errContains) require.Nil(t, result) } else { require.NoError(t, err) require.NotNil(t, result) - // Verify the actual value for the valid case - if tc.name == "valid - positive lookback period" { + if tc.expectedLookbackSeconds != nil { - require.Equal(t, uint64(100), result.LookbackSeconds) + require.Equal(t, *tc.expectedLookbackSeconds, result.LookbackSeconds) } } }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
precompiles/oracle/types_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
precompiles/oracle/types_test.go (1)
precompiles/oracle/types.go (1)
ParseGetTwapsArgs(43-69)
🔇 Additional comments (2)
precompiles/oracle/types_test.go (2)
1-8: LGTM! Clean test setup.The package declaration and imports are appropriate. Using
requirefrom testify is the right choice for validation tests where subsequent assertions depend on earlier ones.
10-65: Excellent test coverage!The test cases comprehensively cover all validation scenarios mentioned in issue #204:
- Nil pointer dereference prevention (line 25)
- Non-positive values rejection (lines 31, 37)
- Overflow prevention with uint64 boundary check (line 43)
- Type validation (line 49)
- Argument count validation (lines 55, 61)
All error message strings correctly match the implementation in
types.go.
Thaleszh
left a comment
There was a problem hiding this comment.
There is something in your code editor that undid part of the lint on type_test.go. You can check your last commit and you will see the indentation of the whole file changed.
You'll need to make lint-fix again.
2e4910e to
8c48c77
Compare
fix: add validation for TWAP lookbackPeriod in oracle precompile
Nil pointer → panic
Negative values or > MaxUint64 → wrap/overflow
Zero is accepted even though wasmbinding rejects it
Fixes #204"