Soulbound transfer rejection, delayed admin transfer, init check, and storage-size tracking by uche001-dev · Pull Request #374 · ChainLearnOfficial/chainlearn-contracts

uche001-dev · 2026-08-30T15:39:02Z

Summary

Implements four related contract-hardening issues across the workspace: soulbound-credential transfer rejection, a time-delayed admin transfer for the token contract, a consistent is_initialized() check on every contract, and incremental storage-size tracking on every contract.

#242 — Credential transfer rejection with reason

credential-nft's transfer function existed but rejected by panic!. It now returns Result<(), ContractError> and always rejects with a new ContractError::Soulbound variant instead.

Design decisions:

The rejection is unconditional and reads/writes no storage, so there's nothing to authorize -- require_auth() was dropped from the old panic-based version so every caller (authorized or not) gets the same typed Soulbound error instead of a different failure mode depending on auth.
Kept the existing extensive doc comment explaining the soulbound rationale, expanded slightly to state the "no storage access" guarantee explicitly.

Edge cases covered: calling with a nonexistent credential_id, no mock_all_auths() at all, and asserting the full credential record + both parties' credential lists are byte-identical before and after the call.

#241 — Admin transfer delay (learn-token)

transfer_admin(new_admin) no longer updates Admin immediately. It now records a PendingAdminTransfer { new_admin, initiated_at } and emits admin_transfer_initiated. The transfer only completes when new_admin itself calls the new accept_admin() after admin_transfer_delay() (default 48h) has elapsed. The current admin can call the new cancel_admin_transfer() at any point before acceptance to abort it. set_admin_transfer_delay(seconds) (admin-only) makes the delay configurable.

Design decisions:

accept_admin() requires the pending new_admin's auth, not the caller's -- this proves control of the destination key before granting it admin rights (mirrors a standard two-step-ownership-transfer pattern, with a timelock added on top).
Calling transfer_admin again before acceptance overwrites the pending candidate and restarts the delay; the previous candidate is simply discarded, no separate "cancel" step required to redirect it.
Events for all three lifecycle transitions (admin_transfer_initiated / _accepted / _cancelled) plus admin_transfer_delay_updated, following the file's existing topic-indexing convention (candidate address in topics[1]).

Edge cases covered: accept before delay elapses, accept exactly at/after the boundary, cancel then attempt accept after the delay has passed anyway, delay reconfiguration applied to a newly-initiated transfer, and re-initiating a transfer overwriting an earlier pending candidate.

#240 — `is_initialized()` on every contract

The workspace has three contracts (learn-token, credential-nft, progress-tracker). Each gets a new is_initialized() returning a plain bool from a single storage existence check (has(&...DataKey::Admin)) -- the same sentinel each contract's own initialize() already checks for double-init.

There's no shared crate pattern for this (the chainlearn-shared crate only holds constants and ContractMetadata, no shared storage/error abstractions), so each contract implements the identical one-liner locally rather than introducing a new shared-crate dependency for a single boolean check. learn-token already had this exact logic as a private storage::is_initialized() helper; this just exposes it on the contract.

#239 — Storage size tracking

Soroban has no API to enumerate or count a contract's persistent entries, so get_storage_size() can't be computed by scanning -- there's nothing to scan. Each contract instead maintains a StorageSize counter, incremented/decremented by two small wrapper functions (write_entry / remove_entry, added to each contract's storage-key module) that check has(key) before mutating so:

overwriting an existing key never double-counts it, and
removing a key that was never set never underflows the counter (saturating_sub).

Every existing persistent-storage call site across all three contracts (not just newly-added code) was migrated to go through these wrappers, so the counter reflects real entry counts from the moment a contract is initialized, not just paths touched by this PR. progress-tracker currently never removes a persistent entry, so it only gained write_entry.

Test plan

