fix(toolkit): [PM-20017] — use checked_add for wallet seed increment#1081
Open
fix(toolkit): [PM-20017] — use checked_add for wallet seed increment#1081
Conversation
Addresses Least Authority audit Issue AL — unchecked addition in wallet seed increment that could silently wrap on overflow.
Change Wallet::increment_seed return type from String to
Result<String, &'static str>. Replace .expect("wallet seed overflow")
with .ok_or("wallet seed overflow")? to return an explicit error on
overflow instead of panicking.
Propagate Result through the caller chain:
- compute_batches_seeds() returns Result<Vec<WalletSeed>, &'static str>
- Builder::relevant_wallet_seeds() returns Result<Vec<WalletSeed>, &'static str>
- TransactionGenerator::build_txs() maps to DynamicError
In BatchesBuilder::build_txs_from(), retain expect() on the Result
since overflow is physically impossible (64-bit seed space starting
from 2, batch sizes of ~hundreds) and the BuildTxs trait constrains
the error type to JoinError.
Add unit tests: normal increment, overflow error, width preservation,
zero increment.
Addresses Least Authority audit Issue AL.
m2ux
force-pushed
the
fix/PM-20017-checked-seed-increment
branch
from
eb5ed35 to
4788683
Compare
m2ux
changed the title
| #toolkit | ||
| # Replace unchecked addition in wallet seed increment with checked_add | ||
|
|
||
| Replace unchecked `+` operator in wallet seed increment with `checked_add` to |
Contributor
There was a problem hiding this comment.
I didn't see a change in this area. Maybe the change description needs a revision.
justinfrevert
approved these changes
Mar 31, 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
Replace panic-on-overflow with explicit error return in wallet seed increment, addressing Least Authority audit finding AL (PM-20017).
🎫 Ticket 📐 Engineering
Motivation
The wallet seed increment function uses
checked_add(1).expect()which panics on arithmetic overflow instead of returning an error. While overflow is physically impossible with the current 64-bit seed value space (u128 counter), the Least Authority audit correctly identified this as a code quality issue — panicking on arithmetic operations violates defensive programming principles.Changes
ledger/helpers) — Changedincrement_seedreturn type fromStringtoResult<String, &'static str>, replacing.expect()with.ok_or()?util/toolkit) — Updatedcompute_batches_seedsto propagateResult; retainedexpect()inbuild_txs_fromwhere overflow is physically impossible and trait constraints applyutil/toolkit) — Updatedrelevant_wallet_seedsto returnResultutil/toolkit) — AddedDynamicErrormapping at thebuild_txsboundary📌 Submission Checklist
🔱 Fork Strategy
🗹 TODO before merging