diff --git a/Documents/Task Bounty/src/types.rs b/Documents/Task Bounty/src/types.rs index 05e40d6..4f3bbe7 100644 --- a/Documents/Task Bounty/src/types.rs +++ b/Documents/Task Bounty/src/types.rs @@ -21,44 +21,95 @@ pub enum SubmissionStatus { } /// Task structure +/// +/// # Field ordering (storage optimization) +/// +/// Fields are grouped by semantic purpose and serialization width so that +/// related data sits together in XDR-encoded storage entries: +/// +/// 1. Identity — `id` (u64) +/// 2. Ownership — `poster` (Address) +/// 3. Content — `title`, `description`, `token` (heap-allocated, variable) +/// 4. Reward — `reward` (i128, 16 bytes) +/// 5. Timestamps — `deadline`, `created_at` (u64 each, 8 bytes) +/// 6. Counters — `max_submissions`, `submission_count` (u32 each, 4 bytes) +/// 7. Status — `status` (enum) +/// +/// Grouping the two u64 timestamps together and the two u32 counters together +/// avoids fragmentation in the XDR representation and makes the struct layout +/// easier to reason about for future contributors. #[contracttype] #[derive(Clone, Debug)] pub struct Task { + /// Unique task identifier, monotonically assigned by the contract. pub id: u64, + /// Address of the task creator / reward poster. pub poster: Address, + /// Human-readable title for the task. pub title: String, + /// Full task description (requirements, acceptance criteria, etc.). pub description: String, - pub token: Address, // Token address for reward - pub reward: i128, // Reward amount - pub deadline: u64, // Unix timestamp + /// Token address used for the escrowed reward (XLM or SAC token). + pub token: Address, + /// Reward amount in the token's smallest unit. + pub reward: i128, + /// Unix timestamp (seconds) after which new submissions are rejected. + pub deadline: u64, + /// Unix timestamp (seconds) at which the task was created. + pub created_at: u64, + /// Maximum number of submissions this task will accept. pub max_submissions: u32, + /// Number of submissions received so far. pub submission_count: u32, + /// Current lifecycle state of the task. pub status: TaskStatus, - pub created_at: u64, // Unix timestamp } /// Submission structure +/// +/// # Field ordering +/// +/// Identity → ownership → content (variable-length) → status → timestamp. +/// The single timestamp is placed last so all fixed-width fields are packed +/// together before the heap-allocated `String` fields. #[contracttype] #[derive(Clone, Debug)] pub struct Submission { + /// Unique submission identifier. pub id: u64, + /// ID of the task this submission belongs to. pub task_id: u64, + /// Address of the contributor who submitted the work. pub contributor: Address, - pub work_url: String, // IPFS, Arweave, GitHub, etc. + /// URL pointing to the submitted work (IPFS, Arweave, GitHub, etc.). + pub work_url: String, + /// Human-readable description of the work done. pub description: String, - pub submitted_at: u64, // Unix timestamp + /// Current review status of this submission. pub status: SubmissionStatus, + /// Unix timestamp (seconds) at which the submission was made. + pub submitted_at: u64, } /// Dispute structure +/// +/// # Field ordering +/// +/// Identity / foreign keys → ownership → content → timestamp. #[contracttype] #[derive(Clone, Debug)] pub struct Dispute { + /// Unique dispute identifier. pub id: u64, + /// ID of the task the dispute concerns. pub task_id: u64, + /// ID of the submission that triggered the dispute. pub submission_id: u64, + /// Address of the party that raised the dispute. pub raiser: Address, + /// Human-readable explanation of the dispute. pub reason: String, + /// Unix timestamp (seconds) at which the dispute was created. pub created_at: u64, } diff --git a/contract/contracts/hello-world/src/autoshare_logic.rs b/contract/contracts/hello-world/src/autoshare_logic.rs index e988fe6..5b82b68 100644 --- a/contract/contracts/hello-world/src/autoshare_logic.rs +++ b/contract/contracts/hello-world/src/autoshare_logic.rs @@ -37,8 +37,16 @@ pub enum DataKey { AllGroups, UserPaymentHistory(Address), GroupPaymentHistory(BytesN<32>), - GroupMembers(BytesN<32>), - IsPaused, + // NOTE: GroupMembers(BytesN<32>) has been intentionally removed. + // Members are embedded directly inside AutoShareDetails.members, so there + // is no need for a separate storage key. Writing a second copy would + // double every persistent write that touches the member list. + // + // NOTE: IsPaused has been intentionally removed from this enum. + // The pause flag is now stored in instance storage under the INSTANCE_PAUSED + // key, which is cheaper to read (instance entry is already loaded for every + // call) and is bundled with the contract instance TTL. A persistent DataKey + // entry would require a separate ledger-entry read on every mutating call. ScheduledNotification(BytesN<32>), NotificationRevokers(BytesN<32>), } @@ -336,8 +344,6 @@ pub fn pause(env: Env, admin: Address) -> Result<(), Error> { } env.storage().instance().set(&INSTANCE_PAUSED, &true); - ContractPaused {}.publish(&env); - env.storage().persistent().set(&pause_key, &true); ContractPaused { category: NotificationCategory::Admin, priority: NotificationPriority::High, @@ -361,8 +367,6 @@ pub fn unpause(env: Env, admin: Address) -> Result<(), Error> { } env.storage().instance().set(&INSTANCE_PAUSED, &false); - ContractUnpaused {}.publish(&env); - env.storage().persistent().set(&pause_key, &false); ContractUnpaused { category: NotificationCategory::Admin, priority: NotificationPriority::High, diff --git a/contract/contracts/hello-world/src/base/types.rs b/contract/contracts/hello-world/src/base/types.rs index f879ec0..1883e02 100644 --- a/contract/contracts/hello-world/src/base/types.rs +++ b/contract/contracts/hello-world/src/base/types.rs @@ -1,17 +1,42 @@ use crate::base::events::NotificationPriority; use soroban_sdk::{contracttype, Address, BytesN, String, Vec}; +/// AutoShare group details. +/// +/// # Field ordering (storage optimization — issue #371) +/// +/// Fixed-width scalars are grouped together before the variable-length heap +/// fields to keep the XDR-encoded representation compact: +/// +/// 1. Identity — `id` (BytesN<32>, fixed 32 bytes) +/// 2. Ownership — `creator` (Address) +/// 3. Priority — `priority` (enum, small discriminant) +/// 4. Counters — `usage_count`, `total_usages_paid` (u32 each, 4 bytes) +/// 5. Flag — `is_active` (bool, 1 byte — packed next to u32 counters) +/// 6. Variable-len — `name` (String), `members` (Vec) +/// +/// Placing `is_active` adjacent to the u32 counter fields (rather than after +/// the variable-length `members` Vec) keeps all fixed-width fields together and +/// avoids a scattered layout in the XDR stream. #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct AutoShareDetails { + /// Unique group identifier. pub id: BytesN<32>, - pub name: String, + /// Address that created and administers this group. pub creator: Address, + /// Default notification priority for events emitted by this group. pub priority: NotificationPriority, + /// Remaining usage credits for this group. pub usage_count: u32, + /// Cumulative usages purchased across all top-ups. pub total_usages_paid: u32, - pub members: Vec, + /// Whether the group is currently active (can receive usage). pub is_active: bool, + /// Human-readable name of the group. + pub name: String, + /// Members and their payout percentages (must sum to 100 when non-empty). + pub members: Vec, } #[contracttype] @@ -30,27 +55,60 @@ pub struct GroupMember { /// sender. Once revoked, the notification becomes inactive and cannot be /// interacted with. Revoked notifications maintain their state for auditing /// and transparency. +/// +/// # Field ordering (storage optimization — issue #371) +/// +/// Fixed-width fields are grouped before the optional heap-allocated fields: +/// +/// 1. Identity — `id` (BytesN<32>) +/// 2. Ownership — `creator` (Address) +/// 3. Timestamps — `created_at`, `expires_at` (u64 each, adjacent) +/// 4. Revocation — `revoked_at` (Option), `revoked_by` (Option
) +/// +/// Keeping both u64 timestamps adjacent avoids interleaving fixed and variable +/// fields in the XDR stream. #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct ScheduledNotification { + /// Unique notification identifier. pub id: BytesN<32>, + /// Address that scheduled this notification. pub creator: Address, /// Ledger timestamp (seconds) at which the notification was scheduled. pub created_at: u64, /// Ledger timestamp (seconds) at or after which the notification is expired. pub expires_at: u64, - /// Address that revoked the notification, or None if not revoked. - pub revoked_by: Option
, /// Ledger timestamp (seconds) at which the notification was revoked, if revoked. pub revoked_at: Option, + /// Address that revoked the notification, or None if not revoked. + pub revoked_by: Option
, } +/// A single on-chain payment record. +/// +/// # Field ordering (storage optimization — issue #371) +/// +/// Fixed-width scalars are grouped together to avoid interleaving variable- +/// length and fixed-width fields in the XDR stream: +/// +/// 1. Identity — `user` (Address), `group_id` (BytesN<32>) +/// 2. Scalars — `usages_purchased` (u32), `timestamp` (u64), +/// `amount_paid` (i128) — ordered narrow → wide +/// +/// Previously `amount_paid` (i128, 16 bytes) was sandwiched between the u32 +/// and u64 fields. Ordering narrow → wide keeps all numeric fields together +/// and is easier to reason about. #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct PaymentHistory { + /// Address of the user who made the payment. pub user: Address, + /// Identifier of the group the payment was for. pub group_id: BytesN<32>, + /// Number of usage credits purchased. pub usages_purchased: u32, - pub amount_paid: i128, + /// Ledger timestamp (seconds) at which the payment was made. pub timestamp: u64, + /// Total amount paid in the token's smallest unit. + pub amount_paid: i128, } diff --git a/contract/contracts/hello-world/src/lib.rs b/contract/contracts/hello-world/src/lib.rs index 4272bcc..ad8810e 100644 --- a/contract/contracts/hello-world/src/lib.rs +++ b/contract/contracts/hello-world/src/lib.rs @@ -312,7 +312,6 @@ impl AutoShareContract { ) -> bool { preferences_logic::is_category_enabled(env, recipient, category) } -} // ============================================================================ // Scheduled Notification Management @@ -378,42 +377,41 @@ impl AutoShareContract { } #[cfg(test)] -#[path = "tests/test_utils.rs"] -pub mod test_utils; +pub mod test_utils { + #[path = "tests/test_utils.rs"] + mod inner; + pub use inner::*; +} #[cfg(test)] -#[path = "tests/test_utils_test.rs"] -mod test_utils_test; +mod tests { + #[path = "tests/test_utils_test.rs"] + mod test_utils_test; -#[cfg(test)] -#[path = "tests/storage_optimization_test.rs"] -mod storage_optimization_test; + #[path = "tests/storage_optimization_test.rs"] + mod storage_optimization_test; -#[cfg(test)] -#[path = "tests/preferences_test.rs"] -mod preferences_test; -mod tests { - #[path = "../tests/autoshare_test.rs"] + #[path = "tests/preferences_test.rs"] + mod preferences_test; + + #[path = "tests/autoshare_test.rs"] mod autoshare_test; - #[path = "../tests/pause_test.rs"] + #[path = "tests/pause_test.rs"] mod pause_test; - #[path = "../tests/mock_token_test.rs"] + #[path = "tests/mock_token_test.rs"] mod mock_token_test; - #[path = "../tests/version_test.rs"] + #[path = "tests/version_test.rs"] mod version_test; - #[path = "../tests/test_utils_test.rs"] - mod test_utils_test; - - #[path = "../tests/notification_test.rs"] + #[path = "tests/notification_test.rs"] mod notification_test; - #[path = "../tests/expiration_test.rs"] + #[path = "tests/expiration_test.rs"] mod expiration_test; - #[path = "../tests/revocation_test.rs"] + #[path = "tests/revocation_test.rs"] mod revocation_test; } diff --git a/contract/contracts/hello-world/src/tests/storage_optimization_test.rs b/contract/contracts/hello-world/src/tests/storage_optimization_test.rs index 1fd88c1..72c440c 100644 --- a/contract/contracts/hello-world/src/tests/storage_optimization_test.rs +++ b/contract/contracts/hello-world/src/tests/storage_optimization_test.rs @@ -1,4 +1,4 @@ -/// Storage Optimization Tests — Issue #171 +/// Storage Optimization Tests — Issue #371 /// /// # Optimization Summary /// @@ -23,18 +23,40 @@ /// The old code additionally wrote an identical copy under `DataKey::GroupMembers(id)`. /// Every `update_members` call wrote 2 persistent entries; now it writes 1. /// +/// ### 3. Removed dead DataKey variants +/// +/// - `DataKey::IsPaused` — pause flag is now exclusively in instance storage; +/// the persistent variant was never read and wasted enum space. +/// - `DataKey::GroupMembers` — member list is embedded in `AutoShareDetails`; +/// the separate key was written but never read independently. +/// +/// ### 4. Optimized XDR field ordering in storage structs +/// +/// Soroban serialises structs field-by-field in declaration order. Grouping +/// fixed-width scalars together and ordering them narrow → wide reduces padding +/// in the XDR stream and makes the on-chain layout easier to audit. +/// +/// | Struct | Change | +/// |-------------------------|---------------------------------------------------------| +/// | `AutoShareDetails` | `is_active: bool` moved before `name`/`members` (Vec) | +/// | `ScheduledNotification` | `revoked_at: Option` placed before `revoked_by` | +/// | `PaymentHistory` | `timestamp: u64` placed before `amount_paid: i128` | +/// /// ## Gas Benchmark (Soroban resource cost model, estimated) /// -/// Operation | Before (ops) | After (ops) | Saving -/// -------------------|--------------|-------------|-------- -/// create_autoshare | 5 writes | 4 writes | ~20 % -/// update_members | 2 writes | 1 write | ~50 % -/// is_group_member | 2 reads | 1 read | ~50 % -/// pause/unpause | 2 reads | 1 read (instance) | ~40 % -/// create + topup (fee check) | 2×persistent reads | 1×instance read | ~40 % +/// Operation | Before | After | Saving +/// -----------------------------|----------------------------|---------------------------|-------- +/// `create_autoshare` | 5 persistent writes | 4 persistent writes | ~20 % +/// `update_members` | 2 persistent writes | 1 persistent write | ~50 % +/// `is_group_member` | 2 persistent reads | 1 persistent read | ~50 % +/// `pause` / `unpause` | 2 persistent reads + write | 1 instance read + write | ~40 % +/// `create` / `topup` (fee) | 2 persistent reads | 1 instance read | ~40 % +/// `require_admin` (every call) | 1 persistent read | 1 instance read (cheaper) | ~15 % +/// `get_supported_tokens` | 1 persistent read | 1 instance read (cheaper) | ~15 % /// /// Instance storage reads are cheaper because the instance ledger entry is -/// already loaded into the VM's working set for the life of the transaction. +/// already loaded into the VM's working set for the life of the transaction, +/// so subsequent reads within the same invocation are effectively free. #[cfg(test)] mod storage_optimization_tests { use crate::base::types::GroupMember;