Skip to content

fix: replace unwrap() with ? in AccountWitness::new() TryFrom impl#1957

Merged
igamigo merged 2 commits into0xMiden:nextfrom
bigeez:fix/account-witness-unwrap
Apr 15, 2026
Merged

fix: replace unwrap() with ? in AccountWitness::new() TryFrom impl#1957
igamigo merged 2 commits into0xMiden:nextfrom
bigeez:fix/account-witness-unwrap

Conversation

@bigeez
Copy link
Copy Markdown
Contributor

@bigeez bigeez commented Apr 5, 2026

Summary

Replaces 4 unwrap()/expect() calls in production code with proper error propagation using ? and map_err().

Changes

1. crates/rust-client/src/rpc/domain/account.rs

AccountWitness::new().unwrap() inside a TryFrom impl where everything else uses ?. Applied the same .map_err(|err| RpcError::InvalidResponse(format!("{err}")))? pattern already used in the same file.

2. crates/sqlite-store/src/chain_data.rs

NonZeroUsize::new().unwrap() and usize::try_from().expect() when parsing DB data. A zero or overflow value would panic inside a Result-returning function. Both now return StoreError::ParsingError.

3. crates/sqlite-store/src/account/accounts.rs

nonces.last().unwrap() with no empty-slice guard in undo_account_nonces(). Now returns StoreError::ParsingError if nonces is empty.

4. crates/rust-client/src/transaction/request/mod.rs

.expect() on try_into() inside build_input_notes() which returns Result. Added NoteRecordError variant to TransactionRequestError and propagated with ?.

Result

  • 4 files changed
  • All panics in Result-returning functions replaced with proper error propagation
  • No behavior changes in the happy path

@bigeez
Copy link
Copy Markdown
Contributor Author

bigeez commented Apr 9, 2026

can you retry the merge?

@igamigo
Copy link
Copy Markdown
Collaborator

igamigo commented Apr 9, 2026

Hello @bigeez, thanks for the PR. Can you rebase this to the latest next and remove the unrelated commits from this PR's history? It's not really reviewable as-is.

@bigeez bigeez force-pushed the fix/account-witness-unwrap branch from b3e9fd9 to e3afdb3 Compare April 9, 2026 17:44
@bigeez
Copy link
Copy Markdown
Contributor Author

bigeez commented Apr 9, 2026

Hi @igamigo, done!
Rebased onto the latest next and the branch now contains only the single relevant commit.

Thanks for the guidance!

@igamigo igamigo changed the base branch from main to next April 10, 2026 02:12
@igamigo igamigo added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Apr 10, 2026
@igamigo
Copy link
Copy Markdown
Collaborator

igamigo commented Apr 10, 2026

I think this might be missing some changes. The PR description mentions four different changes but the diff shows one.

@bigeez
Copy link
Copy Markdown
Contributor Author

bigeez commented Apr 10, 2026

the 3 other fixes were dropped during the rebase cleanup, I've added them back as seperate commits

Copy link
Copy Markdown
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just keep the AccountWitness change. The other ones express invariants that shouldn't be broken and not errors that make sense for the user to react to in any way

@@ -580,7 +580,11 @@ impl SqliteStore {
}

// Step 4: Restore old header from the earliest discarded nonce
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a safety comment to this expressing while the unwrap() was safe

`undo_account_nonces` is only called for accounts present in
`nonces_by_account`, which only gets entries when at least one nonce is
pushed — so `nonces` is guaranteed non-empty at the unwrap site.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bigeez bigeez force-pushed the fix/account-witness-unwrap branch from bf7ca51 to e99063c Compare April 10, 2026 18:56
@bigeez
Copy link
Copy Markdown
Contributor Author

bigeez commented Apr 10, 2026

I've dropped the chain_data and request/mod changes, and replaced the accounts.rs unwrap change with a safety comment explaining the invariant instead. The PR now has just two commits:

  1. The AccountWitness::new() fix in account.rs
  2. A safety comment on nonces.last().unwrap() in accounts.rs

@bigeez bigeez requested a review from igamigo April 15, 2026 05:04
@igamigo igamigo merged commit 7299dd3 into 0xMiden:next Apr 15, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This PR does not require an entry in the `CHANGELOG.md` file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants