Harden oracle snapshots and feeless priority#333
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR makes two independent changes. First, it reduces the priority assigned to feeless transactions from Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🧹 Nitpick comments (3)
x/oracle/keeper/keeper_test.go (1)
746-761: ⚡ Quick winAdd a same-denom overwrite assertion to cover “latest-wins”.
This test verifies union + ordering, but not the key overwrite rule from
mergePriceSnapshotItems. Add a second incoming item for an existing denom and assert the final rate matches the second snapshot.🤖 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 `@x/oracle/keeper/keeper_test.go` around lines 746 - 761, The test currently checks union+ordering but misses verifying that later snapshots overwrite same-denom entries; update the keeper_test.go case around snapshot1/snapshot2 to add a third snapshot or an additional PriceSnapshotItem in snapshot2 with the same denom as snapshot1 (e.g., utils.KiiDenom) but a different ExchangeRate, call oracleKeeper.AddPriceSnapshot(ctx, ...) as done for others, then after GetPriceSnapshotOrDefault(ctx, 1000) assert that the stored item for that denom matches the second/new rate (verifying mergePriceSnapshotItems "latest-wins"); reference PriceSnapshotItems, NewPriceSnapshotItem, AddPriceSnapshot, GetPriceSnapshotOrDefault and mergePriceSnapshotItems when adding the overwrite assertion.ante/feeless.go (2)
46-46: ⚡ Quick winExtract the magic number to a named constant.
The divisor
1_000_000is a magic number without explanation. Consider extracting it to a named constant with documentation explaining the rationale for this specific value.♻️ Proposed refactor
At the package level:
// FeelessPriorityDivisor reduces the maximum priority for feeless transactions // to allow other critical transactions to potentially take precedence while // still maintaining high priority for oracle votes. const FeelessPriorityDivisor = 1_000_000Then update line 46:
- ctx = ctx.WithPriority(math.MaxInt64 / 1_000_000) + ctx = ctx.WithPriority(math.MaxInt64 / FeelessPriorityDivisor)🤖 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 `@ante/feeless.go` at line 46, Extract the magic divisor into a named package-level constant (e.g., FeelessPriorityDivisor) with a brief doc comment explaining why we reduce the max priority for feeless transactions; then replace the literal 1_000_000 in the ctx.WithPriority(math.MaxInt64 / 1_000_000) call with math.MaxInt64 / FeelessPriorityDivisor so the code reads ctx.WithPriority(math.MaxInt64 / FeelessPriorityDivisor) (referencing the ctx.WithPriority call in ante/feeless.go).
34-37: ⚡ Quick winClarify the incomplete comment.
The comment "This also force the TX to take" appears grammatically incomplete and doesn't explain what priority is being set for feeless transactions. Consider revising to clearly document the behavior.
📝 Suggested comment improvement
// AnteHandle executes the antehandler logic for feeless transactions -// This checks if the transaction is gasless and if so, it skips the fee deduction -// This also force the TX to take +// This checks if the transaction is feeless and if so, it skips the fee deduction +// and assigns it a high priority to ensure timely processing of oracle votes🤖 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 `@ante/feeless.go` around lines 34 - 37, The comment on FeelessDecorator.AnteHandle is grammatically incomplete and vague; update it to clearly state the behavior for gasless transactions (e.g., "skips fee deduction for gasless TXs and marks the transaction as high-priority/bypasses mempool min-fee checks so it will still be accepted and included in a block"), replacing "This also force the TX to take" with an explicit description of exactly what priority or flag the code sets and why.
🤖 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 `@x/oracle/keeper/keeper.go`:
- Around line 351-364: The loop currently returns true to stop iteration on the
first non-old snapshot, which prevents detecting any future-dated snapshots
later in the ascending order; change the iteration logic in PriceSnapshot.Walk
so it does not early-stop on a valid (non-old) snapshot — replace the "return
true, nil" early-stop with continued iteration (e.g., "return false, nil") and,
if you still need to record that a valid snapshot was found, set a local flag
(e.g., foundValidSnapshot) when encountering a non-old, non-future snapshot;
keep using timestampsToDelete for old snapshots and still check every snapshot
against currentTime and lookBackDuration to ensure future timestamps are
detected.
- Around line 341-354: The code writes a PriceSnapshot before validating its
timestamp, causing a write-then-fail race; before calling
k.PriceSnapshot.Set(ctx, snapshot.SnapshotTimestamp, snapshot) in keeper.go,
compute currentTime := ctx.BlockTime().Unix() and validate
snapshot.SnapshotTimestamp <= currentTime (and any other business rules) and
return an error if invalid, only then call k.PriceSnapshot.Set; keep the
existing k.PriceSnapshot.Walk logic for pruning but remove the expectation that
the Set will be followed by a future-date check.
---
Nitpick comments:
In `@ante/feeless.go`:
- Line 46: Extract the magic divisor into a named package-level constant (e.g.,
FeelessPriorityDivisor) with a brief doc comment explaining why we reduce the
max priority for feeless transactions; then replace the literal 1_000_000 in the
ctx.WithPriority(math.MaxInt64 / 1_000_000) call with math.MaxInt64 /
FeelessPriorityDivisor so the code reads ctx.WithPriority(math.MaxInt64 /
FeelessPriorityDivisor) (referencing the ctx.WithPriority call in
ante/feeless.go).
- Around line 34-37: The comment on FeelessDecorator.AnteHandle is grammatically
incomplete and vague; update it to clearly state the behavior for gasless
transactions (e.g., "skips fee deduction for gasless TXs and marks the
transaction as high-priority/bypasses mempool min-fee checks so it will still be
accepted and included in a block"), replacing "This also force the TX to take"
with an explicit description of exactly what priority or flag the code sets and
why.
In `@x/oracle/keeper/keeper_test.go`:
- Around line 746-761: The test currently checks union+ordering but misses
verifying that later snapshots overwrite same-denom entries; update the
keeper_test.go case around snapshot1/snapshot2 to add a third snapshot or an
additional PriceSnapshotItem in snapshot2 with the same denom as snapshot1
(e.g., utils.KiiDenom) but a different ExchangeRate, call
oracleKeeper.AddPriceSnapshot(ctx, ...) as done for others, then after
GetPriceSnapshotOrDefault(ctx, 1000) assert that the stored item for that denom
matches the second/new rate (verifying mergePriceSnapshotItems "latest-wins");
reference PriceSnapshotItems, NewPriceSnapshotItem, AddPriceSnapshot,
GetPriceSnapshotOrDefault and mergePriceSnapshotItems when adding the overwrite
assertion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 885f031e-148d-4068-ad1e-3865823b1ce3
📒 Files selected for processing (3)
ante/feeless.gox/oracle/keeper/keeper.gox/oracle/keeper/keeper_test.go
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
PR Checklist:
Make sure each step was done:
make lint-fix