Add future-timestamp buffer to fixed-spread strategy#14
Conversation
Allow Alpaca publish_time to be slightly ahead of block.timestamp via a deployer-tunable grace period (default 20s) so minor clock skew does not revert orders. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe PR introduces a configurable grace period for oracle timestamp validation in the st0x-fixed-spread strategy. A new GUI parameter ChangesOracle timestamp grace period feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
strategy/st0x-fixed-spread.rain (1)
134-156:⚠️ Potential issue | 🔴 Critical | ⚖️ Poor tradeoffFix staleness guard reasoning:
sub(now() oracle-publish-time)may revert when publish_time is in the futureRainlang’s integer model doesn’t support negative integers; subtracting below zero is an error unless “saturating”/clamping math is used. With the future-timestamp guard allowing
oracle-publish-time > now()(up tofuture-timestamp-buffer), the staleness ensure atstrategy/st0x-fixed-spread.rain:151-156(sub(now() oracle-publish-time) <= max-staleness) can hit a negative subtraction and revert—contradicting the comment’s claim that a negative value “is always less than max-staleness” and “silently pass.” Update the staleness logic to use saturating/clamped subtraction (or change the logic/comment so it matches actual failure behavior).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@strategy/st0x-fixed-spread.rain` around lines 134 - 156, The staleness ensure can revert because Rainlang integer subtraction errors on negative results: change the staleness check to use a saturating/clamped subtraction instead of raw sub(now() oracle-publish-time). Concretely, compute age = (oracle-publish-time > now()) ? 0 : sub(now() oracle-publish-time) (or use your language's saturating-sub helper) and then ensure less-than-or-equal-to(age max-staleness) ; also update the comment to reflect that we clamp future publish times to zero for staleness checks. Use the existing symbols oracle-publish-time, now(), future-timestamp-buffer, max-staleness and the staleness :ensure block to locate and change the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@strategy/st0x-fixed-spread.rain`:
- Around line 82-90: The configuration for the "future-timestamp-buffer" binding
lacks an upper bound allowing arbitrarily large values; update the binding named
future-timestamp-buffer to add a max constraint (e.g., max: 60) alongside the
existing min: 0 so the parameter cannot be set to an excessively large number
and still allows a small clock-skew grace period; ensure the change is applied
where the binding is defined (future-timestamp-buffer / "Future Timestamp Buffer
(seconds)") and keep the default at 20.
---
Outside diff comments:
In `@strategy/st0x-fixed-spread.rain`:
- Around line 134-156: The staleness ensure can revert because Rainlang integer
subtraction errors on negative results: change the staleness check to use a
saturating/clamped subtraction instead of raw sub(now() oracle-publish-time).
Concretely, compute age = (oracle-publish-time > now()) ? 0 : sub(now()
oracle-publish-time) (or use your language's saturating-sub helper) and then
ensure less-than-or-equal-to(age max-staleness) ; also update the comment to
reflect that we clamp future publish times to zero for staleness checks. Use the
existing symbols oracle-publish-time, now(), future-timestamp-buffer,
max-staleness and the staleness :ensure block to locate and change the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fef0d6fc-ee99-4ed0-a1b6-40d208f79806
📒 Files selected for processing (2)
strategy/registrystrategy/st0x-fixed-spread.rain
| - binding: future-timestamp-buffer | ||
| name: Future Timestamp Buffer (seconds) | ||
| description: > | ||
| Grace period (in seconds) added to block.timestamp when | ||
| checking that Alpaca publish_time is not in the future. | ||
| Allows minor clock skew between Alpaca, the signer, and | ||
| the chain without reverting. | ||
| default: 20 | ||
| min: 0 |
There was a problem hiding this comment.
Add maximum constraint to future-timestamp-buffer.
The field has min: 0 but no max constraint. Without an upper bound, a deployer could configure an arbitrarily large buffer (e.g., 86400 seconds = 24 hours), effectively disabling the future-timestamp protection. This would allow oracle timestamps far in the future to pass validation, undermining the safety guarantees.
🛡️ Proposed fix to add a reasonable maximum
- binding: future-timestamp-buffer
name: Future Timestamp Buffer (seconds)
description: >
Grace period (in seconds) added to block.timestamp when
checking that Alpaca publish_time is not in the future.
Allows minor clock skew between Alpaca, the signer, and
the chain without reverting.
default: 20
min: 0
+ max: 60A maximum of 60 seconds should be sufficient for legitimate clock skew while preventing abuse.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@strategy/st0x-fixed-spread.rain` around lines 82 - 90, The configuration for
the "future-timestamp-buffer" binding lacks an upper bound allowing arbitrarily
large values; update the binding named future-timestamp-buffer to add a max
constraint (e.g., max: 60) alongside the existing min: 0 so the parameter cannot
be set to an excessively large number and still allows a small clock-skew grace
period; ensure the change is applied where the binding is defined
(future-timestamp-buffer / "Future Timestamp Buffer (seconds)") and keep the
default at 20.
2 participants
Summary
future-timestamp-bufferbinding (default 20 seconds) tost0x-fixed-spread.rain.add(now() future-timestamp-buffer)instead ofnow(), tolerating minor clock skew between Alpaca, the signer, and the chain.strategy/registryto point at the new strategy commit.Rebased onto
ST0x-Technology/st0x-oracle-servermain.Test plan
now() + bufferMade with Cursor
Summary by CodeRabbit
Release Notes
New Features
Chores