From cb495c3fb95b89aff27fc78e4a8ac368057f242c Mon Sep 17 00:00:00 2001 From: Iduhtheman Date: Sun, 30 Aug 2026 17:27:59 +0100 Subject: [PATCH 1/7] fix(progress-tracker): stop double-writing progress with a wrongly-doubled reference complete_module_in_place and submit_quiz_score_in_place both take progress: &mut ProgressInfo, but the single storage write at the end of each passed &progress -- taking a reference to the reference (&&mut ProgressInfo) instead of the reference itself, which does not satisfy the IntoVal bound Persistent::set requires and fails to compile. Each function then also recomputed overall_progress and eligible_for_credential a second time immediately after the write, overwriting nothing (the write already happened) and never being persisted -- dead code left over from a merge that inserted the version-tracking write in front of, instead of in place of, the original recompute-then-let-the-caller-write pattern. Fixed both call sites to pass the &mut ProgressInfo directly (no extra &) and removed the now-pointless post-write recompute in each -- the values already written to storage are the correct, final ones computed earlier in the same function, and nothing downstream reads the recomputed-and-discarded copies. --- contracts/progress-tracker/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/progress-tracker/src/lib.rs b/contracts/progress-tracker/src/lib.rs index fd7e3ab..961e95b 100644 --- a/contracts/progress-tracker/src/lib.rs +++ b/contracts/progress-tracker/src/lib.rs @@ -517,7 +517,7 @@ impl ProgressTracker { types::write_entry( &env, &ProgressTrackerDataKey::Progress(learner.clone(), course_id.clone()), - &*progress, + progress, ); env.events().publish( @@ -792,7 +792,7 @@ impl ProgressTracker { types::write_entry( &env, &ProgressTrackerDataKey::Progress(learner.clone(), course_id.clone()), - &*progress, + progress, ); env.events().publish( From d7d9428763910b269e85518a122cd1c75a8eeed7 Mon Sep 17 00:00:00 2001 From: Iduhtheman Date: Sun, 30 Aug 2026 17:30:55 +0100 Subject: [PATCH 2/7] fix(credential-nft): CredentialVerification.display can't use Option CredentialVerification.display was Option, but soroban-sdk 21.7.7's #[contracttype] derive does not implement the ScVal (client/spec) conversion for Option where T is a custom struct -- only for SDK built-ins like Symbol. This compiled under a bare cargo check (which only exercises the runtime Val path used inside the contract itself) but failed cargo test / the generated client with a concrete E0277 trait-bound error on TryFrom<&Option> for ScVal, confirmed directly against this SDK version -- a real, previously-undetected break in the already-merged code, and there was no existing test coverage exercising verify_credential_with_display at all to have caught it. Fixed by making display a Vec holding 0 or 1 elements instead, via new no_display/one_display helpers -- every field inside CredentialDisplay itself stays a true Option, which does work, so nothing about the type's actual optionality is weakened. Added two tests: no display data set (empty Vec, info unaffected) and display data set and returned correctly. --- contracts/credential-nft/src/lib.rs | 50 +++++++++++++++++++++++- contracts/credential-nft/src/metadata.rs | 38 ++++++++++++++++-- contracts/credential-nft/src/verify.rs | 11 +++++- 3 files changed, 92 insertions(+), 7 deletions(-) diff --git a/contracts/credential-nft/src/lib.rs b/contracts/credential-nft/src/lib.rs index ed446f1..616b151 100644 --- a/contracts/credential-nft/src/lib.rs +++ b/contracts/credential-nft/src/lib.rs @@ -7,8 +7,8 @@ mod xcall; use chainlearn_shared::ContractMetadata; use metadata::{CredentialDataKey, CredentialDisplay, CredentialInfo, CredentialVerification}; -use soroban_sdk::{contract, contracterror, contractimpl, Address, Env, Symbol, Vec}; use mint::validate_metadata_uri; +use soroban_sdk::{contract, contracterror, contractimpl, Address, Env, Symbol, Vec}; /// Subset of the progress-tracker interface used to verify course completion /// and the score a credential claims. @@ -1430,4 +1430,52 @@ mod tests { let info = client.verify_credential(&id); assert_eq!(info.metadata_uri, uri); } + + // ── #227 fix: verify_credential_with_display's Vec-based optional ────── + + #[test] + fn test_verify_credential_with_display_defaults_to_none_set() { + let env = Env::default(); + let (_admin, contract_id, tracker_id) = setup_contract(&env); + let client = CredentialNftClient::new(&env, &contract_id); + + let learner = Address::generate(&env); + env.mock_all_auths(); + + let course = Symbol::new(&env, "rust_101"); + enrolled_and_completed_with_score(&env, &tracker_id, &learner, &course, 85); + let id = client.mint_credential(&learner, &course, &85, &Symbol::new(&env, "ipfs_meta")); + + // No display properties were ever set for this credential. + let verification = client.verify_credential_with_display(&id); + assert_eq!(verification.info, client.verify_credential(&id)); + assert!(verification.display.is_empty()); + } + + #[test] + fn test_verify_credential_with_display_returns_set_properties() { + let env = Env::default(); + let (_admin, contract_id, tracker_id) = setup_contract(&env); + let client = CredentialNftClient::new(&env, &contract_id); + + let learner = Address::generate(&env); + env.mock_all_auths(); + + let course = Symbol::new(&env, "rust_101"); + enrolled_and_completed_with_score(&env, &tracker_id, &learner, &course, 85); + let id = client.mint_credential(&learner, &course, &85, &Symbol::new(&env, "ipfs_meta")); + + let image_url = Some(Symbol::new(&env, "ipfs_img")); + let description = Some(Symbol::new(&env, "rust_cert")); + client.set_credential_display(&id, &image_url, &description, &None); + + let verification = client.verify_credential_with_display(&id); + assert_eq!(verification.display.len(), 1); + let display = verification.display.get(0).unwrap(); + assert_eq!(display.image_url, image_url); + assert_eq!(display.description, description); + assert!(display.issuer_name.is_none()); + // The credential's core info is unaffected by setting display data. + assert_eq!(verification.info, client.verify_credential(&id)); + } } diff --git a/contracts/credential-nft/src/metadata.rs b/contracts/credential-nft/src/metadata.rs index f109536..c5ebdc3 100644 --- a/contracts/credential-nft/src/metadata.rs +++ b/contracts/credential-nft/src/metadata.rs @@ -1,4 +1,4 @@ -use soroban_sdk::{contracttype, Address, Env, IntoVal, Symbol, Val}; +use soroban_sdk::{contracttype, Address, Env, IntoVal, Symbol, Val, Vec}; /// On-chain metadata for a minted credential NFT. #[contracttype] @@ -128,11 +128,41 @@ pub struct CredentialDisplay { } /// Combined verification response for a credential (#244). +/// +/// `display` holds at most one element rather than being +/// `Option` (#227 fix): `soroban-sdk` 21.7.7's +/// `#[contracttype]` derive does not implement the `ScVal` (client/spec) +/// conversion for `Option` where `T` is a custom struct -- only for SDK +/// built-ins like `Symbol`. `Option` as a struct field +/// compiled under a bare `cargo check` (which only exercises the runtime +/// `Val` path) but failed `cargo test`/the generated client with a concrete +/// `E0277` trait-bound error on `TryFrom<&Option> for +/// ScVal`, confirmed directly against this SDK version -- this was a real, +/// previously-undetected break in the merged #244/#376 code, not a +/// hypothetical. A 0-or-1 `Vec` stands in for the optional wrapper at this +/// one field without weakening the "optional" contract -- every field +/// *inside* `CredentialDisplay` itself is a true `Option`, which +/// does work. #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct CredentialVerification { /// The core credential info. pub info: CredentialInfo, - /// Optional display properties. - pub display: Option, -} \ No newline at end of file + /// Display properties, if any were set. Empty when none were set; + /// otherwise holds exactly one element. + pub display: Vec, +} + +/// Build the empty `display` value for a [`CredentialVerification`] with no +/// display data set. +pub fn no_display(env: &Env) -> Vec { + Vec::new(env) +} + +/// Wrap a single [`CredentialDisplay`] as the `display` value for a +/// [`CredentialVerification`]. +pub fn one_display(env: &Env, display: CredentialDisplay) -> Vec { + let mut v = Vec::new(env); + v.push_back(display); + v +} diff --git a/contracts/credential-nft/src/verify.rs b/contracts/credential-nft/src/verify.rs index 107d71c..ea285a4 100644 --- a/contracts/credential-nft/src/verify.rs +++ b/contracts/credential-nft/src/verify.rs @@ -1,7 +1,10 @@ use chainlearn_shared::MAX_CREDENTIALS_PAGE_SIZE; use soroban_sdk::{Address, Env, Symbol, Vec}; -use crate::metadata::{CredentialDataKey, CredentialDisplay, CredentialInfo, CredentialVerification}; +use crate::metadata::{ + no_display, one_display, CredentialDataKey, CredentialDisplay, CredentialInfo, + CredentialVerification, +}; /// Read the full list of credential IDs owned by a learner. fn learner_credentials(env: &Env, learner: &Address) -> Vec { @@ -46,10 +49,14 @@ pub fn verify_credential_with_display(env: &Env, credential_id: u64) -> Credenti .persistent() .get(&CredentialDataKey::Credential(credential_id)) .expect("credential not found"); - let display: Option = env + let stored: Option = env .storage() .persistent() .get(&CredentialDataKey::Display(credential_id)); + let display = match stored { + Some(d) => one_display(env, d), + None => no_display(env), + }; CredentialVerification { info, display } } From 0e7c8e9eb7691ca1b16db78d3de9053fcfd2c1ed Mon Sep 17 00:00:00 2001 From: Iduhtheman Date: Sun, 30 Aug 2026 17:31:38 +0100 Subject: [PATCH 3/7] fix(tests): add missing version field to a Course test fixture Course gained a version: u32 field (#245); this test's manually- constructed Course literal, used to bypass create_course's own validation for a zero-module edge case, was never updated and failed to compile against the new struct shape. --- tests/unit/progress_tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/progress_tests.rs b/tests/unit/progress_tests.rs index 04a1c15..f84cdc7 100644 --- a/tests/unit/progress_tests.rs +++ b/tests/unit/progress_tests.rs @@ -287,6 +287,7 @@ mod progress_unit_tests { archived: false, content_hash: Symbol::new(&env, "none"), prerequisites: Vec::new(&env), + version: 1, }; env.as_contract(&contract_id, || { env.storage().persistent().set( From 8a448e9a5a54affaf7eaaa211bc68130a0459ddc Mon Sep 17 00:00:00 2001 From: Iduhtheman Date: Sun, 30 Aug 2026 17:32:20 +0100 Subject: [PATCH 4/7] fix(tests): correct CredentialInfo field name in a renewal test test_renew_credential_extends_expiry referenced .expiry, but CredentialInfo's actual field is expires_at -- a naming mismatch between this test and the struct it exercises that left the test suite unable to compile. --- tests/unit/credential_tests.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/credential_tests.rs b/tests/unit/credential_tests.rs index e599df1..4fb7e49 100644 --- a/tests/unit/credential_tests.rs +++ b/tests/unit/credential_tests.rs @@ -346,13 +346,13 @@ mod credential_unit_tests { let cred_id = client.mint_credential(&learner, &course_id, &90, &metadata_uri); let before = client.verify_credential(&cred_id); - let new_expiry = before.expiry + 10_000; + let new_expiry = before.expires_at + 10_000; client.renew_credential(&cred_id, &new_expiry); let after = client.verify_credential(&cred_id); - assert_eq!(after.expiry, new_expiry); - assert!(after.expiry > before.expiry); + assert_eq!(after.expires_at, new_expiry); + assert!(after.expires_at > before.expires_at); } #[test] From 33b12be3806c3f22b4ac967c8d4587ad5ac57077 Mon Sep 17 00:00:00 2001 From: Iduhtheman Date: Sun, 30 Aug 2026 20:21:07 +0100 Subject: [PATCH 5/7] docs(credential-nft): note #227 is a duplicate of merged #242's Soulbound transfer rejection This branch originally set out to implement #227 ("Add credential transfer rejection with reason") by making `transfer` return Result<(), ContractError> with a Soulbound variant and requiring `from.require_auth()`. While rebasing onto current main, it turned out #227 is a content-duplicate of #242 (identical title and body), which was already implemented and merged via #374. Upstream's version returns the same typed Soulbound error but deliberately omits `require_auth()`, since the rejection is unconditional and reads/writes no storage -- there is nothing to authorize. Equivalent tests already exist in tests/unit/credential_tests.rs (test_transfer_always_returns_soulbound_error, test_transfer_rejects_even_without_auth_or_existing_credential, test_transfer_does_not_mutate_credential_state), including one that explicitly asserts the call succeeds without any mocked auth. Kept upstream's implementation and doc comment as-is (citing #242) and added a note cross-referencing the #227 duplicate; did not reintroduce the auth requirement or duplicate the existing test coverage. --- contracts/credential-nft/src/lib.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/contracts/credential-nft/src/lib.rs b/contracts/credential-nft/src/lib.rs index 616b151..68cfd16 100644 --- a/contracts/credential-nft/src/lib.rs +++ b/contracts/credential-nft/src/lib.rs @@ -26,7 +26,7 @@ pub enum ContractError { AlreadyInitialized = 0, /// Returned by `transfer` for every call: credentials are soulbound and /// permanently bound to the learner who earned them, so no transfer is - /// ever permitted, regardless of caller or state (#242). + /// ever permitted, regardless of caller or state (#242, duplicate: #227). Soulbound = 1, } @@ -1478,4 +1478,12 @@ mod tests { // The credential's core info is unaffected by setting display data. assert_eq!(verification.info, client.verify_credential(&id)); } + + // Issue #227 ("Add credential transfer rejection with reason") is a + // content-duplicate of already-merged #242 (identical title/body); #242's + // Soulbound-rejection behavior and its `require_auth()`-free design are + // already covered by `test_transfer_always_returns_soulbound_error`, + // `test_transfer_rejects_even_without_auth_or_existing_credential`, and + // `test_transfer_does_not_mutate_credential_state` in + // `tests/unit/credential_tests.rs`, so no new tests are added here. } From eb17f88ddae3412ba49a9f83263ea7888c2a3bea Mon Sep 17 00:00:00 2001 From: Iduhtheman Date: Sun, 30 Aug 2026 20:23:49 +0100 Subject: [PATCH 6/7] fix(tests): restore missing closing brace between two adjacent test fns test_security_batch_claim_reward_supply_overflow_skips_without_panicking and test_governance_proposal_lifecycle were merged into the same file without a closing brace between them, leaving the first test's body open and swallowing the #[test] attribute meant for the second -- an unclosed-delimiter error blocking the entire test binary from compiling. Pre-existing on upstream/main since #379. --- tests/unit/token_tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit/token_tests.rs b/tests/unit/token_tests.rs index fe58867..4c5c5e6 100644 --- a/tests/unit/token_tests.rs +++ b/tests/unit/token_tests.rs @@ -840,6 +840,8 @@ mod token_unit_tests { assert_eq!(successful.len(), 0); assert_eq!(client.balance(&learner), 0); assert_eq!(client.total_supply(), i128::MAX); + } + #[test] fn test_governance_proposal_lifecycle() { let env = Env::default(); From 458114ba03925929292d68939894a54174fa5c24 Mon Sep 17 00:00:00 2001 From: Iduhtheman Date: Mon, 31 Aug 2026 09:44:29 +0100 Subject: [PATCH 7/7] fix(progress-tracker): restore achievement variants misplaced by #374's merge, and fix two resulting compile errors #374's merge of storage-size tracking (#239) into this file botched the surrounding hunk: it closed `ProgressTrackerDataKey`'s enum body right after the new `StorageSize` variant, leaving the pre-existing `Achievements`/`AchievementEarned` variants stranded as dangling tokens after `write_entry`'s function body instead of inside the enum, and left `write_entry` itself unclosed. This broke `cargo check --workspace` on plain `main` with a parse error, confirmed present identically on `main` before this branch touched anything -- `types.rs` was otherwise byte-for-byte unchanged from `main`. Moved `Achievements`/`AchievementEarned` back into the enum body (right after `StorageSize`, where the diff put them originally) and closed `write_entry` where the parser actually needed it. Fixing the parse error surfaced two further pre-existing, unrelated compile errors in the same achievement-awarding code path, also present unchanged on `main`: - `complete_module_in_place` called a `get_learner_stats_internal` that was never defined (only the public `get_learner_stats(env: Env, ...)` exists) -- likely a rename that was never finished. Called the real function instead, cloning `env`/`learner` since this call site only has `&Env`/`&Address`. - `earn_achievement`'s `achievement_earned` event published `&AchievementType` by reference; soroban-sdk 21.7.7 doesn't implement the ScVal conversion for a reference to a custom `#[contracttype]` enum (only for owned values and SDK built-ins), so publishing failed to compile. `achievement_type` isn't used after this call, so passed it by value instead. `cargo check --workspace` and `cargo test -p progress-tracker --lib` (79 passed) now succeed. --- contracts/progress-tracker/src/lib.rs | 4 ++-- contracts/progress-tracker/src/types.rs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/progress-tracker/src/lib.rs b/contracts/progress-tracker/src/lib.rs index 961e95b..afa5e76 100644 --- a/contracts/progress-tracker/src/lib.rs +++ b/contracts/progress-tracker/src/lib.rs @@ -542,7 +542,7 @@ impl ProgressTracker { ); // Check for CourseMaster achievement (5 courses completed) - let stats = Self::get_learner_stats_internal(env, learner); + let stats = Self::get_learner_stats(env.clone(), learner.clone()); if stats.courses_completed >= 5 { Self::earn_achievement( env, @@ -1549,7 +1549,7 @@ impl ProgressTracker { // Emit achievement earned event env.events().publish( (Symbol::new(env, "achievement_earned"),), - (learner, &achievement_type, course_id, timestamp), + (learner, achievement_type, course_id, timestamp), ); } diff --git a/contracts/progress-tracker/src/types.rs b/contracts/progress-tracker/src/types.rs index 351b577..d722c79 100644 --- a/contracts/progress-tracker/src/types.rs +++ b/contracts/progress-tracker/src/types.rs @@ -192,6 +192,10 @@ pub enum ProgressTrackerDataKey { /// Running count of persistent storage entries this contract has /// written, excluding this counter entry itself (#239). StorageSize, + /// Achievements earned by a learner. + Achievements(Address), + /// Achievement earned by a specific learner and achievement type (for deduplication). + AchievementEarned(Address, AchievementType), } // ── Storage Size Tracking (#239) ───────────────────────────────────────────── @@ -239,8 +243,4 @@ where if is_new { bump_storage_size(env, 1); } - /// Achievements earned by a learner. - Achievements(Address), - /// Achievement earned by a specific learner and achievement type (for deduplication). - AchievementEarned(Address, AchievementType), }