Skip to content

[PT1-595] Refactor piecewise linear scale opcode#132

Open
SteMak wants to merge 2 commits into
release/1.2from
piecewise-linear-scale-opcode
Open

[PT1-595] Refactor piecewise linear scale opcode#132
SteMak wants to merge 2 commits into
release/1.2from
piecewise-linear-scale-opcode

Conversation

@SteMak

@SteMak SteMak commented Jun 11, 2026

Copy link
Copy Markdown

Change Summary

Migrate from (scale, timestamp) points to (scale, duration) args packing approach

Duration could be packed in 2 bytes vs 5 bytes timestamp, which saves ~36 Gas for transaction calldata cost

Testing & Verification

How was this tested?

  • Unit tests
  • Integration tests
  • Manual testing (describe steps)
  • Verified on staging

forge test --match-path test/PiecewiseLinearScale.t.sol --fuzz-runs 100000

Risk Assessment

Risk Level:

  • Low - Minor changes, no operational impact
  • Medium - Moderate changes, limited impact, standard rollback available
  • High - Significant changes, potential operational impact, complex rollback

Risks & Impact

Zero durations behavior consistency is particular point to check

Updated high-level docs PiecewiseLinearScale.pdf

@claude

claude Bot commented Jun 11, 2026

Copy link
Copy Markdown

Claude finished @SteMak's task —— View job


Code Review: Refactor piecewise linear scale opcode

I've reviewed the pull request that refactors from (scale, timestamp) points to (scale, duration) args packing approach. Here's my comprehensive feedback:

✅ Code Quality and Best Practices

Strengths:

  • Clean refactoring with clear separation of concerns between the builder and opcode logic
  • Consistent use of assembly memory-safe annotations for gas optimization
  • Good function naming that clearly indicates purpose (parseStartTimestamp, parseIntervalDuration, parsePointScale)
  • Proper use of unchecked blocks in _calcScaleNow where overflow is not possible

Suggestions:

  • Consider adding more detailed NatSpec comments for the parsing functions to explain the byte layout visually

🔍 Potential Bugs or Issues

  1. Zero Duration Handling: The implementation correctly handles zero durations by skipping to the next scale immediately. The test coverage confirms this behavior works as expected in multiple scenarios (single/double/wrapped).

  2. Division by Zero Protection: In line 149 of _calcScaleNow, there's a division by duration. The comment states duration != 0 but this relies on the while loop condition. The logic is sound but could benefit from an explicit assertion for clarity.

  3. Time Underflow Edge Case: The implementation uses block.timestamp subtraction which could theoretically underflow if system time is manipulated, though this is highly unlikely in practice.

⚡ Performance Considerations

Gas Optimization Verified:

  • The claim of ~36 gas savings per transaction is reasonable given:
    • Timestamp: 5 bytes (40 bits)
    • Duration: 2 bytes (16 bits)
    • Savings: 3 bytes × 12 gas/byte = 36 gas for calldata

Efficient Implementation:

  • Smart use of bitwise operations for parsing (shr in assembly)
  • Efficient loop structure that decreases timeLeft instead of accumulating durations
  • Early exit conditions optimize for common cases

Gas Benchmark Results:
The gas benchmark test shows consistent performance across different numbers of points (2-50), with average gas usage scaling linearly as expected.

🔒 Security Concerns

  1. Input Validation: The builder properly validates inputs with custom errors
  2. Arithmetic Safety: All arithmetic operations are either in unchecked blocks with documented safety or use standard overflow protection
  3. Access Control: No access control issues as this is a library/internal function

🧪 Test Coverage

Excellent Coverage:

  • Comprehensive fuzz testing for Dutch auction scenarios (both exact in/out)
  • Specific edge case testing for zero durations in multiple positions
  • Gas benchmarking to verify performance characteristics
  • Boundary condition testing (before start, after end, at exact points)

Test Highlights:

  • testFuzz_PiecewiseLinearScale_UnscaleValue: Verifies the mathematical correctness of scaling/unscaling
  • Zero duration tests: Cover single, double, wrapped, first, and last position scenarios
  • Basic test: Validates behavior at specific timestamps with mixed zero/non-zero durations

🎯 Specific Feedback on Zero Duration Behavior

The zero duration handling is well-implemented and thoroughly tested:

  • When duration is 0, the scale immediately jumps to the next point
  • Multiple consecutive zero durations are handled correctly
  • The implementation maintains consistency regardless of where zero durations appear

📝 Minor Suggestions

  1. Consider adding a comment in _calcScaleNow line 147 explicitly stating the division safety guarantee
  2. The magic number 5 (bytes per segment) could be defined as a constant for clarity
  3. Consider adding a test case for maximum duration values to ensure no unexpected behavior

✅ Conclusion

This is a well-executed refactoring that achieves its gas optimization goal while maintaining correctness and safety. The code is clean, the tests are comprehensive, and the edge cases are properly handled. The medium risk assessment is appropriate given the nature of the changes to core swap logic.

Recommendation: Ready for merge after addressing the minor suggestions if desired.

@SteMak

SteMak commented Jun 11, 2026

Copy link
Copy Markdown
Author

Updated high-level docs PiecewiseLinearScale.pdf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant