feat: add forest-cli mpool nonce + add RPC snapshot test#5999
feat: add forest-cli mpool nonce + add RPC snapshot test#5999LesnyRumcajs merged 4 commits intomainfrom
forest-cli mpool nonce + add RPC snapshot test#5999Conversation
WalkthroughAdds a new forest-cli subcommand Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant CLI as forest-cli
participant Parser as Address Parser
participant RPC as RPC Client
participant Node as Filecoin Node
User->>CLI: forest-cli mpool nonce <address>
CLI->>Parser: parse <address>
Parser-->>CLI: Address
CLI->>RPC: MpoolGetNonce(Address)
RPC->>Node: JSON-RPC request
Node-->>RPC: nonce / error
RPC-->>CLI: nonce / error
CLI-->>User: print nonce or error
Note right of CLI: returns Ok(()) on success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
LesnyRumcajs
force-pushed
the
add-forest-cli-mpool-nonce
branch
from
e2ed3be to
a63d144
Compare
LesnyRumcajs
requested review from
akaladarshi and
elmattic
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/subcommands/mpool_cmd.rs (1)
138-144: Avoid panic in mpool stat when actor sequence is unavailable
compute_statsusesexpect("get must succeed"). However,actor_sequencesis built fromget_actor_sequence, which may returnNoneon lookup errors, making theexpectreachable and crashingmpool stat. Prefer a graceful skip.Apply:
@@ - for (address, bucket) in buckets { - let actor_sequence = *actor_sequences.get(&address).expect("get must succeed"); + for (address, bucket) in buckets { + let Some(&actor_sequence) = actor_sequences.get(&address) else { + // Skip addresses where actor state could not be fetched + continue; + };This keeps the command robust in the presence of transient RPC/state issues.
Also applies to: 270-275
🧹 Nitpick comments (4)
docs/docs/users/reference/cli.sh (1)
65-65: Docs: add mpool nonce section — LGTMThe new docs section hook for
forest-cli mpool nonceis correctly added and follows the existing pattern.If you want the subcommands to be easier to scan, consider grouping them alphabetically within the mpool block (
mpool nonce,mpool pending,mpool stat). No action needed for this PR.src/cli/subcommands/mpool_cmd.rs (3)
35-39: Type the CLI argument as Address and let clap parse itMinor ergonomics improvement: accept
Addressdirectly in the enum and let clap perform parsing viaFromStr. This removes a runtime parse inrunand yields better error messages from clap.Apply:
@@ - /// Get the current nonce for an address + /// Get the current nonce for an address Nonce { - /// Address to check nonce for - address: String, + /// Address to check nonce for + #[arg(value_name = "ADDRESS")] + address: Address, },
283-289: Nonce subcommand implementation — solid and minimalParsing the address and delegating to
MpoolGetNonceis exactly the thin wrapper we want. Output is unambiguous (just the nonce).If you adopt the typed-argument refactor above, you can drop the parse:
- Self::Nonce { address } => { - let address = Address::from_str(&address)?; + Self::Nonce { address } => { let nonce = MpoolGetNonce::call(&client, (address,)).await?; println!("{nonce}"); Ok(()) }Optionally, consider a
--jsonflag for machine-readable output (in line with other commands that support JSON), but that's not required here.
96-104: Send lookup warnings to stderrThese diagnostics currently go to stdout and can mix with the primary command output. Use stderr to keep output streams clean.
Apply:
- println!("{address}, actor state not found"); + eprintln!("mpool stat: actor state for {address} not found"); return None; @@ - println!("{address}, err: {err}"); + eprintln!("mpool stat: error getting actor state for {address}: {err}"); return None;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)docs/docs/users/reference/cli.sh(1 hunks)src/cli/subcommands/mpool_cmd.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
🧬 Code graph analysis (1)
src/cli/subcommands/mpool_cmd.rs (3)
src/shim/address.rs (2)
from_str(178-183)from_str(299-302)src/rpc/reflect/mod.rs (1)
call(316-325)src/rpc/client.rs (2)
call(92-151)client(114-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
CHANGELOG.md (1)
52-53: Changelog entry reads well and matches house styleThe wording, link, and placement under “Added” look consistent with the rest of the file.
src/cli/subcommands/mpool_cmd.rs (1)
281-282: Explicit Ok(()) — good return-path hygieneAdding
Ok(())aligns arm semantics and avoids accidental fall-through or future regressions.
LesnyRumcajs
changed the title
forest-cli mpool nonceforest-cli mpool nonce + add RPC snapshot test
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)src/tool/subcommands/api_cmd/test_snapshots_ignored.txt(0 hunks)
💤 Files with no reviewable changes (1)
- src/tool/subcommands/api_cmd/test_snapshots_ignored.txt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Check
- GitHub Check: tests
- GitHub Check: Analyze (ruby)
- GitHub Check: Deploy to Cloudflare Pages
LesnyRumcajs
force-pushed
the
add-forest-cli-mpool-nonce
branch
from
7687394 to
e4548a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
84-91: Clarify invariant: “zero balance” ≠ “no actor”; document intended error behaviorThis constant is used to provoke “actor not found” (uninitialized address) in MpoolGetNonce, but the doc only promises zero balance. An address can have zero balance and still have an account actor, which would make MpoolGetNonce return a nonce (often 0) instead of an error. Tighten the comment to state “no actor/uninitialized” and spell out the maintenance note.
Apply this doc-only diff:
-/// This address is known to be empty on calibnet. It should always have a zero balance. +/// This address is known to be uninitialized (no actor) on calibnet at the time of writing. +/// Practical effects: +/// - Methods like `MpoolGetNonce` should return an error (`actor not found`). +/// - `WalletBalance` should be zero. +/// If calibnet is reset or this address is ever funded/initialized, update this constant. static KNOWN_EMPTY_CALIBNET_ADDRESS: LazyLock<Address> = LazyLock::new(|| { crate::shim::address::Network::Testnet .parse_address("t1qb2x5qctp34rxd7ucl327h5ru6aazj2heno7x5y") .unwrap() .into() });
1168-1169: Reduce future flakiness risk for the “known-empty” wallet entryIncluding a publicly known empty address is convenient, but if it’s ever funded (or calibnet resets), the WalletBalance test may change behavior. Two options (non-blocking):
- Generate a fresh, random t1 address per run for unhappy-path tests (almost guaranteed to be uninitialized).
- Keep the constant, but add a short maintenance note near this array mirroring the constant’s doc (“update if funded/reset”).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)docs/docs/users/reference/cli.sh(1 hunks)src/cli/subcommands/mpool_cmd.rs(2 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(3 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)src/tool/subcommands/api_cmd/test_snapshots_ignored.txt(0 hunks)
💤 Files with no reviewable changes (1)
- src/tool/subcommands/api_cmd/test_snapshots_ignored.txt
✅ Files skipped from review due to trivial changes (1)
- docs/docs/users/reference/cli.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cli/subcommands/mpool_cmd.rs
- CHANGELOG.md
- src/tool/subcommands/api_cmd/test_snapshots.txt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: All lint checks
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
| /// Get the current nonce for an address | ||
| Nonce { | ||
| /// Address to check nonce for | ||
| address: String, |
There was a problem hiding this comment.
I think address: Address works here since Address implements FromStr
There was a problem hiding this comment.
Yeah, but no. There are those weird network shenanigans (global static variable that governs whether it's t or f) so we can't do it trivially here. See other arguments in this module.
There was a problem hiding this comment.
Oh yeah, actually it works. We have this rather backwards logic here that make it work.
Lines 178 to 182 in e4548a8
There was a problem hiding this comment.
@coderabbitai create an issue to tackle other occurrences of the String that could be Address type in forest-cli params
There was a problem hiding this comment.
🧩 Analysis chain
I'll help you create an issue to track the opportunity to use Address type instead of String for address parameters in forest-cli. Let me first search the codebase to identify specific occurrences.
@LesnyRumcajs I've created an issue to track the opportunity to replace String with Address type for address parameters in forest-cli commands. The issue includes:
- Context from the current PR discussion
- Examples found in the codebase (like the
to/fromparameters in mpool commands) - Benefits of making this change
- Scope and background information
- Links back to this PR and comment
The issue has been assigned to you as requested. This will help ensure consistency across the CLI interface and provide better type safety for address parameters throughout the forest-cli codebase.
🐰✨ ears perk up with organizational satisfaction
🏁 Script executed:
#!/bin/bash
# Find CLI command definitions with String parameters that likely represent addresses
echo "=== Searching for CLI commands with String address parameters ==="
rg -nP --type=rust -C3 'address.*:\s*(Option<)?String' src/cli/subcommands/
echo
echo "=== Looking for from/to String parameters in CLI ==="
rg -nP --type=rust -C3 '(from|to).*:\s*Option<String>' src/cli/subcommands/
echo
echo "=== Finding other potential address String parameters ==="
rg -nP --type=rust -C2 '#\[arg.*\]\s*\w+.*:\s*(Option<)?String' src/cli/subcommands/ | grep -i -A2 -B2 'address\|from\|to\|sender\|recipient'
Length of output: 3982
🏁 Script executed:
#!/bin/bash
# Let's also check the specific files mentioned to get a comprehensive view
echo "=== Examining mpool_cmd.rs for String parameters ==="
rg -nP --type=rust -C3 ':\s*(Option<)?String' src/cli/subcommands/mpool_cmd.rs
echo
echo "=== Looking at other CLI subcommand files ==="
fd -e rs . src/cli/subcommands/ --exec basename {} | sort | uniq
Length of output: 1065
🏁 Script executed:
#!/bin/bash
# Let's examine a few key CLI subcommand files to find String parameters that could be Address
echo "=== Checking state_cmd.rs ==="
rg -nP --type=rust -C2 ':\s*(Option<)?String' src/cli/subcommands/state_cmd.rs | head -20
echo
echo "=== Checking wallet_cmd.rs ==="
rg -nP --type=rust -C2 ':\s*(Option<)?String' src/cli/subcommands/wallet_cmd.rs | head -20
echo
echo "=== Checking send_cmd.rs ==="
rg -nP --type=rust -C2 ':\s*(Option<)?String' src/cli/subcommands/send_cmd.rs | head -20
Length of output: 875
🏁 Script executed:
#!/bin/bash
# Create the GitHub issue
gh issue create \
--title "Replace String with Address type for address parameters in forest-cli commands" \
--body "## Summary
During the review of PR #5999 (https://github.com/ChainSafe/forest/pull/5999#discussion_r2304022509), it was discovered that many forest-cli commands use \`String\` parameters for addresses when they could use the \`Address\` type directly.
As noted in the discussion, \`Address\` implements \`FromStr\` and works correctly with clap due to backwards logic in the Address::from_str implementation: https://github.com/ChainSafe/forest/blob/e4548a87ebb897f5e74282939c063727c6f4d460/src/shim/address.rs#L178-L182
## Examples Found
Based on a search of the codebase, the following CLI commands currently use String parameters that could potentially be Address types:
### mpool_cmd.rs
- \`to: Option<String>\` and \`from: Option<String>\` in the Pending command
- The newly added \`address: String\` in the Nonce command (from PR #5999)
### Other CLI commands
Multiple other CLI subcommands likely have similar opportunities for improvement.
## Benefits
- **Type Safety**: Using Address type provides compile-time validation
- **Consistency**: Aligns with the type system and reduces string parsing errors
- **Better API**: More semantic parameter types make the CLI interface clearer
## Scope
This issue should cover:
1. Audit all forest-cli subcommands for String parameters that represent addresses
2. Replace appropriate String parameters with Address type
3. Update any related parsing/conversion logic
4. Ensure tests are updated accordingly
## Background
This was identified during the implementation of \`forest-cli mpool nonce\` command where the discussion revealed that Address type works correctly with clap argument parsing.
**Related PR**: https://github.com/ChainSafe/forest/pull/5999
**Comment**: https://github.com/ChainSafe/forest/pull/5999#discussion_r2304022509
**Requested by**: @LesnyRumcajs" \
--assignee LesnyRumcajs
Length of output: 2005
@LesnyRumcajs Perfect! I've successfully created issue #6006 to track replacing String with Address type for address parameters in forest-cli commands.
Here's what I found during the codebase search:
Confirmed examples that could use Address type:
-
mpool_cmd.rs:
to: Option<String>andfrom: Option<String>in the Pending commandaddress: Stringin the new Nonce command (from this PR)
-
state_cmd.rs:
actor_address: Stringin the ReadState command
-
send_cmd.rs:
from: Option<String>andtarget_address: String
-
net_cmd.rs:
address: Stringin the Connect command (though this might be a multiaddress rather than a Filecoin address)
The issue has been assigned to you and includes:
- Complete context from our discussion
- Specific examples found in the codebase
- Benefits of the change (type safety, consistency, better API)
- Clear scope for implementation
- Links back to this PR and comment thread
This should provide a solid foundation for systematically improving the type safety across forest-cli address parameters!
🐰📝 proudly organizes the technical debt into a neat tracking issue
🧠 Learnings used
Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
3 participants
Summary of changes
Changes introduced in this pull request:
forest-cli mpool nonceto get the current nonce of an addressReference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Documentation
Tests