feat: calendar contract scaffold and governance#63
feat: calendar contract scaffold and governance#63Bosun-Josh121 merged 1 commit intoLightForgeHub:mainfrom
Conversation
Add calendar-contract with initialize, pause/unpause, transfer_admin, upgrade_contract. Storage keys: Admin, VaultAddress, IsPaused. Events for pause and admin transfer. 11 tests covering all paths. Closes LightForgeHub#56
📝 WalkthroughWalkthroughIntroduced a new Soroban smart contract Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
contracts/calendar-contract/src/contract.rs (1)
35-44: Consider validating new admin is different from current admin.
transfer_adminallows transferring to the same address, which would emit a misleading event and waste gas. While not strictly a bug, it's a defensive check worth adding.Proposed validation
pub fn transfer_admin(env: &Env, new_admin: &Address) -> Result<(), CalendarError> { let admin = storage::get_admin(env).ok_or(CalendarError::NotInitialized)?; admin.require_auth(); if storage::is_paused(env) { return Err(CalendarError::ContractPaused); } + if admin == *new_admin { + return Ok(()); // No-op if same admin + } storage::set_admin(env, new_admin); events::admin_transferred(env, &admin, new_admin); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/calendar-contract/src/contract.rs` around lines 35 - 44, In transfer_admin, add a defensive check after retrieving the current admin (storage::get_admin) to ensure new_admin != admin; if equal, return a distinct error (e.g., CalendarError::SameAdmin or CalendarError::AlreadyAdmin) and skip calling storage::set_admin and events::admin_transferred. If that CalendarError variant doesn't exist, add it to the CalendarError enum and use it in transfer_admin to avoid emitting a misleading admin_transferred event and wasting gas.contracts/calendar-contract/src/test.rs (1)
17-105: Consider adding test coverage forupgrade_contractandunpauseedge cases.The test suite covers most scenarios well, but a few gaps exist:
- No test for
upgrade_contract(could at least test auth requirement)- No test for
unpauserequiring auth (similar totest_pause_requires_auth)- No test for calling
unpausebefore initializationThese are non-blocking but would strengthen the test suite.
Example test for unpause auth requirement
#[test] #[should_panic] fn test_unpause_requires_auth() { let env = Env::default(); let contract_id = env.register(CalendarContract, ()); let client = CalendarContractClient::new(&env, &contract_id); let admin = Address::generate(&env); let vault = Address::generate(&env); env.mock_all_auths(); client.init(&admin, &vault); client.pause(); env.mock_auths(&[]); client.unpause(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/calendar-contract/src/test.rs` around lines 17 - 105, Add tests covering upgrade_contract auth and unpause edge cases: write a test that verifies upgrade_contract requires admin auth by calling client.try_upgrade_contract (or client.upgrade_contract with env.auth mocks) with env.mock_all_auths() then env.mock_auths(&[]) and asserting an auth error; add a test that unpause requires auth similar to the provided example using client.unpause (or try_unpause) and env.mock_auths to assert a panic/error when not authorized; and add a test that calling unpause before initialization returns Err(Ok(CalendarError::NotInitialized)) by invoking client.try_unpause (or client.unpause and asserting the expected failure) on a fresh client. Use the existing client methods (upgrade_contract, try_upgrade_contract, unpause, try_unpause) and CalendarError variants (NotInitialized, AlreadyInitialized, ContractPaused) and the env mocking helpers (env.mock_all_auths, env.mock_auths) to implement these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/calendar-contract/src/events.rs`:
- Around line 4-13: Add a new contract_upgraded event function and emit it from
the upgrade path: implement pub fn contract_upgraded(env: &Env, old_code_hash:
&BytesN<32>, new_code_hash: &BytesN<32>) { let topics =
(symbol_short!("contract_upgraded"),); env.events().publish(topics,
(old_code_hash.clone(), new_code_hash.clone())); } (use the same pattern as
contract_paused and admin_transferred) and call contract_upgraded(...) from the
upgrade_contract function in contract.rs at the point where the upgrade is
applied, passing the previous and new code-hash values so the upgrade is
recorded.
- Line 1: Remove the unnecessary crate attribute by deleting the top-level
#![allow(deprecated)] in events.rs; this module contains no deprecated items
(symbol_short! from soroban-sdk v25.3.0 is not deprecated), so simply remove
that attribute line to avoid suppressing warnings incorrectly and keep the file
clean.
In `@contracts/calendar-contract/src/storage.rs`:
- Around line 13-45: Add a helper extend_instance_ttl(env: &Env) that calls
env.storage().instance().extend_ttl(...) and update storage accessors and admin
flows to extend TTL: call extend_instance_ttl() from initialize() after setting
Admin/Vault/IsPaused, and call it in transfer_admin(), pause(), unpause() and
any admin-sensitive paths (i.e., immediately after successful
get_admin()/set_admin(), set_vault_address(), set_paused() operations) so Admin,
VaultAddress and IsPaused instance keys do not expire; ensure references to
has_admin, set_admin, get_admin, set_vault_address, get_vault_address,
is_paused, set_paused are updated to invoke the TTL extension where those state
changes or admin reads occur.
---
Nitpick comments:
In `@contracts/calendar-contract/src/contract.rs`:
- Around line 35-44: In transfer_admin, add a defensive check after retrieving
the current admin (storage::get_admin) to ensure new_admin != admin; if equal,
return a distinct error (e.g., CalendarError::SameAdmin or
CalendarError::AlreadyAdmin) and skip calling storage::set_admin and
events::admin_transferred. If that CalendarError variant doesn't exist, add it
to the CalendarError enum and use it in transfer_admin to avoid emitting a
misleading admin_transferred event and wasting gas.
In `@contracts/calendar-contract/src/test.rs`:
- Around line 17-105: Add tests covering upgrade_contract auth and unpause edge
cases: write a test that verifies upgrade_contract requires admin auth by
calling client.try_upgrade_contract (or client.upgrade_contract with env.auth
mocks) with env.mock_all_auths() then env.mock_auths(&[]) and asserting an auth
error; add a test that unpause requires auth similar to the provided example
using client.unpause (or try_unpause) and env.mock_auths to assert a panic/error
when not authorized; and add a test that calling unpause before initialization
returns Err(Ok(CalendarError::NotInitialized)) by invoking client.try_unpause
(or client.unpause and asserting the expected failure) on a fresh client. Use
the existing client methods (upgrade_contract, try_upgrade_contract, unpause,
try_unpause) and CalendarError variants (NotInitialized, AlreadyInitialized,
ContractPaused) and the env mocking helpers (env.mock_all_auths, env.mock_auths)
to implement these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9b79bd7-2399-4613-8ae6-3b5c0c9feebc
📒 Files selected for processing (8)
contracts/calendar-contract/Cargo.tomlcontracts/calendar-contract/src/contract.rscontracts/calendar-contract/src/error.rscontracts/calendar-contract/src/events.rscontracts/calendar-contract/src/lib.rscontracts/calendar-contract/src/storage.rscontracts/calendar-contract/src/test.rscontracts/calendar-contract/src/types.rs
| @@ -0,0 +1,13 @@ | |||
| #![allow(deprecated)] | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
soroban-sdk symbol_short deprecated alternative 2025
💡 Result:
symbol_short! macro in soroban-sdk is not deprecated. It remains the recommended way to create short Symbol constants (up to 9 characters) at compile time for efficiency. For longer symbols (up to 32 characters), use Symbol::new(&env, "string"). Official docs.rs documentation for latest soroban-sdk v25.3.0 (published 2026-03-07) actively documents and exemplifies symbol_short! usage. Migration guides (v23->v25, v22->v23) list other breaking changes but do not mention deprecation of symbol_short!. Some older Symbol::short method may be deprecated in favor of the macro (per secondary docs), but the macro itself is current best practice. No 2025-specific deprecation found in changelogs, releases, or official Stellar sources.
Citations:
- 1: https://docs.rs/soroban-sdk/latest/soroban_sdk/macro.symbol_short.html
- 2: https://docs.rs/soroban-sdk/latest/soroban_sdk/struct.Symbol.html
- 3: https://docs.rs/soroban-sdk
- 4: https://crates.io/crates/soroban-sdk
- 5: https://docs.rs/crate/soroban-sdk/latest
- 6: https://mintlify.com/stellar/rs-soroban-sdk/migration/overview
- 7: https://docs.rs/crate/soroban-sdk/23.4.0/source/src/_migrating.rs
- 8: https://docs.rs/soroban-sdk/latest/soroban_sdk/
- 9: https://mintlify.com/stellar/rs-soroban-sdk/api/types/symbol
🏁 Script executed:
cat -n contracts/calendar-contract/src/events.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 591
Remove the unnecessary #![allow(deprecated)] attribute.
The symbol_short! macro is not deprecated in soroban-sdk v25.3.0 and remains the recommended way to create short Symbol constants. This module does not contain any deprecated code, so the attribute can be safely removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/calendar-contract/src/events.rs` at line 1, Remove the unnecessary
crate attribute by deleting the top-level #![allow(deprecated)] in events.rs;
this module contains no deprecated items (symbol_short! from soroban-sdk v25.3.0
is not deprecated), so simply remove that attribute line to avoid suppressing
warnings incorrectly and keep the file clean.
| pub fn contract_paused(env: &Env, paused: bool) { | ||
| let topics = (symbol_short!("paused"),); | ||
| env.events().publish(topics, paused); | ||
| } | ||
|
|
||
| pub fn admin_transferred(env: &Env, old_admin: &Address, new_admin: &Address) { | ||
| let topics = (symbol_short!("adm_xfer"),); | ||
| env.events() | ||
| .publish(topics, (old_admin.clone(), new_admin.clone())); | ||
| } |
There was a problem hiding this comment.
Missing contract_upgraded event per issue #56.
The linked issue #56 specifies three events: contract_paused, admin_transferred, and contract_upgraded. Only the first two are implemented here. The upgrade_contract function in contract.rs currently performs the upgrade without emitting an event.
Proposed addition for contract_upgraded event
+pub fn contract_upgraded(env: &Env, new_wasm_hash: &soroban_sdk::BytesN<32>) {
+ let topics = (symbol_short!("upgraded"),);
+ env.events().publish(topics, new_wasm_hash.clone());
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn contract_paused(env: &Env, paused: bool) { | |
| let topics = (symbol_short!("paused"),); | |
| env.events().publish(topics, paused); | |
| } | |
| pub fn admin_transferred(env: &Env, old_admin: &Address, new_admin: &Address) { | |
| let topics = (symbol_short!("adm_xfer"),); | |
| env.events() | |
| .publish(topics, (old_admin.clone(), new_admin.clone())); | |
| } | |
| pub fn contract_paused(env: &Env, paused: bool) { | |
| let topics = (symbol_short!("paused"),); | |
| env.events().publish(topics, paused); | |
| } | |
| pub fn admin_transferred(env: &Env, old_admin: &Address, new_admin: &Address) { | |
| let topics = (symbol_short!("adm_xfer"),); | |
| env.events() | |
| .publish(topics, (old_admin.clone(), new_admin.clone())); | |
| } | |
| pub fn contract_upgraded(env: &Env, new_wasm_hash: &soroban_sdk::BytesN<32>) { | |
| let topics = (symbol_short!("upgraded"),); | |
| env.events().publish(topics, new_wasm_hash.clone()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/calendar-contract/src/events.rs` around lines 4 - 13, Add a new
contract_upgraded event function and emit it from the upgrade path: implement
pub fn contract_upgraded(env: &Env, old_code_hash: &BytesN<32>, new_code_hash:
&BytesN<32>) { let topics = (symbol_short!("contract_upgraded"),);
env.events().publish(topics, (old_code_hash.clone(), new_code_hash.clone())); }
(use the same pattern as contract_paused and admin_transferred) and call
contract_upgraded(...) from the upgrade_contract function in contract.rs at the
point where the upgrade is applied, passing the previous and new code-hash
values so the upgrade is recorded.
| pub fn has_admin(env: &Env) -> bool { | ||
| env.storage().instance().has(&DataKey::Admin) | ||
| } | ||
|
|
||
| pub fn set_admin(env: &Env, admin: &Address) { | ||
| env.storage().instance().set(&DataKey::Admin, admin); | ||
| } | ||
|
|
||
| pub fn get_admin(env: &Env) -> Option<Address> { | ||
| env.storage().instance().get(&DataKey::Admin) | ||
| } | ||
|
|
||
| // --- Vault --- | ||
|
|
||
| pub fn set_vault_address(env: &Env, vault: &Address) { | ||
| env.storage().instance().set(&DataKey::VaultAddress, vault); | ||
| } | ||
|
|
||
| pub fn get_vault_address(env: &Env) -> Option<Address> { | ||
| env.storage().instance().get(&DataKey::VaultAddress) | ||
| } | ||
|
|
||
| // --- Pause --- | ||
|
|
||
| pub fn is_paused(env: &Env) -> bool { | ||
| env.storage() | ||
| .instance() | ||
| .get(&DataKey::IsPaused) | ||
| .unwrap_or(false) | ||
| } | ||
|
|
||
| pub fn set_paused(env: &Env, paused: bool) { | ||
| env.storage().instance().set(&DataKey::IsPaused, &paused); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
soroban instance storage extend_ttl best practices 2025
💡 Result:
Best practices for extending TTL on Soroban instance storage, based on official Rust SDK documentation: Instance storage is for small, contract-level data like admin addresses, config, metadata (keep under ~100KB, ideally <10KB). It shares TTL with the contract instance and code; expiration makes the entire contract inaccessible. Use env.storage.instance.extend_ttl(threshold, extend_to): - threshold: Extend only if current TTL < this (in ledgers, e.g., 100_000 ~5.5 days, 518_400 ~30 days) - extend_to: New TTL (e.g., 5_250_000 ~1 year). Do not exceed env.storage.max_ttl. Key practices: 1. Extend regularly: On initialization, admin ops, every interaction for critical contracts. 2. Use threshold to avoid redundant extensions. 3. Guidelines: Short-lived: 100k ledgers; Medium: 1M; Long-term/critical: 5.25M+. 4. Example: pub fn bump_instance(env: Env) { env.storage.instance.extend_ttl(518_400, 5_250_000); } Extend proactively to avoid restoration costs. Unlike persistent/temp, no key param—extends whole instance/code. No 2025-specific changes noted; practices from current SDK (2026-relevant). Official Stellar docs pages were 404, but SDK sources authoritative.
Citations:
- 1: https://mintlify.com/stellar/rs-soroban-sdk/api/storage/instance
- 2: https://www.mintlify.com/stellar/rs-soroban-sdk/core/storage
- 3: https://mintlify.com/stellar/rs-soroban-sdk/api/storage/overview
- 4: https://docs.rs/soroban-sdk/latest/soroban_sdk/storage/struct.Storage.html
- 5: https://docs.rs/soroban-sdk/latest/soroban_sdk/storage/struct.Instance.html
🏁 Script executed:
# First, find and read the full storage.rs file to see complete context
find . -name "storage.rs" -path "*/calendar-contract/*" | head -5Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 123
🏁 Script executed:
# Search for extend_ttl calls in the entire contract codebase
rg "extend_ttl" --type rustRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 411
🏁 Script executed:
# Check contract initialization and admin operations
rg -A 5 "pub fn set_admin|pub fn get_admin|pub fn new|fn init" --type rust contracts/Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 7125
🏁 Script executed:
# Read the full calendar-contract storage.rs to see complete context
cat -n contracts/calendar-contract/src/storage.rsRepository: LightForgeHub/SkillSphere-Contracts
Length of output: 1389
🏁 Script executed:
# Check how identity-registry-contract implements TTL extension
rg -B 5 -A 10 "extend_ttl" contracts/identity-registry-contract/src/storage.rs | head -40Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 1298
🏁 Script executed:
# Check calendar-contract initialization function for TTL calls
cat -n contracts/calendar-contract/src/contract.rs | head -100Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 2030
🏁 Script executed:
# Check if other calendar-contract contracts have extend_ttl for instance storage
cat -n contracts/payment-vault-contract/src/storage.rs | head -80Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 3217
🏁 Script executed:
# Check if reputation-scoring-contract has extend_ttl for instance storage
cat -n contracts/reputation-scoring-contract/src/storage.rs | head -80Repository: LightForgeHub/SkillSphere-Contracts
Length of output: 1148
Instance storage TTL extension is missing and critical for contract longevity.
Soroban instance storage entries expire after their TTL if not extended. Without calling extend_ttl(), the contract's admin address, vault address, and pause state will become inaccessible when the instance storage TTL expires, rendering the entire contract unusable. Extending TTL on initialization and during admin operations is a required Soroban best practice.
Add an extend_instance_ttl() function and call it during initialization and in critical admin paths (initialize, transfer_admin, pause, unpause):
Proposed TTL extension pattern
+const INSTANCE_LIFETIME_THRESHOLD: u32 = 17280; // ~1 day
+const INSTANCE_BUMP_AMOUNT: u32 = 518400; // ~30 days
+
+pub fn extend_instance_ttl(env: &Env) {
+ env.storage()
+ .instance()
+ .extend_ttl(INSTANCE_LIFETIME_THRESHOLD, INSTANCE_BUMP_AMOUNT);
+}Then call extend_instance_ttl(env) in initialize() and after get_admin() reads in contract operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/calendar-contract/src/storage.rs` around lines 13 - 45, Add a
helper extend_instance_ttl(env: &Env) that calls
env.storage().instance().extend_ttl(...) and update storage accessors and admin
flows to extend TTL: call extend_instance_ttl() from initialize() after setting
Admin/Vault/IsPaused, and call it in transfer_admin(), pause(), unpause() and
any admin-sensitive paths (i.e., immediately after successful
get_admin()/set_admin(), set_vault_address(), set_paused() operations) so Admin,
VaultAddress and IsPaused instance keys do not expire; ensure references to
has_admin, set_admin, get_admin, set_vault_address, get_vault_address,
is_paused, set_paused are updated to invoke the TTL extension where those state
changes or admin reads occur.
3 participants
Summary
Closes #56
Test plan
Summary by CodeRabbit
Release Notes
New Features
Tests