From bb6f21db7f7e409e8856d3a25bb06bbd0de8da4a Mon Sep 17 00:00:00 2001 From: Ricardo Guilherme Schmidt <3esmit@gmail.com> Date: Tue, 12 May 2026 13:04:59 -0300 Subject: [PATCH] fix(stablecoin): address open position review feedback --- Cargo.lock | 1 + artifacts/stablecoin-idl.json | 177 ++++++++++++++++-- stablecoin/core/Cargo.toml | 1 + stablecoin/core/src/lib.rs | 77 ++++---- stablecoin/methods/guest/Cargo.lock | 1 + .../methods/guest/src/bin/stablecoin.rs | 51 +---- stablecoin/src/lib.rs | 2 - stablecoin/src/noop.rs | 6 - stablecoin/src/open_position.rs | 28 ++- stablecoin/src/tests.rs | 58 +++--- 10 files changed, 259 insertions(+), 143 deletions(-) delete mode 100644 stablecoin/src/noop.rs diff --git a/Cargo.lock b/Cargo.lock index 4dc8bfe..531e40b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3044,6 +3044,7 @@ dependencies = [ "nssa_core", "risc0-zkvm", "serde", + "spel-framework-macros", ] [[package]] diff --git a/artifacts/stablecoin-idl.json b/artifacts/stablecoin-idl.json index 1015da1..015b818 100644 --- a/artifacts/stablecoin-idl.json +++ b/artifacts/stablecoin-idl.json @@ -2,18 +2,6 @@ "version": "0.1.0", "name": "stablecoin", "instructions": [ - { - "name": "noop", - "accounts": [ - { - "name": "account", - "writable": false, - "signer": false, - "init": false - } - ], - "args": [] - }, { "name": "open_position", "accounts": [ @@ -49,10 +37,6 @@ } ], "args": [ - { - "name": "stablecoin_program_id", - "type": "program_id" - }, { "name": "collateral_amount", "type": "u128" @@ -60,5 +44,166 @@ ] } ], + "accounts": [ + { + "name": "Position", + "type": { + "kind": "struct", + "fields": [ + { + "name": "collateral_vault_id", + "type": "account_id" + }, + { + "name": "collateral_definition_id", + "type": "account_id" + }, + { + "name": "collateral_amount", + "type": "u128" + }, + { + "name": "debt_amount", + "type": "u128" + } + ] + } + }, + { + "name": "TokenDefinition", + "type": { + "kind": "enum", + "variants": [ + { + "name": "Fungible", + "fields": [ + { + "name": "name", + "type": "string" + }, + { + "name": "total_supply", + "type": "u128" + }, + { + "name": "metadata_id", + "type": { + "option": "account_id" + } + } + ] + }, + { + "name": "NonFungible", + "fields": [ + { + "name": "name", + "type": "string" + }, + { + "name": "printable_supply", + "type": "u128" + }, + { + "name": "metadata_id", + "type": "account_id" + } + ] + } + ] + } + }, + { + "name": "TokenHolding", + "type": { + "kind": "enum", + "variants": [ + { + "name": "Fungible", + "fields": [ + { + "name": "definition_id", + "type": "account_id" + }, + { + "name": "balance", + "type": "u128" + } + ] + }, + { + "name": "NftMaster", + "fields": [ + { + "name": "definition_id", + "type": "account_id" + }, + { + "name": "print_balance", + "type": "u128" + } + ] + }, + { + "name": "NftPrintedCopy", + "fields": [ + { + "name": "definition_id", + "type": "account_id" + }, + { + "name": "owned", + "type": "bool" + } + ] + } + ] + } + }, + { + "name": "TokenMetadata", + "type": { + "kind": "struct", + "fields": [ + { + "name": "definition_id", + "type": "account_id" + }, + { + "name": "standard", + "type": { + "defined": "MetadataStandard" + } + }, + { + "name": "uri", + "type": "string" + }, + { + "name": "creators", + "type": "string" + }, + { + "name": "primary_sale_date", + "type": "u64" + } + ] + } + } + ], + "types": [ + { + "name": "MetadataStandard", + "kind": "enum", + "variants": [ + { + "name": "Simple" + }, + { + "name": "Expanded" + } + ] + } + ], "instruction_type": "stablecoin_core::Instruction" } diff --git a/stablecoin/core/Cargo.toml b/stablecoin/core/Cargo.toml index d63f52e..a4ca11e 100644 --- a/stablecoin/core/Cargo.toml +++ b/stablecoin/core/Cargo.toml @@ -8,3 +8,4 @@ nssa_core = { git = "https://github.com/logos-blockchain/logos-execution-zone.gi borsh = { version = "1.5", features = ["derive"] } serde = { version = "1.0", features = ["derive"] } risc0-zkvm = { version = "=3.0.5", default-features = false } +spel-framework-macros = { git = "https://github.com/logos-co/spel.git", rev = "6473ab4c400bc59bac8db83a286faaeafa7d1999", package = "spel-framework-macros" } diff --git a/stablecoin/core/src/lib.rs b/stablecoin/core/src/lib.rs index a96b53c..11527c7 100644 --- a/stablecoin/core/src/lib.rs +++ b/stablecoin/core/src/lib.rs @@ -6,34 +6,27 @@ use nssa_core::{ program::{PdaSeed, ProgramId}, }; use serde::{Deserialize, Serialize}; +use spel_framework_macros::account_type; -// Domain-separation tags for the PDA seeds derived by the Stablecoin Program. -// These bytes are part of the on-chain derivation scheme and must stay unchanged for -// account-address compatibility. -const POSITION_PDA_DOMAIN: [u8; 32] = *b"stablecoin::position::seed::v1\0\0"; -const POSITION_VAULT_PDA_DOMAIN: [u8; 32] = *b"stablecoin::position::vault::v1\0"; +const POSITION_PDA_DOMAIN: [u8; 32] = [0; 32]; +const POSITION_VAULT_PDA_DOMAIN: [u8; 32] = [1; 32]; /// Stablecoin Program Instruction. #[derive(Debug, Serialize, Deserialize)] pub enum Instruction { - /// Heartbeat / sanity-check entry point that returns the input account unchanged. - Noop, - /// Open a new collateral-only [`Position`] for the calling owner. /// /// Required accounts (5): /// - Owner account (authorized) /// - Position account (uninitialized, address must match - /// `compute_position_pda(stablecoin_program_id, owner)`) + /// `compute_position_pda(self_program_id, owner, token_definition)`) /// - Position vault token holding account (uninitialized, address must match - /// `compute_position_vault_pda(stablecoin_program_id, position_id)`) + /// `compute_position_vault_pda(self_program_id, position_id)`) /// - Owner's source token holding for the collateral (authorized, initialized) /// - Token definition account for the collateral (matches the user holding's `definition_id`; /// its `program_owner` determines the Token Program used by the chained `InitializeAccount` /// / `Transfer` calls) OpenPosition { - /// `ProgramId` under which the [`Position`] and vault PDAs are derived. - stablecoin_program_id: ProgramId, /// Amount of collateral tokens to deposit into the position vault. collateral_amount: u128, }, @@ -43,6 +36,7 @@ pub enum Instruction { /// /// `debt_amount` is included for forward compatibility with `generate_debt`; until that /// instruction lands `open_position` always initializes it to `0`. +#[account_type] #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, BorshSerialize, BorshDeserialize)] pub struct Position { /// Token holding account (vault PDA) that custodies the collateral backing this position. @@ -66,30 +60,26 @@ impl TryFrom<&Data> for Position { impl From<&Position> for Data { fn from(position: &Position) -> Self { let mut data = Vec::with_capacity(std::mem::size_of_val(position)); - #[allow( - clippy::expect_used, - reason = "BorshSerialize::serialize is infallible when writing to a Vec" - )] BorshSerialize::serialize(position, &mut data) .expect("Serialization to Vec should not fail"); - #[allow( - clippy::expect_used, - reason = "Position encodes to a small, bounded byte length that fits Data" - )] Self::try_from(data).expect("Position encoded data should fit into Data") } } -/// PDA seed for the [`Position`] account owned by `owner_id`. +/// PDA seed for the [`Position`] account owned by `owner_id` for `collateral_definition_id`. /// -/// Derived from the owner's address with a domain-separation tag so the resulting seed -/// cannot collide with other stablecoin-managed PDAs that hash the same owner id. -pub fn compute_position_pda_seed(owner_id: AccountId) -> PdaSeed { - use risc0_zkvm::sha::{Impl, Sha256}; +/// Derived from the owner and collateral definition addresses with a domain-separation tag +/// so one owner can hold separate positions for separate collateral definitions. +pub fn compute_position_pda_seed( + owner_id: AccountId, + collateral_definition_id: AccountId, +) -> PdaSeed { + use risc0_zkvm::sha::{Impl, Sha256 as _}; - let mut bytes = [0u8; 64]; + let mut bytes = [0u8; 96]; bytes[0..32].copy_from_slice(&owner_id.to_bytes()); - bytes[32..64].copy_from_slice(&POSITION_PDA_DOMAIN); + bytes[32..64].copy_from_slice(&collateral_definition_id.to_bytes()); + bytes[64..96].copy_from_slice(&POSITION_PDA_DOMAIN); let mut out = [0u8; 32]; out.copy_from_slice(Impl::hash_bytes(&bytes).as_bytes()); @@ -97,8 +87,15 @@ pub fn compute_position_pda_seed(owner_id: AccountId) -> PdaSeed { } /// Account id of the [`Position`] PDA owned by `owner_id` under `stablecoin_program_id`. -pub fn compute_position_pda(stablecoin_program_id: ProgramId, owner_id: AccountId) -> AccountId { - AccountId::from((&stablecoin_program_id, &compute_position_pda_seed(owner_id))) +pub fn compute_position_pda( + stablecoin_program_id: ProgramId, + owner_id: AccountId, + collateral_definition_id: AccountId, +) -> AccountId { + AccountId::for_public_pda( + &stablecoin_program_id, + &compute_position_pda_seed(owner_id, collateral_definition_id), + ) } /// PDA seed for the collateral vault token holding bound to a [`Position`]. @@ -106,7 +103,7 @@ pub fn compute_position_pda(stablecoin_program_id: ProgramId, owner_id: AccountI /// Derived from the position's address with a distinct domain-separation tag so the vault /// id cannot collide with the position id even though both PDAs share the same program. pub fn compute_position_vault_pda_seed(position_id: AccountId) -> PdaSeed { - use risc0_zkvm::sha::{Impl, Sha256}; + use risc0_zkvm::sha::{Impl, Sha256 as _}; let mut bytes = [0u8; 64]; bytes[0..32].copy_from_slice(&position_id.to_bytes()); @@ -122,25 +119,27 @@ pub fn compute_position_vault_pda( stablecoin_program_id: ProgramId, position_id: AccountId, ) -> AccountId { - AccountId::from(( + AccountId::for_public_pda( &stablecoin_program_id, &compute_position_vault_pda_seed(position_id), - )) + ) } -/// Verify the position account's address matches `(stablecoin_program_id, owner)` and -/// return the [`PdaSeed`] for use in chained calls. +/// Verify the position account's address matches +/// `(stablecoin_program_id, owner, collateral_definition_id)` and return the [`PdaSeed`] for +/// use in post-state claims. /// /// # Panics -/// If `position.account_id` does not match the address derived from `owner` and -/// `stablecoin_program_id`. +/// If `position.account_id` does not match the address derived from `owner`, +/// `collateral_definition_id`, and `stablecoin_program_id`. pub fn verify_position_and_get_seed( position: &AccountWithMetadata, owner: &AccountWithMetadata, + collateral_definition_id: AccountId, stablecoin_program_id: ProgramId, ) -> PdaSeed { - let seed = compute_position_pda_seed(owner.account_id); - let expected_id = AccountId::from((&stablecoin_program_id, &seed)); + let seed = compute_position_pda_seed(owner.account_id, collateral_definition_id); + let expected_id = AccountId::for_public_pda(&stablecoin_program_id, &seed); assert_eq!( position.account_id, expected_id, "Position account ID does not match expected derivation" @@ -160,7 +159,7 @@ pub fn verify_position_vault_and_get_seed( stablecoin_program_id: ProgramId, ) -> PdaSeed { let seed = compute_position_vault_pda_seed(position_id); - let expected_id = AccountId::from((&stablecoin_program_id, &seed)); + let expected_id = AccountId::for_public_pda(&stablecoin_program_id, &seed); assert_eq!( vault.account_id, expected_id, "Position vault account ID does not match expected derivation" diff --git a/stablecoin/methods/guest/Cargo.lock b/stablecoin/methods/guest/Cargo.lock index b04737f..eacd8fe 100644 --- a/stablecoin/methods/guest/Cargo.lock +++ b/stablecoin/methods/guest/Cargo.lock @@ -2948,6 +2948,7 @@ dependencies = [ "nssa_core", "risc0-zkvm", "serde", + "spel-framework-macros", ] [[package]] diff --git a/stablecoin/methods/guest/src/bin/stablecoin.rs b/stablecoin/methods/guest/src/bin/stablecoin.rs index 5f46e9c..d9ff26e 100644 --- a/stablecoin/methods/guest/src/bin/stablecoin.rs +++ b/stablecoin/methods/guest/src/bin/stablecoin.rs @@ -1,65 +1,29 @@ -//! RISC Zero guest binary for the Stablecoin Program. -//! -//! Wires the host-side `stablecoin_program` instruction handlers to the LEZ framework via -//! `#[lez_program]` so the entry points can be invoked from a deployed program. - #![no_main] -#![allow( - missing_docs, - reason = "lez_program / instruction proc macros emit module, function, and constant items \ - that do not carry doc strings; user-written handlers document inline" -)] -use nssa_core::{account::AccountWithMetadata, program::ProgramId}; +use nssa_core::account::AccountWithMetadata; +use spel_framework::context::ProgramContext; use spel_framework::prelude::*; risc0_zkvm::guest::entry!(main); #[lez_program(instruction = "stablecoin_core::Instruction")] mod stablecoin { - #[allow( - unused_imports, - reason = "lez_program expansion may or may not reference every super:: import" - )] + #[allow(unused_imports)] use super::*; - /// Heartbeat instruction that returns the input account unchanged. - /// - /// # Errors - /// Currently never; reserved for future validation paths. - #[instruction] - #[allow( - deprecated, - reason = "SpelOutput::states_only: lez_program macro only rewrites vec![a, b, ...] \ - literals into execute_with_claims; this handler delegates to a host function \ - that returns Vec, so migration requires restructuring the \ - handler shape — workspace-wide follow-up across token/amm/ata" - )] - pub fn noop(account: AccountWithMetadata) -> SpelResult { - Ok(spel_framework::SpelOutput::execute(stablecoin_program::noop::noop( - account, - ), vec![])) - } - /// Open a new collateral-only position for the calling owner. /// /// # Errors /// Returns the host program's panic-converted error if any precondition fails (see /// [`stablecoin_program::open_position::open_position`] for the full list). #[instruction] - #[allow( - deprecated, - reason = "SpelOutput::with_chained_calls: same reason as noop above — migration to \ - SpelOutput::execute requires the macro's vec![...] literal shape, which \ - conflicts with delegating to host helpers" - )] pub fn open_position( + ctx: ProgramContext, owner: AccountWithMetadata, position: AccountWithMetadata, vault: AccountWithMetadata, user_holding: AccountWithMetadata, token_definition: AccountWithMetadata, - stablecoin_program_id: ProgramId, collateral_amount: u128, ) -> SpelResult { let (post_states, chained_calls) = stablecoin_program::open_position::open_position( @@ -68,9 +32,12 @@ mod stablecoin { vault, user_holding, token_definition, - stablecoin_program_id, + ctx.self_program_id, collateral_amount, ); - Ok(SpelOutput::with_chained_calls(post_states, chained_calls)) + Ok(spel_framework::SpelOutput::execute( + post_states, + chained_calls, + )) } } diff --git a/stablecoin/src/lib.rs b/stablecoin/src/lib.rs index 4790558..7f5c47e 100644 --- a/stablecoin/src/lib.rs +++ b/stablecoin/src/lib.rs @@ -2,8 +2,6 @@ pub use stablecoin_core as core; -/// No-op instruction used as a heartbeat / sanity-check entry point. -pub mod noop; /// Open a new collateral-only position for a calling owner. pub mod open_position; diff --git a/stablecoin/src/noop.rs b/stablecoin/src/noop.rs deleted file mode 100644 index b405083..0000000 --- a/stablecoin/src/noop.rs +++ /dev/null @@ -1,6 +0,0 @@ -use nssa_core::{account::AccountWithMetadata, program::AccountPostState}; - -/// Pass `account` through unchanged as a single post-state entry. -pub fn noop(account: AccountWithMetadata) -> Vec { - vec![AccountPostState::new(account.account)] -} diff --git a/stablecoin/src/open_position.rs b/stablecoin/src/open_position.rs index 35b836e..d3fb71f 100644 --- a/stablecoin/src/open_position.rs +++ b/stablecoin/src/open_position.rs @@ -23,11 +23,6 @@ use token_core::TokenHolding; /// - `user_holding` cannot be decoded as a [`TokenHolding`]. /// - `user_holding`'s definition does not match `token_definition`. /// - `token_definition.program_owner` does not match `user_holding.program_owner`. -#[allow( - clippy::needless_pass_by_value, - reason = "instruction handler shape: spel #[instruction] macro deserializes owned \ - AccountWithMetadata values into these parameters" -)] pub fn open_position( owner: AccountWithMetadata, position: AccountWithMetadata, @@ -53,15 +48,6 @@ pub fn open_position( "Position vault account must be uninitialized" ); - let position_seed = verify_position_and_get_seed(&position, &owner, stablecoin_program_id); - let vault_seed = - verify_position_vault_and_get_seed(&vault, position.account_id, stablecoin_program_id); - - #[allow( - clippy::expect_used, - reason = "open_position uses the same panic-on-bad-input pattern as the surrounding \ - assert!/assert_eq! invariant checks; runtime must supply a valid TokenHolding" - )] let user_holding_definition_id = TokenHolding::try_from(&user_holding.account.data) .expect("User holding must be a valid Token Holding") .definition_id(); @@ -75,6 +61,15 @@ pub fn open_position( "Collateral token definition is not owned by the user holding's Token Program" ); + let position_seed = verify_position_and_get_seed( + &position, + &owner, + token_definition.account_id, + stablecoin_program_id, + ); + let vault_seed = + verify_position_vault_and_get_seed(&vault, position.account_id, stablecoin_program_id); + let mut position_post = position.account; position_post.program_owner = stablecoin_program_id; position_post.data = Data::from(&Position { @@ -92,9 +87,8 @@ pub fn open_position( AccountPostState::new(token_definition.account.clone()), ]; - // Chained Token::InitializeAccount sets the vault up as a zero-balance holding of the - // collateral token. We hand the runtime the vault marked authorized via PDA so it can - // satisfy `InitializeAccount`'s authorization requirement without a user signature. + // Chained Token::InitializeAccount owns the vault as a Token holding. The Stablecoin + // program only authorizes that claim by passing the vault PDA seed to the chained call. let mut vault_authorized = vault.clone(); vault_authorized.is_authorized = true; let initialize_call = ChainedCall::new( diff --git a/stablecoin/src/tests.rs b/stablecoin/src/tests.rs index 03ec652..b7fe339 100644 --- a/stablecoin/src/tests.rs +++ b/stablecoin/src/tests.rs @@ -1,5 +1,4 @@ #![allow( - clippy::expect_used, clippy::indexing_slicing, clippy::panic, clippy::unwrap_used, @@ -32,7 +31,11 @@ fn user_holding_id() -> AccountId { } fn position_id() -> AccountId { - compute_position_pda(STABLECOIN_PROGRAM_ID, owner_id()) + compute_position_pda( + STABLECOIN_PROGRAM_ID, + owner_id(), + collateral_definition_id(), + ) } fn vault_id() -> AccountId { @@ -96,17 +99,6 @@ fn uninit_vault_account() -> AccountWithMetadata { } } -#[test] -fn noop_returns_single_post_state() { - let account = AccountWithMetadata { - account: Account::default(), - is_authorized: false, - account_id: AccountId::new([0u8; 32]), - }; - let post_states = crate::noop::noop(account); - assert_eq!(post_states.len(), 1); -} - #[test] fn open_position_claims_pda_and_emits_chained_calls() { let collateral_amount: u128 = 500; @@ -126,7 +118,10 @@ fn open_position_claims_pda_and_emits_chained_calls() { let position_post = &post_states[1]; assert_eq!( position_post.required_claim(), - Some(Claim::Pda(compute_position_pda_seed(owner_id()))) + Some(Claim::Pda(compute_position_pda_seed( + owner_id(), + collateral_definition_id() + ))) ); let position = Position::try_from(&position_post.account().data).expect("valid Position"); assert_eq!( @@ -356,23 +351,44 @@ fn open_position_rejects_definition_with_wrong_token_program() { } #[test] -fn position_pda_is_deterministic_and_owner_specific() { - let id_a = compute_position_pda(STABLECOIN_PROGRAM_ID, owner_id()); - let id_b = compute_position_pda(STABLECOIN_PROGRAM_ID, owner_id()); +fn position_pda_is_deterministic_and_owner_and_collateral_specific() { + let id_a = compute_position_pda( + STABLECOIN_PROGRAM_ID, + owner_id(), + collateral_definition_id(), + ); + let id_b = compute_position_pda( + STABLECOIN_PROGRAM_ID, + owner_id(), + collateral_definition_id(), + ); assert_eq!(id_a, id_b); let other_owner = AccountId::new([0x11u8; 32]); assert_ne!( - compute_position_pda(STABLECOIN_PROGRAM_ID, other_owner), + compute_position_pda( + STABLECOIN_PROGRAM_ID, + other_owner, + collateral_definition_id() + ), + id_a + ); + + let other_definition = AccountId::new([0x21u8; 32]); + assert_ne!( + compute_position_pda(STABLECOIN_PROGRAM_ID, owner_id(), other_definition), id_a ); } #[test] fn position_pda_and_vault_pda_do_not_collide() { - // Distinct domain tags must keep the position id and its vault id disjoint, even - // though both derivations involve only the owner's address. - let position = compute_position_pda(STABLECOIN_PROGRAM_ID, owner_id()); + // Distinct domain tags must keep the position id and its vault id disjoint. + let position = compute_position_pda( + STABLECOIN_PROGRAM_ID, + owner_id(), + collateral_definition_id(), + ); let vault = compute_position_vault_pda(STABLECOIN_PROGRAM_ID, position); assert_ne!(position, vault); }