Skip to content

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

Description

@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

Metadata

Metadata

Assignees

No one assigned

    Labels

    GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardThird CampaignCampaign: Third CampaignbugSomething isn't workingvery hardDifficulty: very hard

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions