Skip to content

chore(cli): replace unwrap() with expect() in non-test code - #8420

Merged
dpc merged 1 commit into
fedimint:masterfrom
gabrielrondon:chore/replace-unwrap-with-expect-cli
Apr 7, 2026
Merged

chore(cli): replace unwrap() with expect() in non-test code#8420
dpc merged 1 commit into
fedimint:masterfrom
gabrielrondon:chore/replace-unwrap-with-expect-cli

Conversation

@gabrielrondon

Copy link
Copy Markdown
Contributor

Summary

Replace bare unwrap() calls with descriptive expect() messages in fedimint-cli, following the project convention documented in CLAUDE.md:

Never use unwrap() in non-test code — Always use expect() with a succinct message explaining why the condition cannot fail.

Changes

fedimint-cli/src/client.rs (8 replacements):

  • subscribe_reissue_external_notes().await.unwrap().expect("just created operation can't already be deleted")
  • serde_json::to_value(amount).unwrap().expect("Amount is serializable")
  • serde_json::to_value(LnInvoiceResponse{..}).unwrap().expect("LnInvoiceResponse is serializable")
  • serde_json::to_value(Vec::<String>::new()).unwrap().expect("empty vec is serializable")
  • 2x serde_json::to_value(()).unwrap().expect("unit type is serializable")
  • serde_json::to_value(InfoResponse{..}).unwrap().expect("InfoResponse is serializable")
  • balance.checked_sub(fees).unwrap() → refactored to idiomatic let-else pattern

fedimint-cli/src/lib.rs (3 replacements):

  • serde_json::to_string_pretty(self).unwrap().expect("CliOutput is serializable")
  • serde_json::to_value(()).unwrap().expect("unit type is serializable")
  • read_to_end(&mut plaintext_bytes).unwrap().expect("Could not read input cfg file")

Test plan

  • No functional changes — only panic messages improved
  • All expect() messages follow existing codebase patterns
  • The let-else refactor in Withdraw preserves identical behavior

@gabrielrondon
gabrielrondon requested a review from a team as a code owner March 25, 2026 16:32
dpc
dpc previously approved these changes Mar 25, 2026
@elsirion

Copy link
Copy Markdown
Contributor

Follow-up: forbid unwrap in CI

Replace bare unwrap() calls with descriptive expect() messages in
fedimint-cli, following the project convention documented in CLAUDE.md:
"Never use unwrap() in non-test code — Always use expect() with a
succinct message explaining why the condition cannot fail."

Also refactor checked_sub + is_none + unwrap pattern to idiomatic
let-else in the withdraw command.
@gabrielrondon
gabrielrondon force-pushed the chore/replace-unwrap-with-expect-cli branch from 2dd8170 to d6394a7 Compare March 30, 2026 18:32
gabrielrondon added a commit to gabrielrondon/fedimint that referenced this pull request Mar 30, 2026
Add `clippy::unwrap_used` to the deny list in `fedimint-cli` to prevent
reintroduction of `unwrap()` calls in non-test code.

Follow-up to fedimint#8420 which replaced all existing `unwrap()` with
`expect()` in non-test code. This lint ensures the cleanup is
maintained going forward.

Allow the lint in the existing test function where `unwrap()` is
acceptable.
@dpc
dpc added this pull request to the merge queue Apr 7, 2026
Merged via the queue into fedimint:master with commit 19afcea Apr 7, 2026
20 of 22 checks passed
gabrielrondon added a commit to gabrielrondon/fedimint that referenced this pull request Apr 8, 2026
Add `clippy::unwrap_used` to the deny list in `fedimint-cli` to prevent
reintroduction of `unwrap()` calls in non-test code.

Follow-up to fedimint#8420 which replaced all existing `unwrap()` with
`expect()` in non-test code. This lint ensures the cleanup is
maintained going forward.

Allow the lint in the existing test function where `unwrap()` is
acceptable.
github-merge-queue Bot pushed a commit that referenced this pull request Apr 8, 2026
Follow-up to #8420 (merged).

As suggested by @elsirion, this adds `clippy::unwrap_used` to the deny
list in `fedimint-cli/src/lib.rs` to prevent future regressions.

The only `#[allow(clippy::unwrap_used)]` is in the test module where
unwrap is acceptable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants