diff --git a/packages/contracts/adapter-common/src/lib.rs b/packages/contracts/adapter-common/src/lib.rs index e3678bae..d3edd83f 100644 --- a/packages/contracts/adapter-common/src/lib.rs +++ b/packages/contracts/adapter-common/src/lib.rs @@ -6,7 +6,7 @@ //! and error types used across all adapters (blend-adapter, defindex-adapter, //! etc.). Protocol-specific yield logic remains in each adapter's own crate. -use soroban_sdk::{contracterror, symbol_short, Address, Env, Symbol}; +use soroban_sdk::{contracterror, panic_with_error, symbol_short, Address, Env, Error, Symbol}; // --------------------------------------------------------------------------- // Storage keys @@ -75,3 +75,28 @@ pub fn get_vault(env: &Env) -> Option
{ pub fn get_usdc(env: &Env) -> Address { env.storage().instance().get(&USDC_KEY).unwrap() } + +// --------------------------------------------------------------------------- +// Shared NotInitialized helper +// --------------------------------------------------------------------------- + +/// Lets each adapter's own `ContractError` supply its `NotInitialized` +/// variant to the shared `get_or_not_initialized` helper below, since each +/// adapter defines that enum itself (with a different discriminant) rather +/// than sharing one across crates. +pub trait NotInitializedError { + fn not_initialized() -> Self; +} + +/// Reads an instance-storage value, panicking with the caller's typed +/// NotInitialized error instead of an opaque unwrap trap if it's unset. In +/// practice this branch is unreachable on any contract deployed via +/// `__constructor`, since every storage key this is used for is set before +/// any other method is reachable; this exists as a defensive, correctly +/// typed fallback rather than a path expected to actually fire. +pub fn get_or_not_initialized(env: &Env, value: Option) -> T +where + E: NotInitializedError + Into, +{ + value.unwrap_or_else(|| panic_with_error!(env, E::not_initialized())) +} diff --git a/packages/contracts/blend-adapter/src/lib.rs b/packages/contracts/blend-adapter/src/lib.rs index 483bbd45..158aae66 100644 --- a/packages/contracts/blend-adapter/src/lib.rs +++ b/packages/contracts/blend-adapter/src/lib.rs @@ -133,6 +133,8 @@ pub enum ContractError { AlreadyInitialized = 1, /// An intermediate arithmetic operation would overflow `i128`. Overflow = 2, + /// A state-mutating call was made before `initialize`. + NotInitialized = 3, } impl From for ContractError { @@ -143,6 +145,12 @@ impl From for ContractError { } } +impl adapter_common::NotInitializedError for ContractError { + fn not_initialized() -> Self { + ContractError::NotInitialized + } +} + // --------------------------------------------------------------------------- // Contract // --------------------------------------------------------------------------- @@ -207,7 +215,10 @@ impl MeridianBlendAdapter { pub fn deposit(env: Env, amount: i128) -> i128 { require_vault_auth(&env); - let pool: Address = env.storage().instance().get(&POOL_KEY).unwrap(); + let pool: Address = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&POOL_KEY), + ); let usdc = get_usdc(&env); let adapter = env.current_contract_address(); @@ -231,6 +242,7 @@ impl MeridianBlendAdapter { let client = BlendPoolClient::new(&env, &pool); let index = client.get_reserve(&usdc).config.index; + // Map.get() safely returns Option, defaulting to 0 if the index doesn't exist. let b_tokens_before = client .get_positions(&adapter) .collateral @@ -251,6 +263,7 @@ impl MeridianBlendAdapter { ], ); + // Map.get() safely returns Option, defaulting to 0 if the index doesn't exist. let b_tokens_after = client .get_positions(&adapter) .collateral @@ -258,6 +271,9 @@ impl MeridianBlendAdapter { .unwrap_or(0); let b_tokens_credited = b_tokens_after - b_tokens_before; + // Instance storage read defaults to 0 if TOTAL_KEY hasn't been set, which is safe since + // initialize() sets this key to 0. This unwrap_or pattern is the idiomatic way to handle + // optional storage values in Soroban. let prev: i128 = env.storage().instance().get(&TOTAL_KEY).unwrap_or(0); env.storage().instance().set(&TOTAL_KEY, &(prev + amount)); @@ -272,7 +288,10 @@ impl MeridianBlendAdapter { pub fn withdraw(env: Env, shares: i128, recipient: Address) -> i128 { require_vault_auth(&env); - let pool: Address = env.storage().instance().get(&POOL_KEY).unwrap(); + let pool: Address = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&POOL_KEY), + ); let usdc = get_usdc(&env); let adapter = env.current_contract_address(); @@ -304,6 +323,9 @@ impl MeridianBlendAdapter { let after = usdc_client.balance(&recipient); let delivered = after - before; + // Instance storage read defaults to 0 if TOTAL_KEY hasn't been set, which is safe since + // initialize() sets this key to 0. This unwrap_or pattern is the idiomatic way to handle + // optional storage values in Soroban. let prev: i128 = env.storage().instance().get(&TOTAL_KEY).unwrap_or(0); let remaining = if prev > delivered { prev - delivered @@ -326,13 +348,18 @@ impl MeridianBlendAdapter { /// (`get_positions`) rather than self-tracking it, so there is no risk of /// drift between the stored total and Blend's actual accounting. pub fn accrue(env: Env) -> Result<(), ContractError> { - let pool: Address = env.storage().instance().get(&POOL_KEY).unwrap(); + let pool: Address = env + .storage() + .instance() + .get(&POOL_KEY) + .ok_or(ContractError::NotInitialized)?; let usdc = get_usdc(&env); let adapter = env.current_contract_address(); let client = BlendPoolClient::new(&env, &pool); let reserve = client.get_reserve(&usdc); let positions = client.get_positions(&adapter); + // Map.get() safely returns Option, defaulting to 0 if the index doesn't exist. let b_tokens = positions.collateral.get(reserve.config.index).unwrap_or(0); let current_value = b_tokens_to_usdc(b_tokens, reserve.data.b_rate)?; @@ -345,21 +372,37 @@ impl MeridianBlendAdapter { /// last call, satisfying the shared YieldAdapterInterface contract. /// Currently just calls accrue(), which remains a public, /// permissionless entry point in its own right. - #[allow(unused_must_use)] + /// + /// Panics on failure rather than returning a Result: the shared adapter + /// interface's refresh() has no error return, so propagating accrue()'s + /// Result would mean changing that interface's ABI across both adapters + /// and the vault's calls into them. Panicking here instead of silently + /// discarding the error preserves this function's pre-existing + /// fail-loud behaviour (accrue()'s storage read used to be a bare + /// unwrap(), which panicked directly) rather than downgrading a real + /// failure into a silent no-op success. pub fn refresh(env: Env) { - Self::accrue(env); + if let Err(err) = Self::accrue(env.clone()) { + panic_with_error!(&env, err); + } } /// Returns the cached USDC value of the adapter's Blend position. Reflects /// yield only as of the last `accrue()` call; call `accrue()` first for a /// value that includes interest accrued since then. pub fn total_assets(env: Env) -> i128 { + // Instance storage read defaults to 0 if TOTAL_KEY hasn't been set, which is safe since + // initialize() sets this key to 0. This unwrap_or pattern is the idiomatic way to handle + // optional storage values in Soroban. env.storage().instance().get(&TOTAL_KEY).unwrap_or(0) } /// Returns the Blend pool this adapter supplies to. pub fn get_pool(env: Env) -> Address { - env.storage().instance().get(&POOL_KEY).unwrap() + adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&POOL_KEY), + ) } /// Returns "blend", identifying which protocol this adapter wraps. @@ -427,8 +470,15 @@ mod tests { to: Address, requests: Vec, ) -> Val { - let scalar: i128 = env.storage().instance().get(&M_SCALAR).unwrap(); - let rate: i128 = env.storage().instance().get(&M_RATE).unwrap(); + // Scalar and rate are always set in initialize(), so these are safe. + let scalar: i128 = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&M_SCALAR), + ); + let rate: i128 = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&M_RATE), + ); let mut collateral: i128 = env.storage().instance().get(&M_COLLAT).unwrap_or(0); for req in requests.iter() { @@ -453,14 +503,24 @@ mod tests { } pub fn get_reserve(env: Env, asset: Address) -> Reserve { - let internal_scalar: i128 = env.storage().instance().get(&M_SCALAR).unwrap(); + // Scalar and rate are always set in initialize(), so these are safe. + let internal_scalar: i128 = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&M_SCALAR), + ); let scalar: i128 = env .storage() .instance() .get(&M_REP_SCL) .unwrap_or(internal_scalar); - let rate: i128 = env.storage().instance().get(&M_RATE).unwrap(); - let index: u32 = env.storage().instance().get(&M_INDEX).unwrap(); + let rate: i128 = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&M_RATE), + ); + let index: u32 = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&M_INDEX), + ); Reserve { asset, config: ReserveConfig { @@ -492,7 +552,12 @@ mod tests { } pub fn get_positions(env: Env, _address: Address) -> Positions { - let index: u32 = env.storage().instance().get(&M_INDEX).unwrap(); + // Index is always set in initialize(), so this is safe. + let index: u32 = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&M_INDEX), + ); + // Collateral safely defaults to 0 if not set yet, which is correct for a fresh adapter. let collateral: i128 = env.storage().instance().get(&M_COLLAT).unwrap_or(0); let mut collateral_map = Map::new(&env); collateral_map.set(index, collateral); @@ -593,6 +658,34 @@ mod tests { assert_eq!(adapter.total_assets(), amount + amount / 10); } + #[test] + fn accrue_returns_typed_error_when_pool_key_is_unset() { + // __constructor always sets POOL_KEY on any real deployment, so this + // state is unreachable in practice; this test clears it directly + // after construction to prove accrue() still fails with a typed + // error rather than an opaque unwrap panic if that invariant is ever + // violated by a future change. + let (env, _vault, _usdc, adapter, _pool) = setup(); + env.as_contract(&adapter.address, || { + env.storage().instance().remove(&POOL_KEY); + }); + + assert_eq!(adapter.try_accrue(), Err(Ok(ContractError::NotInitialized))); + } + + #[test] + #[should_panic] + fn refresh_panics_when_pool_key_is_unset() { + // refresh() has no error return (shared adapter interface), so it + // must panic rather than silently no-op when accrue() fails. + let (env, _vault, _usdc, adapter, _pool) = setup(); + env.as_contract(&adapter.address, || { + env.storage().instance().remove(&POOL_KEY); + }); + + adapter.refresh(); + } + #[test] fn accrue_ignores_reserve_scalar_and_uses_the_real_rate_base() { // Regression test for the bug fixed alongside RATE_SCALAR: accrue() diff --git a/packages/contracts/defindex-adapter/src/lib.rs b/packages/contracts/defindex-adapter/src/lib.rs index 24b8bfaa..101cbc7c 100644 --- a/packages/contracts/defindex-adapter/src/lib.rs +++ b/packages/contracts/defindex-adapter/src/lib.rs @@ -52,6 +52,8 @@ pub trait DefindexVaultInterface { pub enum ContractError { /// `initialize` was called on an adapter that already has a vault set. AlreadyInitialized = 1, + /// A state-mutating call was made before `initialize`. + NotInitialized = 2, } impl From for ContractError { @@ -62,6 +64,12 @@ impl From for ContractError { } } +impl adapter_common::NotInitializedError for ContractError { + fn not_initialized() -> Self { + ContractError::NotInitialized + } +} + // --------------------------------------------------------------------------- // Contract // --------------------------------------------------------------------------- @@ -124,7 +132,10 @@ impl MeridianDefindexAdapter { pub fn deposit(env: Env, amount: i128) -> i128 { require_vault_auth(&env); - let dfx: Address = env.storage().instance().get(&DFX_VAULT).unwrap(); + let dfx: Address = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&DFX_VAULT), + ); let adapter = env.current_contract_address(); let client = DefindexVaultClient::new(&env, &dfx); @@ -141,13 +152,18 @@ impl MeridianDefindexAdapter { pub fn withdraw(env: Env, shares: i128, recipient: Address) -> i128 { require_vault_auth(&env); - let dfx: Address = env.storage().instance().get(&DFX_VAULT).unwrap(); + let dfx: Address = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&DFX_VAULT), + ); let usdc = get_usdc(&env); let adapter = env.current_contract_address(); let amounts = DefindexVaultClient::new(&env, &dfx).withdraw(&shares, &vec![&env, 0_i128], &adapter); + // Vec.get() returns Option which safely defaults to 0 if the vector doesn't contain + // index 0 or is empty, so this unwrap_or is safe and intentional. let usdc_out: i128 = amounts.get(0).unwrap_or(0); if usdc_out > 0 { TokenClient::new(&env, &usdc).transfer(&adapter, &recipient, &usdc_out); @@ -159,7 +175,10 @@ impl MeridianDefindexAdapter { /// Live USDC value of the adapter's dfToken position, computed by the /// DeFindex vault's exchange rate. Updates automatically as yield accrues. pub fn total_assets(env: Env) -> i128 { - let dfx: Address = env.storage().instance().get(&DFX_VAULT).unwrap(); + let dfx: Address = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&DFX_VAULT), + ); let adapter = env.current_contract_address(); let client = DefindexVaultClient::new(&env, &dfx); @@ -169,6 +188,8 @@ impl MeridianDefindexAdapter { } let amounts = client.get_asset_amounts_per_shares(&shares); + // Vec.get() returns Option which safely defaults to 0 if the vector doesn't contain + // index 0 or is empty, so this unwrap_or is safe and intentional. amounts.get(0).unwrap_or(0) } @@ -178,7 +199,10 @@ impl MeridianDefindexAdapter { /// Returns the DeFindex vault this adapter deposits into. pub fn get_pool(env: Env) -> Address { - env.storage().instance().get(&DFX_VAULT).unwrap() + adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&DFX_VAULT), + ) } /// Returns "defindex", identifying which protocol this adapter wraps. @@ -236,7 +260,12 @@ mod tests { from: Address, _invest: bool, ) -> Val { - let usdc: Address = env.storage().instance().get(&MDV_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&MDV_USDC), + ); + // Vec.get() safely returns Option which defaults to 0, so unwrap_or is safe. let amount = amounts_desired.get(0).unwrap_or(0); TokenClient::new(&env, &usdc).transfer(&from, &env.current_contract_address(), &amount); @@ -262,7 +291,12 @@ mod tests { .get(&MDV_WAMT) .unwrap_or_else(|| vec![&env, withdraw_shares]); - let usdc: Address = env.storage().instance().get(&MDV_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = adapter_common::get_or_not_initialized::<_, ContractError>( + &env, + env.storage().instance().get(&MDV_USDC), + ); + // Vec.get() safely returns Option which defaults to 0, so unwrap_or is safe. let payout = amounts.get(0).unwrap_or(0); if payout > 0 { TokenClient::new(&env, &usdc).transfer( @@ -324,6 +358,27 @@ mod tests { assert_eq!(adapter.get_pool(), dfx.address); } + #[test] + #[should_panic] + fn withdraw_panics_with_typed_error_when_dfx_vault_is_unset() { + // __constructor always sets DFX_VAULT on any real deployment, so + // this state is unreachable in practice; this test clears it + // directly after construction to prove withdraw() still fails with + // the typed NotInitialized panic rather than an opaque unwrap trap + // if that invariant is ever violated by a future change. + let (env, vault, usdc_id, adapter, _dfx) = setup(); + let amount = 100_0000000_i128; + TokenClient::new(&env, &usdc_id).transfer(&vault, &adapter.address, &amount); + adapter.deposit(&amount); + + env.as_contract(&adapter.address, || { + env.storage().instance().remove(&DFX_VAULT); + }); + + let recipient = Address::generate(&env); + adapter.withdraw(&amount, &recipient); + } + #[test] fn get_protocol_returns_defindex() { let (env, _vault, _usdc, adapter, _dfx) = setup(); diff --git a/packages/contracts/vault/src/lib.rs b/packages/contracts/vault/src/lib.rs index 5da2079f..28b044af 100644 --- a/packages/contracts/vault/src/lib.rs +++ b/packages/contracts/vault/src/lib.rs @@ -555,12 +555,21 @@ impl MeridianVault { mod tests { use super::*; use soroban_sdk::{ - contract, contractimpl, symbol_short, + contract, contractimpl, panic_with_error, symbol_short, testutils::{Address as _, Ledger as _}, token::{StellarAssetClient, TokenClient}, Address, Env, Symbol, }; + // Reads an instance-storage value, panicking with the typed + // NotInitialized error instead of an opaque unwrap trap if it's unset. + // Collapses what was previously a repeated 6-line + // `unwrap_or_else(|| { panic_with_error!(...) })` block, used across + // these mock adapters, into one call site per use. + fn get_or_not_initialized(env: &Env, value: Option) -> T { + value.unwrap_or_else(|| panic_with_error!(env, ContractError::NotInitialized)) + } + // ----------------------------------------------------------------------- // Shared logic for the proportional, live-priced mock adapters below // (MockAdapter, LossyMockAdapter, ZeroShareMockAdapter). Each mock has @@ -627,12 +636,16 @@ mod tests { } pub fn withdraw(env: Env, shares: i128, recipient: Address) -> i128 { - let usdc: Address = env.storage().instance().get(&MA_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = + get_or_not_initialized(&env, env.storage().instance().get(&MA_USDC)); mock_proportional_withdraw(&env, &usdc, &MA_SH, shares, &recipient) } pub fn total_assets(env: Env) -> i128 { - let usdc: Address = env.storage().instance().get(&MA_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = + get_or_not_initialized(&env, env.storage().instance().get(&MA_USDC)); mock_total_assets(&env, &usdc) } @@ -666,7 +679,9 @@ mod tests { } pub fn deposit(env: Env, amount: i128) -> i128 { - let usdc: Address = env.storage().instance().get(&LA_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = + get_or_not_initialized(&env, env.storage().instance().get(&LA_USDC)); let half = amount / 2; let sink = Address::generate(&env); TokenClient::new(&env, &usdc).transfer( @@ -680,12 +695,16 @@ mod tests { } pub fn withdraw(env: Env, shares: i128, recipient: Address) -> i128 { - let usdc: Address = env.storage().instance().get(&LA_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = + get_or_not_initialized(&env, env.storage().instance().get(&LA_USDC)); mock_proportional_withdraw(&env, &usdc, &LA_SH, shares, &recipient) } pub fn total_assets(env: Env) -> i128 { - let usdc: Address = env.storage().instance().get(&LA_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = + get_or_not_initialized(&env, env.storage().instance().get(&LA_USDC)); mock_total_assets(&env, &usdc) } @@ -727,12 +746,16 @@ mod tests { } pub fn withdraw(env: Env, shares: i128, recipient: Address) -> i128 { - let usdc: Address = env.storage().instance().get(&ZS_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = + get_or_not_initialized(&env, env.storage().instance().get(&ZS_USDC)); mock_proportional_withdraw(&env, &usdc, &ZS_SH, shares, &recipient) } pub fn total_assets(env: Env) -> i128 { - let usdc: Address = env.storage().instance().get(&ZS_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = + get_or_not_initialized(&env, env.storage().instance().get(&ZS_USDC)); mock_total_assets(&env, &usdc) } @@ -780,7 +803,9 @@ mod tests { // redemptions off the live b_rate (#486). This test double // intentionally uses live pricing so the test below can // isolate what refresh() itself does or doesn't affect. - let usdc: Address = env.storage().instance().get(&CM_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = + get_or_not_initialized(&env, env.storage().instance().get(&CM_USDC)); let total_sh: i128 = env.storage().instance().get(&CM_SH).unwrap_or(0); let balance = TokenClient::new(&env, &usdc).balance(&env.current_contract_address()); @@ -804,11 +829,15 @@ mod tests { pub fn total_assets(env: Env) -> i128 { // Cached: only reflects the balance as of the last refresh() call. + // Instance storage read defaults to 0 if CM_TOTAL hasn't been set, which is safe since + // initialize() sets this key to 0. env.storage().instance().get(&CM_TOTAL).unwrap_or(0) } pub fn refresh(env: Env) { - let usdc: Address = env.storage().instance().get(&CM_USDC).unwrap(); + // USDC address is always set in initialize(), so this is safe. + let usdc: Address = + get_or_not_initialized(&env, env.storage().instance().get(&CM_USDC)); let balance = TokenClient::new(&env, &usdc).balance(&env.current_contract_address()); env.storage().instance().set(&CM_TOTAL, &balance);