Implement Filecoin.MpoolGetConfig and align replace_by_fee_ratio#7141
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR implements the ChangesMpoolGetConfig RPC Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CHANGELOG.md`:
- Line 32: Replace the PR reference with the issue reference for the changelog
entry mentioning Filecoin.MpoolGetConfig: change the linked text and URL from PR
`#7141` to issue `#7100` (i.e., update the markdown line that currently reads
"[`#7141`](.../pull/7141): Implemented `Filecoin.MpoolGetConfig` RPC method." to
reference issue 7100 and its issues URL instead).
In `@src/lotus_json/percent.rs`:
- Around line 19-25: The from_lotus_json implementation must reject non-finite,
negative, or out-of-range f64 values before performing the saturating cast;
update Percent::from_lotus_json (using the lotus_json, scaled and rounded
locals) to first assert lotus_json.is_finite() and lotus_json >= 0.0 (panic on
failure), then compute scaled = lotus_json * 100.0 and keep the existing
two-decimal check, and finally ensure rounded is within 0.0..=u64::MAX as f64
(panic if not) before returning Percent(rounded as u64).
🪄 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: 3769cda7-8be1-4761-8638-18f9371af631
⛔ Files ignored due to path filters (2)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
CHANGELOG.mdsrc/cli/subcommands/mpool_cmd.rssrc/lotus_json/mod.rssrc/lotus_json/percent.rssrc/message_pool/config.rssrc/message_pool/msgpool/mod.rssrc/message_pool/msgpool/msg_pool.rssrc/message_pool/msgpool/utils.rssrc/rpc/methods/mpool.rssrc/rpc/mod.rssrc/shim/mod.rssrc/shim/percent.rssrc/tool/subcommands/api_cmd/api_compare_tests.rssrc/tool/subcommands/api_cmd/test_snapshots.txt
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lotus_json/percent.rs (1)
21-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent saturated
f64 -> i64cast from accepting invalid large ratiosOn Line 28,
scaled as i64can saturate for oversized/non-finite inputs, then still passu64::try_from, yielding an unintended accepted value instead of a hard reject. Validate finiteness/non-negativity/range before integer casting.💡 Proposed fix
-use std::convert::TryFrom; @@ fn from_lotus_json(lotus_json: Self::LotusJson) -> Self { let scaled = format!("{lotus_json}e2") .parse::<f64>() .expect("unable to parse ratio"); + assert!( + scaled.is_finite() && scaled >= 0.0, + "ratio must be a finite non-negative number: {lotus_json}" + ); assert!( scaled.trunc() == scaled, "ratio may only have two decimals: {lotus_json}" ); - Percent(u64::try_from(scaled as i64).expect("ratio out of range")) + assert!(scaled <= u64::MAX as f64, "ratio out of range: {lotus_json}"); + Percent(scaled as u64) }In Rust stable, what are the exact semantics of casting `f64` to `i64` using `as` for out-of-range values, `NaN`, and infinities?🤖 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 `@src/lotus_json/percent.rs` around lines 21 - 29, The current conversion uses `scaled as i64` which can saturate for NaN/Infinity/out-of-range values; before casting validate that `scaled` is finite, non-negative, integral (use the existing scaled.trunc()==scaled check) and within the i64 bounds (scaled <= i64::MAX as f64), reject otherwise; only after those checks perform the integer cast and then `u64::try_from` to construct `Percent` (refer to the local variable `scaled` and the `Percent(...)` construction).
🤖 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.
Duplicate comments:
In `@src/lotus_json/percent.rs`:
- Around line 21-29: The current conversion uses `scaled as i64` which can
saturate for NaN/Infinity/out-of-range values; before casting validate that
`scaled` is finite, non-negative, integral (use the existing
scaled.trunc()==scaled check) and within the i64 bounds (scaled <= i64::MAX as
f64), reject otherwise; only after those checks perform the integer cast and
then `u64::try_from` to construct `Percent` (refer to the local variable
`scaled` and the `Percent(...)` construction).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 357a3b6c-0634-4ebb-8c85-0b1bde1c3296
📒 Files selected for processing (2)
src/lotus_json/percent.rssrc/shim/percent.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shim/percent.rs
sudo-shashank
requested review from
akaladarshi and
hanabi1224
and removed request for
a team
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
hanabi1224
approved these changes
Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
Filecoin.MpoolGetConfigRPC to return the current mpool configuration and add parity test.Percenttype (125 = 1.25×) matching types.Percent and use it forreplace_by_fee_ratioinMpoolConfigand RBF helpers.Reference issue to close (if applicable)
Closes #7100
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Filecoin.MpoolGetConfigRPC method to retrieve current mempool configuration settings.Tests