From f0de9f9a3abdc083902654045cdd4e539daf1ce8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Noah=20Bayindirli=20=F0=9F=A5=82?= Date: Fri, 7 Feb 2025 12:13:21 -0500 Subject: [PATCH] refactor(interchain-token): use contractstorage --- .../stellar-interchain-token/src/contract.rs | 107 +++++----------- contracts/stellar-interchain-token/src/lib.rs | 2 +- .../stellar-interchain-token/src/storage.rs | 36 ++++++ .../src/storage_types.rs | 24 ---- ...ata_key_storage_schema_is_unchanged.golden | 20 +++ .../src/tests/test.rs | 84 +++++++++++++ .../stellar-axelar-std-derive/src/storage.rs | 115 +++++++++++------- .../storage_schema_generation_succeeds.golden | 56 +++++++++ 8 files changed, 299 insertions(+), 145 deletions(-) create mode 100644 contracts/stellar-interchain-token/src/storage.rs delete mode 100644 contracts/stellar-interchain-token/src/storage_types.rs create mode 100644 contracts/stellar-interchain-token/src/testdata/ensure_data_key_storage_schema_is_unchanged.golden diff --git a/contracts/stellar-interchain-token/src/contract.rs b/contracts/stellar-interchain-token/src/contract.rs index 196560a2..29df0d2a 100644 --- a/contracts/stellar-interchain-token/src/contract.rs +++ b/contracts/stellar-interchain-token/src/contract.rs @@ -5,13 +5,13 @@ use soroban_token_sdk::metadata::TokenMetadata; use soroban_token_sdk::TokenUtils; use stellar_axelar_std::events::Event; use stellar_axelar_std::interfaces::OwnableInterface; -use stellar_axelar_std::ttl::{extend_instance_ttl, extend_persistent_ttl}; +use stellar_axelar_std::ttl::extend_instance_ttl; use stellar_axelar_std::{ensure, interfaces, Upgradable}; use crate::error::ContractError; use crate::event::{MinterAddedEvent, MinterRemovedEvent}; use crate::interface::InterchainTokenInterface; -use crate::storage_types::{AllowanceDataKey, AllowanceValue, DataKey}; +use crate::storage::{self, AllowanceDataKey, AllowanceValue}; #[contract] #[derive(Upgradable)] @@ -30,12 +30,10 @@ impl InterchainToken { Self::write_metadata(&env, token_metadata); - env.storage().instance().set(&DataKey::TokenId, &token_id); + storage::set_token_id(&env, &token_id); if let Some(minter) = minter { - env.storage() - .instance() - .set(&DataKey::Minter(minter.clone()), &()); + storage::set_minter_status(&env, minter.clone()); MinterAddedEvent { minter }.emit(&env); } @@ -96,14 +94,11 @@ impl StellarAssetInterface for InterchainToken { #[contractimpl] impl InterchainTokenInterface for InterchainToken { fn token_id(env: &Env) -> BytesN<32> { - env.storage() - .instance() - .get(&DataKey::TokenId) - .expect("token id not found") + storage::token_id(env) } fn is_minter(env: &Env, minter: Address) -> bool { - env.storage().instance().has(&DataKey::Minter(minter)) + storage::is_minter(env, minter) } fn mint_from( @@ -133,11 +128,7 @@ impl InterchainTokenInterface for InterchainToken { fn add_minter(env: &Env, minter: Address) { Self::owner(env).require_auth(); - env.storage() - .instance() - .set(&DataKey::Minter(minter.clone()), &()); - - extend_instance_ttl(env); + storage::set_minter_status(env, minter.clone()); MinterAddedEvent { minter }.emit(env); } @@ -145,11 +136,7 @@ impl InterchainTokenInterface for InterchainToken { fn remove_minter(env: &Env, minter: Address) { Self::owner(env).require_auth(); - env.storage() - .instance() - .remove(&DataKey::Minter(minter.clone())); - - extend_instance_ttl(env); + storage::remove_minter_status(env, minter.clone()); MinterRemovedEvent { minter }.emit(env); } @@ -183,8 +170,7 @@ impl token::Interface for InterchainToken { } fn balance(env: Env, id: Address) -> i128 { - extend_instance_ttl(&env); - Self::read_balance(&env, id) + storage::try_balance(&env, id).unwrap_or_default() } fn transfer(env: Env, from: Address, to: Address, amount: i128) { @@ -257,26 +243,23 @@ impl InterchainToken { } fn read_allowance(env: &Env, from: Address, spender: Address) -> AllowanceValue { - let key = DataKey::Allowance(AllowanceDataKey { from, spender }); - env.storage() - .temporary() - .get::<_, AllowanceValue>(&key) - .map_or( - AllowanceValue { - amount: 0, - expiration_ledger: 0, - }, - |allowance| { - if allowance.expiration_ledger < env.ledger().sequence() { - AllowanceValue { - amount: 0, - expiration_ledger: allowance.expiration_ledger, - } - } else { - allowance + let key = AllowanceDataKey { from, spender }; + storage::try_allowance(env, key).map_or( + AllowanceValue { + amount: 0, + expiration_ledger: 0, + }, + |allowance| { + if allowance.expiration_ledger < env.ledger().sequence() { + AllowanceValue { + amount: 0, + expiration_ledger: allowance.expiration_ledger, } - }, - ) + } else { + allowance + } + }, + ) } fn write_allowance( @@ -297,17 +280,15 @@ impl InterchainToken { ContractError::InvalidExpirationLedger ); - let key = DataKey::Allowance(AllowanceDataKey { from, spender }); - env.storage().temporary().set(&key, &allowance); + let key = AllowanceDataKey { from, spender }; + storage::set_allowance(env, key.clone(), &allowance); if amount > 0 { let live_for = expiration_ledger .checked_sub(env.ledger().sequence()) .unwrap(); - env.storage() - .temporary() - .extend_ttl(&key, live_for, live_for) + storage::extend_allowance_ttl(env, key, live_for, live_for); } } @@ -334,45 +315,21 @@ impl InterchainToken { } } - fn read_balance(env: &Env, addr: Address) -> i128 { - let key = DataKey::Balance(addr); - env.storage() - .persistent() - .get::<_, i128>(&key) - .inspect(|_| { - // Extend the TTL of the balance entry when the balance is successfully retrieved. - extend_persistent_ttl(env, &key); - }) - .unwrap_or_default() - } - fn receive_balance(env: &Env, addr: Address, amount: i128) { - let key = DataKey::Balance(addr); + let current_balance = storage::try_balance(env, addr.clone()).unwrap_or_default(); - env.storage() - .persistent() - .update(&key, |balance: Option| { - balance.unwrap_or_default() + amount - }); + storage::set_balance(env, addr, &(current_balance + amount)); } fn spend_balance(env: &Env, addr: Address, amount: i128) { - let balance = Self::read_balance(env, addr.clone()); + let balance = storage::try_balance(env, addr.clone()).unwrap_or_default(); assert_with_error!(env, balance >= amount, ContractError::InsufficientBalance); - Self::write_balance(env, addr, balance - amount); + storage::set_balance(env, addr, &(balance - amount)); } fn write_metadata(env: &Env, metadata: TokenMetadata) { TokenUtils::new(env).metadata().set_metadata(&metadata); } - - fn write_balance(env: &Env, addr: Address, amount: i128) { - let key = DataKey::Balance(addr); - - env.storage().persistent().set(&key, &amount); - - extend_persistent_ttl(env, &key); - } } diff --git a/contracts/stellar-interchain-token/src/lib.rs b/contracts/stellar-interchain-token/src/lib.rs index 0b88c687..dc4c1e93 100644 --- a/contracts/stellar-interchain-token/src/lib.rs +++ b/contracts/stellar-interchain-token/src/lib.rs @@ -15,7 +15,7 @@ cfg_if::cfg_if! { pub use interface::{InterchainTokenClient, InterchainTokenInterface}; } else { pub mod event; - mod storage_types; + mod storage; mod contract; pub use contract::{InterchainToken, InterchainTokenClient}; diff --git a/contracts/stellar-interchain-token/src/storage.rs b/contracts/stellar-interchain-token/src/storage.rs new file mode 100644 index 00000000..a0153cbd --- /dev/null +++ b/contracts/stellar-interchain-token/src/storage.rs @@ -0,0 +1,36 @@ +use soroban_sdk::{contracttype, Address, BytesN}; +use stellar_axelar_std::contractstorage; + +#[derive(Clone)] +#[contracttype] +pub struct AllowanceDataKey { + pub from: Address, + pub spender: Address, +} + +#[contracttype] +pub struct AllowanceValue { + pub amount: i128, + pub expiration_ledger: u32, +} + +/// Do not use the symbol `METADATA` as a key as it is reserved for token metadata. +#[contractstorage] +#[derive(Clone)] +pub enum DataKey { + #[temporary] + #[value(AllowanceValue)] + Allowance { key: AllowanceDataKey }, + + #[persistent] + #[value(i128)] + Balance { address: Address }, + + #[instance] + #[status] + Minter { minter: Address }, + + #[instance] + #[value(BytesN<32>)] + TokenId, +} diff --git a/contracts/stellar-interchain-token/src/storage_types.rs b/contracts/stellar-interchain-token/src/storage_types.rs deleted file mode 100644 index 53a705c1..00000000 --- a/contracts/stellar-interchain-token/src/storage_types.rs +++ /dev/null @@ -1,24 +0,0 @@ -use soroban_sdk::{contracttype, Address}; - -#[derive(Clone)] -#[contracttype] -pub struct AllowanceDataKey { - pub from: Address, - pub spender: Address, -} - -#[contracttype] -pub struct AllowanceValue { - pub amount: i128, - pub expiration_ledger: u32, -} - -/// Do not use the symbol `METADATA` as a key as it is reserved for token metadata. -#[contracttype] -#[derive(Clone)] -pub enum DataKey { - Allowance(AllowanceDataKey), - Balance(Address), - Minter(Address), - TokenId, -} diff --git a/contracts/stellar-interchain-token/src/testdata/ensure_data_key_storage_schema_is_unchanged.golden b/contracts/stellar-interchain-token/src/testdata/ensure_data_key_storage_schema_is_unchanged.golden new file mode 100644 index 00000000..03de45b0 --- /dev/null +++ b/contracts/stellar-interchain-token/src/testdata/ensure_data_key_storage_schema_is_unchanged.golden @@ -0,0 +1,20 @@ +/// Do not use the symbol `METADATA` as a key as it is reserved for token metadata. +#[derive(Clone)] +pub enum DataKey { + + #[temporary] + #[value(AllowanceValue)] + Allowance { key: AllowanceDataKey }, + + #[persistent] + #[value(i128)] + Balance { address: Address }, + + #[instance] + #[status] + Minter { minter: Address }, + + #[instance] + #[value(BytesN<32>)] + TokenId, +} diff --git a/contracts/stellar-interchain-token/src/tests/test.rs b/contracts/stellar-interchain-token/src/tests/test.rs index e1c2616f..ee7f933a 100644 --- a/contracts/stellar-interchain-token/src/tests/test.rs +++ b/contracts/stellar-interchain-token/src/tests/test.rs @@ -570,3 +570,87 @@ fn clawback_fails() { token.clawback(&token.owner(), &1); } + +#[test] +fn allowance_returns_zero_when_expired() { + let env = Env::default(); + + let user1 = Address::generate(&env); + let user2 = Address::generate(&env); + let amount = 1000; + + let (token, _) = setup_token(&env); + + // Set current ledger to 100 + let current_ledger = 100; + env.ledger().set_sequence_number(current_ledger); + + // Set allowance to expire at ledger 200 + let expiration_ledger = 200; + assert_auth!( + user1, + token.approve(&user1, &user2, &amount, &expiration_ledger) + ); + assert_eq!(token.allowance(&user1, &user2), amount); + + // Move to ledger after expiration + env.ledger().set_sequence_number(expiration_ledger + 1); + + // Allowance should be 0 after expiration + assert_eq!(token.allowance(&user1, &user2), 0); +} + +#[test] +#[should_panic(expected = "HostError: Error(Contract, #7)")] // InvalidExpirationLedger +fn approve_fails_with_expired_ledger() { + let env = Env::default(); + + let user1 = Address::generate(&env); + let user2 = Address::generate(&env); + let amount = 1000; + + let (token, _) = setup_token(&env); + + // Set current ledger to 100 + let current_ledger = 100; + env.ledger().set_sequence_number(current_ledger); + + // Try to set allowance with already expired ledger (before current) + let expired_ledger = current_ledger - 1; + + token + .mock_all_auths() + .approve(&user1, &user2, &amount, &expired_ledger); +} + +#[test] +#[should_panic(expected = "HostError: Error(Contract, #7)")] // InvalidExpirationLedger +fn allowance_preserves_expiration_when_expired() { + let env = Env::default(); + let user1 = Address::generate(&env); + let user2 = Address::generate(&env); + let amount = 1000; + let (token, _) = setup_token(&env); + + // Set current ledger to 100 + let current_ledger = 100; + env.ledger().set_sequence_number(current_ledger); + + // Set allowance to expire at ledger 200 + let expiration_ledger = current_ledger + 100; + assert_auth!( + user1, + token.approve(&user1, &user2, &amount, &expiration_ledger) + ); + + // Move past expiration + env.ledger().set_sequence_number(expiration_ledger + 1); + + // First check returns 0 + assert_eq!(token.allowance(&user1, &user2), 0); + + // Try to set new allowance with the same expired ledger - should fail + token + .mock_all_auths() + .approve(&user1, &user2, &amount, &expiration_ledger); +} diff --git a/packages/stellar-axelar-std-derive/src/storage.rs b/packages/stellar-axelar-std-derive/src/storage.rs index 8901f9df..0ecd185e 100644 --- a/packages/stellar-axelar-std-derive/src/storage.rs +++ b/packages/stellar-axelar-std-derive/src/storage.rs @@ -96,6 +96,14 @@ impl VariantExt for Variant { } } +struct FnNames { + getter_name: Ident, + setter_name: Ident, + remover_name: Ident, + try_getter_name: Ident, + ttl_extender_name: Ident, +} + impl Value { /// Returns the getter, setter, and remover functions for a storage enum variant. fn storage_fns( @@ -104,35 +112,19 @@ impl Value { storage_type: &StorageType, variant: &Variant, ) -> TokenStream { - let (getter_name, setter_name, remover_name, try_getter_name) = - self.fn_names(&variant.ident); + let fn_names = self.fn_names(&variant.ident); let params = variant.storage_params(); let storage_key = variant.storage_key(enum_name); match self { Self::Status => { - let status_fns = self.status_fns( - storage_type, - &getter_name, - &setter_name, - &remover_name, - ¶ms, - &storage_key, - ); + let status_fns = self.status_fns(storage_type, &fn_names, ¶ms, &storage_key); quote! { #status_fns } } Self::Type(value_type) => { - let value_fns = self.value_fns( - storage_type, - &getter_name, - &setter_name, - &remover_name, - &try_getter_name, - ¶ms, - &storage_key, - value_type, - ); + let value_fns = + self.value_fns(storage_type, &fn_names, ¶ms, &storage_key, value_type); quote! { #value_fns } } @@ -142,21 +134,26 @@ impl Value { fn status_fns( &self, storage_type: &StorageType, - getter_name: &Ident, - setter_name: &Ident, - remover_name: &Ident, + FnNames { + getter_name, + setter_name, + remover_name, + ttl_extender_name, + .. + }: &FnNames, params: &TokenStream, storage_key: &TokenStream, ) -> TokenStream { let storage_method = storage_type.storage_method(); let ttl_fn = storage_type.ttl_fn("e! { key }); + let custom_ttl_fn = storage_type.custom_ttl_fn("e! { key }); quote! { pub fn #getter_name(#params) -> bool { let key = #storage_key; let value = #storage_method.has(&key); - #ttl_fn + #custom_ttl_fn value } @@ -165,29 +162,38 @@ impl Value { let key = #storage_key; #storage_method.set(&key, &()); - #ttl_fn + #custom_ttl_fn } pub fn #remover_name(#params) { let key = #storage_key; #storage_method.remove(&key); } + + pub fn #ttl_extender_name(#params, threshold: u32, extend_to: u32) { + let key = #storage_key; + #ttl_fn + } } } fn value_fns( &self, storage_type: &StorageType, - getter_name: &Ident, - setter_name: &Ident, - remover_name: &Ident, - try_getter_name: &Ident, + FnNames { + getter_name, + setter_name, + remover_name, + try_getter_name, + ttl_extender_name, + }: &FnNames, params: &TokenStream, storage_key: &TokenStream, value_type: &Type, ) -> TokenStream { let storage_method = storage_type.storage_method(); let ttl_fn = storage_type.ttl_fn("e! { key }); + let custom_ttl_fn = storage_type.custom_ttl_fn("e! { key }); quote! { pub fn #getter_name(#params) -> #value_type { @@ -196,7 +202,7 @@ impl Value { .get::<_, #value_type>(&key) .unwrap(); - #ttl_fn + #custom_ttl_fn value } @@ -206,7 +212,7 @@ impl Value { let value = #storage_method.get::<_, #value_type>(&key); if value.is_some() { - #ttl_fn + #custom_ttl_fn } value @@ -216,32 +222,39 @@ impl Value { let key = #storage_key; #storage_method.set(&key, value); - #ttl_fn + #custom_ttl_fn } pub fn #remover_name(#params) { let key = #storage_key; #storage_method.remove(&key); } + + pub fn #ttl_extender_name(#params, threshold: u32, extend_to: u32) { + let key = #storage_key; + #ttl_fn + } } } /// Returns the getter, setter, and remover names for a storage enum variant. - fn fn_names(&self, variant_ident: &Ident) -> (Ident, Ident, Ident, Ident) { + fn fn_names(&self, variant_ident: &Ident) -> FnNames { let ident = variant_ident.to_string().to_snake_case(); match self { - Self::Status => ( - format_ident!("is_{}", ident), - format_ident!("set_{}_status", ident), - format_ident!("remove_{}_status", ident), - format_ident!("_"), - ), - Self::Type(_) => ( - format_ident!("{}", ident), - format_ident!("set_{}", ident), - format_ident!("remove_{}", ident), - format_ident!("try_{}", ident), - ), + Self::Status => FnNames { + getter_name: format_ident!("is_{}", ident), + setter_name: format_ident!("set_{}_status", ident), + remover_name: format_ident!("remove_{}_status", ident), + try_getter_name: format_ident!("_"), + ttl_extender_name: format_ident!("extend_{}_ttl", ident), + }, + Self::Type(_) => FnNames { + getter_name: format_ident!("{}", ident), + setter_name: format_ident!("set_{}", ident), + remover_name: format_ident!("remove_{}", ident), + try_getter_name: format_ident!("try_{}", ident), + ttl_extender_name: format_ident!("extend_{}_ttl", ident), + }, } } } @@ -283,6 +296,18 @@ impl StorageType { } fn ttl_fn(&self, ttl_fn_key: &TokenStream) -> TokenStream { + match self { + Self::Persistent => { + quote! { env.storage().persistent().extend_ttl(&#ttl_fn_key, threshold, extend_to); } + } + Self::Instance => quote! { env.storage().instance().extend_ttl(threshold, extend_to); }, + Self::Temporary => { + quote! { env.storage().temporary().extend_ttl(&#ttl_fn_key, threshold, extend_to); } + } + } + } + + fn custom_ttl_fn(&self, ttl_fn_key: &TokenStream) -> TokenStream { match self { Self::Persistent => { quote! { stellar_axelar_std::ttl::extend_persistent_ttl(env, &#ttl_fn_key); } diff --git a/packages/stellar-axelar-std-derive/src/testdata/storage_schema_generation_succeeds.golden b/packages/stellar-axelar-std-derive/src/testdata/storage_schema_generation_succeeds.golden index 32fa267e..56170f74 100644 --- a/packages/stellar-axelar-std-derive/src/testdata/storage_schema_generation_succeeds.golden +++ b/packages/stellar-axelar-std-derive/src/testdata/storage_schema_generation_succeeds.golden @@ -36,6 +36,11 @@ pub fn remove_counter(env: &soroban_sdk::Env) { env.storage().instance().remove(&key); } +pub fn extend_counter_ttl(env: &soroban_sdk::Env, threshold: u32, extend_to: u32) { + let key = DataKey::Counter; + env.storage().instance().extend_ttl(threshold, extend_to); +} + pub fn message(env: &soroban_sdk::Env, sender: Address) -> String { let key = DataKey::Message(sender); let value = env.storage().persistent().get::<_, String>(&key).unwrap(); @@ -63,6 +68,16 @@ pub fn remove_message(env: &soroban_sdk::Env, sender: Address) { env.storage().persistent().remove(&key); } +pub fn extend_message_ttl( + env: &soroban_sdk::Env, + sender: Address, + threshold: u32, + extend_to: u32, +) { + let key = DataKey::Message(sender); + env.storage().persistent().extend_ttl(&key, threshold, extend_to); +} + pub fn last_caller(env: &soroban_sdk::Env, timestamp: u64) -> Address { let key = DataKey::LastCaller(timestamp); let value = env.storage().temporary().get::<_, Address>(&key).unwrap(); @@ -86,6 +101,16 @@ pub fn remove_last_caller(env: &soroban_sdk::Env, timestamp: u64) { env.storage().temporary().remove(&key); } +pub fn extend_last_caller_ttl( + env: &soroban_sdk::Env, + timestamp: u64, + threshold: u32, + extend_to: u32, +) { + let key = DataKey::LastCaller(timestamp); + env.storage().temporary().extend_ttl(&key, threshold, extend_to); +} + pub fn flag(env: &soroban_sdk::Env, key: String, owner: Address) -> bool { let key = DataKey::Flag(key, owner); let value = env.storage().persistent().get::<_, bool>(&key).unwrap(); @@ -113,6 +138,17 @@ pub fn remove_flag(env: &soroban_sdk::Env, key: String, owner: Address) { env.storage().persistent().remove(&key); } +pub fn extend_flag_ttl( + env: &soroban_sdk::Env, + key: String, + owner: Address, + threshold: u32, + extend_to: u32, +) { + let key = DataKey::Flag(key, owner); + env.storage().persistent().extend_ttl(&key, threshold, extend_to); +} + pub fn optional_message(env: &soroban_sdk::Env, id: u32) -> Option { let key = DataKey::OptionalMessage(id); let value = env.storage().persistent().get::<_, Option>(&key).unwrap(); @@ -140,6 +176,16 @@ pub fn remove_optional_message(env: &soroban_sdk::Env, id: u32) { env.storage().persistent().remove(&key); } +pub fn extend_optional_message_ttl( + env: &soroban_sdk::Env, + id: u32, + threshold: u32, + extend_to: u32, +) { + let key = DataKey::OptionalMessage(id); + env.storage().persistent().extend_ttl(&key, threshold, extend_to); +} + pub fn is_initialized(env: &soroban_sdk::Env) -> bool { let key = DataKey::Initialized; let value = env.storage().instance().has(&key); @@ -158,6 +204,11 @@ pub fn remove_initialized_status(env: &soroban_sdk::Env) { env.storage().instance().remove(&key); } +pub fn extend_initialized_ttl(env: &soroban_sdk::Env, threshold: u32, extend_to: u32) { + let key = DataKey::Initialized; + env.storage().instance().extend_ttl(threshold, extend_to); +} + pub fn is_paused(env: &soroban_sdk::Env) -> bool { let key = DataKey::Paused; let value = env.storage().persistent().has(&key); @@ -176,6 +227,11 @@ pub fn remove_paused_status(env: &soroban_sdk::Env) { env.storage().persistent().remove(&key); } +pub fn extend_paused_ttl(env: &soroban_sdk::Env, threshold: u32, extend_to: u32) { + let key = DataKey::Paused; + env.storage().persistent().extend_ttl(&DataKey::Paused, threshold, extend_to); +} + #[cfg(test)] mod data_key_storage_layout_tests { use goldie;