[Bug]: remove_asset doesn't clean up SpendLimit/DailySpent — inconsistent with migrate_user_assets, which does the same cleanup for the same conceptual operation

[ndii-dev]

Context

GlobeWallet::remove_asset vs. GlobeWallet::migrate_user_assets, contracts/globe-wallet/src/lib.rs. Both remove one or more entries from a user's UserAssets list. Only one of them cleans up the corresponding SpendLimit/DailySpent entries.

Problem

migrate_user_assets (the fix for issue #30) explicitly does this cleanup for every asset it trims:

for i in Self::MAX_ASSETS..len {
    let dropped = assets.get(i).unwrap();
    env.storage().persistent().remove(&DataKey::SpendLimit(user.clone(), dropped.code.clone()));
    env.storage().persistent().remove(&DataKey::DailySpent(user.clone(), dropped.code.clone()));
}

remove_asset — the far more commonly-called function, since it's the normal user-facing "I don't want to track this asset anymore" path — does not:

pub fn remove_asset(env: Env, user: Address, asset_code: String) -> Result<(), WalletError> {
    user.require_auth();
    ...
    // Removes the entry from `assets`/`UserAssets` only. SpendLimit and
    // DailySpent for `asset_code` are never touched.
    ...
}

Reproduction steps

#[test]
fn test_remove_asset_leaves_orphaned_spend_limit() {
    let (env, _cid, admin, client) = setup();
    let user = Address::generate(&env);
    client.add_asset(&user, &usdc(&env));
    client.set_spend_limit(&user, &String::from_str(&env, "USDC"), &500);
    client.record_spend(&user, &String::from_str(&env, "USDC"), &200);

    client.remove_asset(&user, &String::from_str(&env, "USDC"));
    assert_eq!(client.get_assets(&user).len(), 0);

    // The limit the user set before removing the asset silently survives.
    assert_eq!(client.get_spend_limit(&user, &String::from_str(&env, "USDC")), 500); // still 500, not reset

    // Re-adding the same asset later silently reactivates the old limit and
    // whatever's left of that day's counter -- config the user never
    // re-entered and has no reason to expect still applies.
    client.add_asset(&user, &usdc(&env));
    assert_eq!(client.get_spend_limit(&user, &String::from_str(&env, "USDC")), 500);
}

Impact

Two distinct problems from one gap: (1) unbounded persistent-storage growth over a wallet's lifetime — every asset a user ever adds and removes leaves a permanent SpendLimit/DailySpent entry behind, with no path to reclaim that storage, the exact kind of orphaned-state accumulation issue #44 already flagged for PendingAdmin and fixed there; (2) a correctness/UX surprise — a user who removes an asset (reasonably assuming its configuration goes with it) and later re-adds it gets an old, possibly stale limit silently reapplied without ever having configured it for the "new" registration.

Suggested fix

Give remove_asset the same cleanup migrate_user_assets already has for the dropped-asset case: remove SpendLimit(user, asset_code) and DailySpent(user, asset_code) alongside removing the UserAssets entry.

Definition of done

• remove_asset removes the corresponding SpendLimit and DailySpent entries for the removed asset
• Test proving get_spend_limit returns to the default (0 / unlimited) after remove_asset
• Test proving re-adding a previously-removed asset starts with a fresh, unconfigured limit rather than inheriting the old one
• No regression to migrate_user_assets's existing equivalent cleanup
• cargo test --workspace output pasted

[DevSunix]

Hi team! I'd like to work on this issue.
I'll update remove_asset in contracts/globe-wallet/src/lib.rs to clean up the persistent storage entries for SpendLimit and DailySpent upon asset removal, matching migrate_user_assets. I'll also add the required tests to verify state reset on asset re-addition.

[syed-ghufran-hassan]

Hi mate, may I work on this issue?

[ZacLou]

Opened a PR that fixes this: #109.