fix: audit and replace unwrap() calls with error handling - #542
fix: audit and replace unwrap() calls with error handling#542summer-0ma wants to merge 4 commits into
Conversation
|
@summer-0ma is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
collinsezedike
left a comment
There was a problem hiding this comment.
CI is failing on three checks:
- Commit Messages: header is 73 characters, over the 72-char limit.
- PR Title: "Audit/issue 534 unwrap calls" has no conventional-commit type prefix (fix/chore/etc.), see CONTRIBUTING.md's Commit Convention section, since squash merge is enforced, this becomes the final commit message.
- Soroban Contract Tests: fails to build, see the inline comment below, this isn't a flaky failure.
Vercel is also failing, but that's the pre-existing #513 outage, unrelated to this PR.
| .get(&VAULT_KEY) | ||
| .unwrap_or_else(|| { | ||
| panic_with_error!(&env, ContractError::NotInitialized); | ||
| unreachable!() |
There was a problem hiding this comment.
panic_with_error!(&env, ...) returns the never type, so rustc proves the following unreachable!() is dead code and errors on unreachable_code under cargo clippy --all-targets -- -D warnings. This pattern is repeated roughly 20 times across all three files (blend-adapter, defindex-adapter, vault) and fails to compile as written, confirmed by running that exact clippy command against this branch. panic_with_error! alone already panics and returns !, the trailing unreachable!() isn't needed at all, dropping it from every occurrence should fix this.
| .storage() | ||
| .instance() | ||
| .get(&POOL_KEY) | ||
| .ok_or(ContractError::NotInitialized)?; |
There was a problem hiding this comment.
Converting these from .unwrap() to .ok_or(NotInitialized)? changes refresh()'s behavior, not just its error type. refresh() (below) discards accrue()'s result via #[allow(unused_must_use)], so calling it on an uninitialized adapter used to panic (trap the transaction) and now silently does nothing. Worth having refresh() propagate or explicitly handle the error instead of swallowing it, so this doesn't become a quiet no-op.
| .storage() | ||
| .instance() | ||
| .get(&VAULT_KEY) | ||
| .unwrap_or_else(|| { |
There was a problem hiding this comment.
This 6-line unwrap_or_else(|| { panic_with_error!(...); unreachable!() }) block is copy-pasted around 20 times across all three files. Since fixing the unreachable_code build error above means touching every one of those sites anyway, worth collapsing this into a single helper now, e.g. a small extension trait method like .get_or_not_initialized(&env), so future changes to this pattern are a one-location fix.
dd8adf3 to
93ede63
Compare
|
@collinsezedike correction done |
collinsezedike
left a comment
There was a problem hiding this comment.
cargo fmt --all -- --check is failing, run pnpm --filter contracts fmt (or cargo fmt --all directly in packages/contracts) before pushing. This is also currently masking whether the unreachable_code issue from the last review is actually fixed, the fmt failure stops the job before clippy/test run, so that can't be confirmed yet.
| let vault: Address = env.storage().instance().get(&VAULT_KEY).unwrap(); | ||
| let vault: Address = env | ||
| .storage() | ||
| .instance() |
There was a problem hiding this comment.
No test exercises deposit/withdraw/get_pool/accrue on a freshly-registered, uninitialized contract, so the new NotInitialized path this PR adds is never actually verified to fire.
| /// Supplies the USDC to the Blend lending pool as collateral and returns | ||
| /// the real bTokens credited, measured from Blend's own ledger rather | ||
| /// than assumed 1:1, so the vault's adapter-share accounting (`ADPT_SH`) | ||
| /// tracks genuine, appreciating shares instead of raw principal (#486). |
There was a problem hiding this comment.
This unwrap_or_else(|| panic_with_error!(...)) block is still duplicated ~20 times across all three files, worth collapsing into one helper now rather than after another round of edits touches all 20 sites again.
|
@summer-0ma |
|
@summer-0ma checking in, the last two commits are just merges from |
Summary
Comprehensive audit and remediation of all 33
.unwrap()calls across three Soroban contract crates. Each call has been systematically evaluated andreplaced with typed
ContractErrorreturns or documented with justifying comments explaining why the pattern is genuinely infallible.Changes by File
defindex-adapter/src/lib.rs (9 unwrap calls)
NotInitializederror variant toContractErrorenum.unwrap()calls withpanic_with_error()for proper error handlingVec.get().unwrap_or()patternsdeposit(),withdraw(),total_assets(),get_pool()MockDefindexVaultdeposit/withdraw methodsblend-adapter/src/lib.rs (15 unwrap calls)
NotInitializederror variant toContractErrorenum.ok_or(ContractError::NotInitialized)?for Result-returningaccrue()functionpanic_with_error!()withunreachable!()for non-Result functionsdeposit(),withdraw(),accrue(),get_pool()MockBlendPool::submit(),get_reserve(),get_positions()unwrap_or()patterns on Map/Vec accessvault/src/lib.rs (9 unwrap calls in test mocks)
panic_with_errorimport for consistent error handlingMockAdapter,LossyMockAdapter,ZeroShareMockAdapter,CachedMockAdapter.unwrap()calls in:deposit(),withdraw(),total_assets(),refresh()Implementation Details
Storage Initialization Failures: Panics with typed
NotInitializederror, providing context about invalid contract stateCollection Access with Defaults: Justified with comments explaining why
unwrap_or()is safe (e.g., Map/Vec returning Option)Consistency: All adapters follow identical error handling patterns
Backward Compatibility: No public function signatures changed, no external ABI impacts
Acceptance Criteria Met ✅
.unwrap()in the three crates has been addressedContractErroror documented justificationCloses #534