Added unit tests per issue per contract (see commits) covering: the Soulbound rejection + no-state-change guarantee; delay enforcement, cancellation, configurability, and event emission for admin transfer; before/after-init state and read-only-ness for is_initialized; and zero/increment/no-double-count/decrement/no-underflow behavior for get_storage_size.
cargo build --workspace -- passes.
cargo build --release --target wasm32-unknown-unknown -- passes; all three .wasm outputs produced with non-zero size (matches CI's build+verify steps).
cargo test --workspace on every [[test]]-declared unit/integration binary -- all pass except two pre-existing failures unrelated to this PR (see Caveats).
cargo clippy on learn-token and credential-nft (both fully touched by this PR) -- clean, zero warnings.
cargo fmt run on every file this PR touches.

Caveats (pre-existing, verified present on `main` before this PR, not introduced by it)

Cargo.toml had a malformed [[test]] block (admin_role_flow's entry was missing its own [[test]] header and got merged into upgrade_tests's), which made cargo unable to parse the manifest at all -- nothing built or tested on a clean checkout. Fixed as a standalone first commit since it blocked verifying anything else.
tests/unit/credential_tests.rs had a pre-existing unclosed-brace bug that failed the whole test binary to compile. Fixed alongside . Add credential transfer rejection with reason #242 since new tests were being added to that same file.
contracts/learn-token/src/lib.rs's internal #[cfg(test)] mod tests (compiled only via cargo test -p learn-token --lib, separate from the [[test]]-declared binaries in tests/) has 35 pre-existing compile errors -- mint/unpause/get_proposal and others gained/changed parameters in a recent commit and ~15 call sites in this module were never updated. Confirmed via git blame this predates this PR and is unrelated to any of the four issues; left as-is rather than folding an unrelated 35-error fix into this PR.
contracts/progress-tracker/src/rewards.rs:19 fails cargo clippy -- -D warnings on a pre-existing manual_checked_ops lint, which blocks clippy from reaching the rest of the progress-tracker crate (including the code this PR adds there). Confirmed identical on main; left unfixed as out of scope.
tests/unit/security_arithmetic_tests.rs::test_mint_... and tests/integration/security_reentrancy_tests.rs::test_reentrancy_during_transfer each have one pre-existing failing test, confirmed failing identically on main (verified via a clean worktree checkout). Unrelated to this PR's scope.

closes #242
closes #241
closes #240
closes #239

…inLearnOfficial#242) transfer() now returns Result<(), ContractError> and always rejects with the new ContractError::Soulbound variant instead of panicking, so callers get a documented, typed reason instead of a raw host trap. The rejection is unconditional (no auth check, no storage access), so no state is ever mutated. Also fixes an existing unclosed-brace bug in credential_tests.rs that otherwise fails the whole test binary to compile, and adds test coverage for the new behavior.

…rnOfficial#241) transfer_admin(new_admin) no longer updates the admin immediately. It now records a PendingAdminTransfer (new_admin + initiated_at) and emits admin_transfer_initiated; the transfer only takes effect once new_admin calls accept_admin() after admin_transfer_delay() has elapsed (default 48h, admin-configurable via set_admin_transfer_delay). The current admin can call cancel_admin_transfer() at any point before acceptance to abort an unauthorized or mistaken transfer. This closes the window where a single compromised admin key could hand control to an attacker-controlled address in one transaction. Added: - storage::PendingAdminTransfer, TokenDataKey::PendingAdmin/AdminTransferDelay - accept_admin(), cancel_admin_transfer(), pending_admin(), admin_transfer_delay(), set_admin_transfer_delay() - events: admin_transfer_initiated/accepted/cancelled, admin_transfer_delay_updated - unit tests covering delay enforcement, cancellation, configurability, auth requirements, event emission, and re-initiation overwrite semantics

Adds a read-only is_initialized() function to learn-token, credential-nft, and progress-tracker, so deployment scripts can check initialization status directly instead of inferring it from some other call panicking with 'not initialized'. Implementation is consistent across all three contracts: each checks for the presence of its own Admin storage key, matching the same sentinel each contract's initialize() already uses to guard against double-initialization. learn-token already had this exact check as a private storage::is_initialized() helper; this just exposes it on the contract. credential-nft and progress-tracker gained the equivalent inline (neither has a separate storage.rs module). Tests added per contract: false before initialize, true after, and a before/after metadata comparison confirming the call is read-only.

…icial#239) Adds a get_storage_size() read-only function to learn-token, credential-nft, and progress-tracker that returns the number of persistent storage entries each contract has written. Soroban has no API to enumerate or count a contract's storage entries at runtime, so the count can't be computed by scanning -- there is nothing to scan. Instead each contract maintains an ordinary persistent counter (StorageSize), kept in sync by routing every persistent write (and, for learn-token, every removal) through a write_entry()/remove_entry() wrapper instead of calling env.storage().persistent().set()/remove() directly. Both wrappers check whether the key already exists before mutating, so overwriting an existing key never double-counts it and removing a key that was never set never underflows the counter. Every existing persistent-storage call site in all three contracts was migrated to the wrappers so the counter reflects real entry counts, not just newly-added code paths. Tests added per contract verify: zero before initialize, the exact count after initialize, +1/+N on genuinely new keys, unchanged on overwrites of existing keys, and (learn-token) -1 on removal with no underflow when removing a nonexistent entry.

drips-wave · 2026-08-31T01:50:35Z

@uche001-dev Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

…ck-storage-tracking

…erged ChainLearnOfficial#242's Soulbound transfer rejection This branch originally set out to implement ChainLearnOfficial#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 ChainLearnOfficial#227 is a content-duplicate of ChainLearnOfficial#242 (identical title and body), which was already implemented and merged via ChainLearnOfficial#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 ChainLearnOfficial#242) and added a note cross-referencing the ChainLearnOfficial#227 duplicate; did not reintroduce the auth requirement or duplicate the existing test coverage.

…nLearnOfficial#374's merge, and fix two resulting compile errors ChainLearnOfficial#374's merge of storage-size tracking (ChainLearnOfficial#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.

…d-blocking bug fixes (#378) * 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. * fix(credential-nft): CredentialVerification.display can't use Option<CredentialDisplay> CredentialVerification.display was Option<CredentialDisplay>, but soroban-sdk 21.7.7's #[contracttype] derive does not implement the ScVal (client/spec) conversion for Option<T> 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<CredentialDisplay>> 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<CredentialDisplay> holding 0 or 1 elements instead, via new no_display/one_display helpers -- every field inside CredentialDisplay itself stays a true Option<Symbol>, 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. * 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. * 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. * 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. * 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. * 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.

uche001-dev added 4 commits August 31, 2026 02:29

uche001-dev force-pushed the feat/soulbound-transfer-admin-delay-init-check-storage-tracking branch from f7ef01b to f225136 Compare August 31, 2026 01:50

Merge branch 'main' into feat/soulbound-transfer-admin-delay-init-che…

8175ec9

…ck-storage-tracking

DeFiVC merged commit 78c5a66 into ChainLearnOfficial:main Aug 31, 2026

iduhtheman mentioned this pull request Aug 31, 2026

fix: reject credential transfer with a typed error, plus several build-blocking bug fixes #378

Merged

Navigation Menu

Uh oh!

Soulbound transfer rejection, delayed admin transfer, init check, and storage-size tracking - #374

Soulbound transfer rejection, delayed admin transfer, init check, and storage-size tracking#374
DeFiVC merged 5 commits into
ChainLearnOfficial:mainfrom
uche001-dev:feat/soulbound-transfer-admin-delay-init-check-storage-tracking

uche001-dev commented Aug 30, 2026

Uh oh!

drips-wave Bot commented Aug 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

uche001-dev commented Aug 30, 2026

Summary

#242 — Credential transfer rejection with reason

#241 — Admin transfer delay (learn-token)

#240 — is_initialized() on every contract

#239 — Storage size tracking

Test plan

Caveats (pre-existing, verified present on main before this PR, not introduced by it)

Uh oh!

drips-wave Bot commented Aug 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

#240 — `is_initialized()` on every contract

Caveats (pre-existing, verified present on `main` before this PR, not introduced by it)