fix: include strict bound in blocks included for calculating gas premium#6849
fix: include strict bound in blocks included for calculating gas premium#6849akaladarshi merged 4 commits intomainfrom
Conversation
WalkthroughClamp Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/rpc/methods/gas.rs (1)
114-118: Add boundary regression tests for the new clamp behavior.Please add tests for
nblocksinclnormalization (0,1,128,129) to lock in this compatibility fix and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/gas.rs` around lines 114 - 118, Add unit tests that exercise the nblocksincl normalization logic around the MAX_GAS_HISTORY clamp in src/rpc/methods/gas.rs: specifically assert that input nblocksincl values 0 and 1 normalize to 1, and that values 128 and 129 normalize to MAX_GAS_HISTORY (e.g., 128), to lock in the compatibility fix; add these as table-driven cases in the same test module (or a new test) with clear names like test_nblocksincl_normalizes_zero_to_one and test_nblocksincl_clamps_above_max, call the function or logic that performs the normalization (the code that reads/sets nblocksincl) and assert the normalized result equals the expected value for each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 48: The changelog entry references the PR number instead of the issue;
update the line that mentions "GasEstimateGasPremium" to use issue `#6825` rather
than PR `#6849` by replacing the link and bracketed number ([`#6849`](...)) with the
issue link and number ([`#6825`](...)) so the entry reads "[`#6825`]: Included
strict bound in blocks included for calculating gas premium
`GasEstimateGasPremium`." Ensure the URL points to the issue, not the PR.
---
Nitpick comments:
In `@src/rpc/methods/gas.rs`:
- Around line 114-118: Add unit tests that exercise the nblocksincl
normalization logic around the MAX_GAS_HISTORY clamp in src/rpc/methods/gas.rs:
specifically assert that input nblocksincl values 0 and 1 normalize to 1, and
that values 128 and 129 normalize to MAX_GAS_HISTORY (e.g., 128), to lock in the
compatibility fix; add these as table-driven cases in the same test module (or a
new test) with clear names like test_nblocksincl_normalizes_zero_to_one and
test_nblocksincl_clamps_above_max, call the function or logic that performs the
normalization (the code that reads/sets nblocksincl) and assert the normalized
result equals the expected value for each case.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e3fd9c2b-f58e-4e53-ac96-0b1b5401fa48
📒 Files selected for processing (2)
CHANGELOG.mdsrc/rpc/methods/gas.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rpc/methods/gas.rs (1)
114-114: Consider adding unit tests for the clamping behavior.The clamping logic is straightforward, but adding tests for edge cases would improve confidence:
nblocksincl = 0→ should clamp to 1nblocksincl > 128→ should clamp to 128This could be a simple unit test for
estimate_gas_premiumor a dedicated test for the clamping behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/gas.rs` at line 114, Add unit tests that assert the clamping behavior of nblocksincl in estimate_gas_premium: specifically test that when nblocksincl is 0 it clamps to 1 and when nblocksincl exceeds MAX_GAS_HISTORY (e.g., 129) it clamps to MAX_GAS_HISTORY (128). Create tests that call estimate_gas_premium (or a small helper that performs the clamp) with values 0, 1, MAX_GAS_HISTORY, and MAX_GAS_HISTORY+1 and verify the effective nblocksincl used or the resulting output matches the expected clamped behavior; reference the nblocksincl variable and MAX_GAS_HISTORY constant in assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/rpc/methods/gas.rs`:
- Line 114: Add unit tests that assert the clamping behavior of nblocksincl in
estimate_gas_premium: specifically test that when nblocksincl is 0 it clamps to
1 and when nblocksincl exceeds MAX_GAS_HISTORY (e.g., 129) it clamps to
MAX_GAS_HISTORY (128). Create tests that call estimate_gas_premium (or a small
helper that performs the clamp) with values 0, 1, MAX_GAS_HISTORY, and
MAX_GAS_HISTORY+1 and verify the effective nblocksincl used or the resulting
output matches the expected clamped behavior; reference the nblocksincl variable
and MAX_GAS_HISTORY constant in assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ffa1ac94-c697-4a7c-9457-ad9a9b9b2f37
📒 Files selected for processing (1)
src/rpc/methods/gas.rs
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6825
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit