feat(wallet): strict address validation in forest-wallet CLI#6968
Open
0xDevNinja wants to merge 1 commit into
Open
feat(wallet): strict address validation in forest-wallet CLI#69680xDevNinja wants to merge 1 commit into
0xDevNinja wants to merge 1 commit into
Conversation
0xDevNinja
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughMigrate wallet CLI address arguments to typed StrictAddress, add early RPC-based network detection before CLI parsing to set CurrentNetwork, update wallet command handlers and backend default-address return type to use typed Address, and add tests verifying parse-time rejection of malformed addresses. ChangesWallet strict address migration
Sequence DiagramsequenceDiagram
participant Main as main.rs
participant RPC as rpc::Client
participant GlobalNet as CurrentNetwork
participant CLIParser as clap (Cli::parse_from)
participant Handler as Command Handler
rect rgba(100, 150, 200, 0.5)
Note over Main,GlobalNet: Early network detection before CLI parsing
Main->>RPC: initialize client without token
Main->>RPC: request StateNetworkName
RPC-->>Main: return NetworkChain
alt parsed and not Mainnet
Main->>GlobalNet: set CurrentNetwork to Testnet
else parse/lookup failed
Main-->>GlobalNet: keep default CurrentNetwork
end
end
rect rgba(150, 100, 200, 0.5)
Note over CLIParser,Handler: CLI parsing with StrictAddress validation
Main->>CLIParser: Cli::parse_from(args) with CurrentNetwork set
CLIParser->>CLIParser: validate StrictAddress fields
alt valid
CLIParser-->>Main: parsed command with typed addresses
Main->>Handler: execute command using StrictAddress/Address
else invalid
CLIParser-->>Main: return clap::Error (ValueValidation)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/wallet/main.rs (1)
24-28: Add a debug/warn log when the network probe fails and mainnet fallback is used.On Line 24, probe failures are silently ignored; a small log here would make prefix-validation fallbacks much easier to diagnose.
Suggested adjustment
- if let Ok(name) = StateNetworkName::call(&client, ()).await - && !matches!(NetworkChain::from_str(&name), Ok(NetworkChain::Mainnet)) - { - CurrentNetwork::set_global(Network::Testnet); - } + match StateNetworkName::call(&client, ()).await { + Ok(name) if !matches!(NetworkChain::from_str(&name), Ok(NetworkChain::Mainnet)) => { + CurrentNetwork::set_global(Network::Testnet); + } + Ok(_) => {} + Err(err) => { + tracing::debug!( + %err, + "network probe failed; keeping CurrentNetwork at mainnet default for CLI parsing" + ); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/main.rs` around lines 24 - 28, The probe using StateNetworkName::call(&client, ()).await currently swallows failures; update the branch around StateNetworkName::call, NetworkChain::from_str and the fallback that calls CurrentNetwork::set_global(Network::Testnet) to log a warning or debug message when the call fails or when parsing does not yield Mainnet. Specifically, capture the Err from StateNetworkName::call and log the error (including the error object and context like "network probe failed, falling back to Testnet"), and also log when NetworkChain::from_str returns non-Mainnet to explain why CurrentNetwork::set_global(Network::Testnet) is invoked.src/wallet/subcommands/mod.rs (1)
48-69: Add a clap regression test forsend --frommalformed addresses.
balanceandsignare covered, butsend --fromwas also migrated toStrictAddress. A matchingValueValidationtest would prevent regressions in that path.Suggested test addition
#[test] fn wallet_sign_rejects_malformed_address() { assert_eq!( parse_err_kind(&[ "forest-wallet", "sign", "-m", "deadbeef", "-a", "not-an-address", ]), clap::error::ErrorKind::ValueValidation, ); } + + #[test] + fn wallet_send_from_rejects_malformed_address() { + assert_eq!( + parse_err_kind(&[ + "forest-wallet", + "send", + "--from", + "not-an-address", + "f01234", + "1", + ]), + clap::error::ErrorKind::ValueValidation, + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/subcommands/mod.rs` around lines 48 - 69, Add a new unit test similar to wallet_balance_rejects_malformed_address and wallet_sign_rejects_malformed_address that asserts parse_err_kind returns clap::error::ErrorKind::ValueValidation for a malformed address passed to the send command's --from flag; use the existing helper parse_err_kind and name the test wallet_send_from_rejects_malformed_address, and supply the minimal extra send arguments (e.g. to and amount) so the clap parser reaches --from validation.
🤖 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/wallet/main.rs`:
- Around line 24-28: The probe using StateNetworkName::call(&client, ()).await
currently swallows failures; update the branch around StateNetworkName::call,
NetworkChain::from_str and the fallback that calls
CurrentNetwork::set_global(Network::Testnet) to log a warning or debug message
when the call fails or when parsing does not yield Mainnet. Specifically,
capture the Err from StateNetworkName::call and log the error (including the
error object and context like "network probe failed, falling back to Testnet"),
and also log when NetworkChain::from_str returns non-Mainnet to explain why
CurrentNetwork::set_global(Network::Testnet) is invoked.
In `@src/wallet/subcommands/mod.rs`:
- Around line 48-69: Add a new unit test similar to
wallet_balance_rejects_malformed_address and
wallet_sign_rejects_malformed_address that asserts parse_err_kind returns
clap::error::ErrorKind::ValueValidation for a malformed address passed to the
send command's --from flag; use the existing helper parse_err_kind and name the
test wallet_send_from_rejects_malformed_address, and supply the minimal extra
send arguments (e.g. to and amount) so the clap parser reaches --from
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3f00574d-3b2b-42b7-be1a-1f0963ceca9c
📒 Files selected for processing (4)
CHANGELOG.mdsrc/wallet/main.rssrc/wallet/subcommands/mod.rssrc/wallet/subcommands/wallet_cmd.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 115 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
akaladarshi
requested changes
Apr 28, 2026
Collaborator
|
@0xDevNinja Any update on this ? |
0xDevNinja
force-pushed
the
0xdevninja/issue-6012-wallet-strict-address
branch
from
90b968b to
7bd7a85
Compare
Author
|
Hey, sorry for the delay. Pushed an update addressing the review:
Left the two CodeRabbit nits as-is — the |
Collaborator
|
@0xDevNinja Linter is still failing you can verify the linter locally using |
0xDevNinja
force-pushed
the
0xdevninja/issue-6012-wallet-strict-address
branch
from
7bd7a85 to
12f8b2d
Compare
Author
|
Fixed — |
akaladarshi
requested changes
May 14, 2026
0xDevNinja
force-pushed
the
0xdevninja/issue-6012-wallet-strict-address
branch
from
12f8b2d to
c8005c8
Compare
Author
|
Both fold-ins pushed:
Local: |
akaladarshi
reviewed
May 14, 2026
| use rstest::rstest; | ||
|
|
||
| /// Guards against an accidental revert of #6012 (address args ↔ `StrictAddress`). | ||
| /// Malformed addresses must be rejected by clap at parse time, with a |
Collaborator
There was a problem hiding this comment.
Please remove the comments it's not helping anyone.
Mirrors the `forest-cli` treatment introduced in ChainSafe#6011 for the `forest-wallet` binary. Address arguments to `balance`, `export`, `has`, `delete`, `set-default`, `sign`, `verify` and the `--from` option of `send` are now parsed into `StrictAddress` at CLI-parse time. clap surfaces a `ValueValidation` error on a malformed input instead of deferring to a runtime `StrictAddress::from_str(...)?` fallback, and each handler simply destructures the typed value via `field: StrictAddress(inner)`. Network detection is moved ahead of `Cli::parse_from` so that clap-time `StrictAddress` validation accepts testnet (`t0...`) addresses when the daemon reports a testnet chain. Client construction errors propagate (matching `forest-cli`); an unreachable daemon leaves the global `CurrentNetwork` at its mainnet default. `wallet_default_address()` now returns `Option<Address>` rather than `Option<String>`, removing a stringify-then-reparse round trip through `StrictAddress::from_str` in both the `list` and `send` handlers. `Send.target_address` is intentionally kept as a `String` because it accepts ETH (`0x...`) recipients, which `StrictAddress` rejects; `resolve_target_address` continues to handle the dispatch. `ValidateAddress.address` also stays a `String`, since its purpose is to validate arbitrary user input. Two clap-level regression tests guard against an accidental revert of the `StrictAddress` typing. Refs ChainSafe#6012
0xDevNinja
force-pushed
the
0xdevninja/issue-6012-wallet-strict-address
branch
from
c8005c8 to
8df812a
Compare
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:
forest-walletsubcommands toStrictAddress(parsed at clap time). Coversbalance,export,has,delete,set-default,sign,verify, and the--fromoption ofsend. Handlers destructure the typed value viafield: StrictAddress(inner); runtimeStrictAddress::from_str(...)?fallbacks are removed.Cli::parse_frominsrc/wallet/main.rsso that clap-timeStrictAddressvalidation accepts testnet (t0...) addresses when the daemon reports a testnet chain. Client construction errors propagate (mirroringforest-cliafter feat: strict checks for address args inforest-cli#6011); an unreachable daemon leaves the globalCurrentNetworkat its mainnet default.wallet_default_address()return type toOption<Address>(wasOption<String>), removing a stringify→reparse round trip throughStrictAddress::from_strin both thelistandsendhandlers.balance, one forsign -a ...) assertingValueValidationon malformed addresses, guarding against an accidental revert.CHANGELOG.md:Addedentry listing the migrated subcommands and explicitly calling out thesend target_addressnon-migration.Intentional non-migrations
sendpositionaltarget_addressstaysString:resolve_target_addressaccepts both FIL (f0.../t0...) and ETH (0x...) forms, butStrictAddress::FromStrrejects the ETH form, so typing it would break the existing ETH-recipient path. Deviates from the issue body, which assumed all address fields could beStrictAddress.validate-addresspositionaladdressstaysString: the purpose of the subcommand is to validate arbitrary user input.Reference issue to close (if applicable)
Closes #6012
Other information and links
forest-cli#6011 (same change forforest-cli, including the pre-parse network detection).Recipientenum (Fil(StrictAddress),Eth(EthAddress)) forsend target_address, so the ETH form gets the same compile-time check.Change checklist
Outside contributions
AI Usage Disclosure
This PR was prepared with assistance from Claude Code (Anthropic). Extent:
forest-cli#6011), implementation, two self-review passes with an independent reviewer agent, test design, and PR drafting were AI-assisted.cargo fmt --all -- --check,cargo clippy --profile quick --all-targets --no-deps -- -D warnings(withFOREST_F3_SIDECAR_FFI_BUILD_OPT_OUT=1), andcargo test --lib -- wallet::subcommands(12/12 pass) are all green.Summary by CodeRabbit
New Features
Tests
Chore