From f62444ffa2bcdef16c6fc0c6c6b682be454ef449 Mon Sep 17 00:00:00 2001 From: Andrea Franz Date: Wed, 19 Aug 2026 09:59:45 +0000 Subject: [PATCH] refactor(stablecoin): address review on Position migration --- programs/stablecoin/core/src/lib.rs | 17 +++--- programs/stablecoin/src/open_position.rs | 2 +- programs/stablecoin/src/repay_debt.rs | 16 ++++-- programs/stablecoin/src/tests.rs | 53 +++++++++++++++++++ .../stablecoin/src/withdraw_collateral.rs | 18 +++++-- 5 files changed, 88 insertions(+), 18 deletions(-) diff --git a/programs/stablecoin/core/src/lib.rs b/programs/stablecoin/core/src/lib.rs index c14bb3a..8116e0a 100644 --- a/programs/stablecoin/core/src/lib.rs +++ b/programs/stablecoin/core/src/lib.rs @@ -40,7 +40,7 @@ pub enum Instruction { /// Required accounts (5): /// - Owner account (authorized) /// - Position account (uninitialized, address must match - /// `compute_position_pda(self_program_id, owner, token_definition)`) + /// `compute_position_pda(self_program_id, owner, position_nonce)`) /// - Position vault token holding account (uninitialized, address must match /// `compute_position_vault_pda(self_program_id, position_id)`) /// - Owner's source token holding for the collateral (authorized, initialized) @@ -62,14 +62,14 @@ pub enum Instruction { /// - Position vault token holding (address must match /// `compute_position_vault_pda(self_program_id, position_id)`) /// - Destination user collateral holding (initialized, owned by the vault's Token Program, - /// `TokenHolding.definition_id == Position.collateral_definition_id`) + /// `TokenHolding.definition_id` matches the vault holding's definition) /// /// `token_program_id` is derived from `vault.account.program_owner`; - /// `collateral_definition_id` is read from the decoded [`Position`]. + /// the collateral definition is read from the PDA-verified vault holding. /// - /// **Note:** until issues #97/#96/#95 land, this instruction hard-asserts - /// `Position.debt_amount == 0` instead of accruing fees and checking the - /// collateralization ratio. + /// **Note:** until issues #97/#95 land, this instruction hard-asserts + /// `Position.normalized_debt_amount == 0` instead of accruing fees and + /// checking the collateralization ratio. WithdrawCollateral { /// Amount of collateral tokens to move from the vault back to `destination`. amount: u128, @@ -84,13 +84,14 @@ pub enum Instruction { /// the definition, with `TokenHolding.definition_id == stablecoin_definition.account_id`) /// /// `token_program_id` is derived from `user_stablecoin_holding.account.program_owner`. - /// `collateral_definition_id` (for position PDA verification) is read from the + /// `position_nonce` (for position PDA verification) is read from the /// decoded [`Position`]. /// /// **Note:** until issue #97 (stability fee accrual) lands, this instruction does /// not accrue fees before reducing debt. A `// TODO(#97)` comment in the host /// function marks where the accrual code will plug in. Today every position has - /// `debt_amount = 0` (no `generate_debt` yet), so the precondition is vacuously met. + /// `normalized_debt_amount = 0` (no `generate_debt` yet), so the precondition + /// is vacuously met. /// /// **Note:** until issue #91 (`generate_debt`) records the stablecoin definition /// into `Position`, this instruction cannot validate that the passed diff --git a/programs/stablecoin/src/open_position.rs b/programs/stablecoin/src/open_position.rs index 0933915..38f03e6 100644 --- a/programs/stablecoin/src/open_position.rs +++ b/programs/stablecoin/src/open_position.rs @@ -78,7 +78,7 @@ pub fn open_position( vault_account_id: vault.account_id, collateral_amount, normalized_debt_amount: 0, - // TODO(Plan 3): read from ctx clock once `open_position` is rebuilt with + // TODO(#173): read from ctx clock once `open_position` is rebuilt with // the fee-aware flow. Setting 0 keeps Plan 1 a pure refactor. opened_at: 0, }); diff --git a/programs/stablecoin/src/repay_debt.rs b/programs/stablecoin/src/repay_debt.rs index 25ddd5e..79f0efc 100644 --- a/programs/stablecoin/src/repay_debt.rs +++ b/programs/stablecoin/src/repay_debt.rs @@ -12,13 +12,13 @@ use token_core::TokenHolding; /// amount. The position post-state uses plain [`AccountPostState::new`] — the /// PDA was already claimed at `open_position` time. /// -/// Until Plan 3 (stability fee accrual) lands, the fee-accrual step is a +/// Until #173 (stability fee accrual) lands, the fee-accrual step is a /// no-op (every position structurally has `normalized_debt_amount = 0` today /// because `generate_debt` is unimplemented; "fees-accrued" is therefore -/// vacuously true). A `// TODO(Plan 3)` comment marks where the accrual code +/// vacuously true). A `// TODO(#173)` comment marks where the accrual code /// will plug in — right before the `checked_sub` below. /// -/// Until Plan 3 (`generate_debt`) records the stablecoin definition into +/// Until #173 (`generate_debt`) records the stablecoin definition into /// `Position`, this instruction cannot validate that `stablecoin_definition` /// is the correct one for the position's debt. The caller is trusted. /// @@ -54,7 +54,7 @@ pub fn repay_debt( let position_data = Position::try_from(&position.account.data) .expect("Position account must hold valid Position state"); // `verify_position_and_get_seed` asserts the position address matches the - // (owner, collateral_definition) PDA derivation. The returned seed is + // (owner, position_nonce) PDA derivation. The returned seed is // dropped — the position is already PDA-claimed. let _position_seed = verify_position_and_get_seed( &position, @@ -62,6 +62,12 @@ pub fn repay_debt( position_data.position_nonce, stablecoin_program_id, ); + // The PDA derivation above already binds the owner; this guards the stored + // discovery copy against silently drifting out of sync. + assert_eq!( + position_data.owner_account_id, owner.account_id, + "Position owner_account_id does not match the owner account" + ); assert!( user_stablecoin_holding.is_authorized, @@ -89,7 +95,7 @@ pub fn repay_debt( "Stablecoin holding does not match the provided stablecoin definition" ); - // TODO(Plan 3): accrue stability fees onto position_data.normalized_debt_amount + // TODO(#173): accrue stability fees onto position_data.normalized_debt_amount // here, before the checked_sub below. Today every position has // normalized_debt_amount = 0 (no generate_debt yet), so the precondition is // trivially met. diff --git a/programs/stablecoin/src/tests.rs b/programs/stablecoin/src/tests.rs index 3582b7b..64a9335 100644 --- a/programs/stablecoin/src/tests.rs +++ b/programs/stablecoin/src/tests.rs @@ -959,3 +959,56 @@ fn repay_debt_rejects_overrepay() { 200, ); } + +/// Mutates only the stored discovery copy, leaving the position address (and so +/// the `(owner, position_nonce)` PDA derivation) valid — the state a corrupted +/// or malformed `Position` would present. +fn position_with_mutated_fields(mutate: impl FnOnce(&mut Position)) -> AccountWithMetadata { + let mut position = init_position_account(500, 0); + let mut data = Position::try_from(&position.account.data).expect("valid Position"); + mutate(&mut data); + position.account.data = Data::from(&data); + position +} + +#[test] +#[should_panic(expected = "Position owner_account_id does not match the owner account")] +fn withdraw_collateral_rejects_position_with_stale_owner_field() { + crate::withdraw_collateral::withdraw_collateral( + owner_account(), + position_with_mutated_fields(|p| p.owner_account_id = AccountId::new([0xAAu8; 32])), + init_vault_account(), + destination_holding_account(), + STABLECOIN_PROGRAM_ID, + 100, + ); +} + +#[test] +#[should_panic(expected = "Position vault_account_id does not match the vault account")] +fn withdraw_collateral_rejects_position_with_stale_vault_field() { + crate::withdraw_collateral::withdraw_collateral( + owner_account(), + position_with_mutated_fields(|p| p.vault_account_id = AccountId::new([0xBBu8; 32])), + init_vault_account(), + destination_holding_account(), + STABLECOIN_PROGRAM_ID, + 100, + ); +} + +#[test] +#[should_panic(expected = "Position owner_account_id does not match the owner account")] +fn repay_debt_rejects_position_with_stale_owner_field() { + crate::repay_debt::repay_debt( + owner_account(), + position_with_mutated_fields(|p| { + p.normalized_debt_amount = 300; + p.owner_account_id = AccountId::new([0xAAu8; 32]); + }), + stablecoin_definition_account(), + user_stablecoin_holding_account(1_000), + STABLECOIN_PROGRAM_ID, + 100, + ); +} diff --git a/programs/stablecoin/src/withdraw_collateral.rs b/programs/stablecoin/src/withdraw_collateral.rs index e25d751..c4cc31d 100644 --- a/programs/stablecoin/src/withdraw_collateral.rs +++ b/programs/stablecoin/src/withdraw_collateral.rs @@ -13,7 +13,7 @@ use token_core::TokenHolding; /// the initial PDA claim already happened in /// [`crate::open_position::open_position`]. /// -/// Until Plan 3 lands (redemption price, price feed, stability fee accrual), +/// Until #173 lands (redemption price, price feed, stability fee accrual), /// this instruction hard-asserts `Position.normalized_debt_amount == 0`. /// When that lands, this guard is replaced by real fee accrual + a /// collateralization-ratio check against the post-withdrawal collateral. @@ -52,7 +52,7 @@ pub fn withdraw_collateral( let position_data = Position::try_from(&position.account.data) .expect("Position account must hold valid Position state"); // `verify_position_and_get_seed` asserts the position address matches the - // (owner, collateral_definition) PDA derivation. We do not use the seed + // (owner, position_nonce) PDA derivation. We do not use the seed // downstream — the position is already PDA-claimed. let _position_seed = verify_position_and_get_seed( &position, @@ -60,15 +60,25 @@ pub fn withdraw_collateral( position_data.position_nonce, stablecoin_program_id, ); + // The PDA derivation above already binds the owner; this guards the stored + // discovery copy against silently drifting out of sync. + assert_eq!( + position_data.owner_account_id, owner.account_id, + "Position owner_account_id does not match the owner account" + ); let vault_seed = verify_position_vault_and_get_seed(&vault, position.account_id, stablecoin_program_id); + assert_eq!( + position_data.vault_account_id, vault.account_id, + "Position vault_account_id does not match the vault account" + ); let vault_holding = TokenHolding::try_from(&vault.account.data) .expect("Vault account must hold a valid TokenHolding"); // The vault PDA is verified to belong to this position, so its holding's // definition is the authoritative collateral definition. Plan 1 dropped the // redundant copy from `Position`; `ProtocolParameters` owns the global - // collateral definition from Plan 3 onward. + // collateral definition from #173 onward. let collateral_definition_id = vault_holding.definition_id(); let token_program_id = vault.account.program_owner; @@ -91,7 +101,7 @@ pub fn withdraw_collateral( assert_eq!( position_data.normalized_debt_amount, 0, - "withdraw_collateral with debt is not supported yet — fee accrual + collateralization check land in Plan 3" + "withdraw_collateral with debt is not supported yet — fee accrual + collateralization check land in #173" ); let new_collateral = position_data .collateral_amount