[Bug]: token-wrapper's Allowance is keyed only by (owner, spender), not (owner, spender, token) — approving a second asset silently destroys the first asset's allowance

[ndii-dev]

Context

TokenWrapper::approve / TokenWrapper::transfer_from, contracts/token-wrapper/src/lib.rs. Per docs/design/architecture.md, this contract's stated purpose is "to enable the globe-wallet contract to move tokens on behalf of users" across the assets a user has registered — i.e. one spender (globe-wallet's contract ID) acting across multiple tokens for the same owner.

Problem

#[contracttype]
pub enum DataKey {
    /// (owner, spender) → (amount, expiry_ledger)
    Allowance(Address, Address),
}

token_id never appears in the storage key. It's only used inside transfer_from to construct the token client for the actual transfer — it plays no role in which Allowance record is read, checked, or written:

pub fn transfer_from(env: Env, spender: Address, token_id: Address, from: Address, to: Address, amount: i128) -> Result<(), WrapperError> {
    spender.require_auth();
    ...
    let key = DataKey::Allowance(from.clone(), spender.clone()); // no token_id component
    ...
}

Since approve's doc comment already establishes "calling approve for an existing (owner, spender) pair replaces the previous allowance wholesale" — that overwrite isn't scoped to "re-approving the same token." Approving spender for any second token silently wipes out whatever allowance was in place for the first token, with the same spender, even if the spender never used a cent of it.

Reproduction steps

#[test]
fn test_approving_second_token_destroys_first_tokens_allowance() {
    let (env, _id, client) = setup();
    let admin = Address::generate(&env);
    let owner = Address::generate(&env);
    let spender = Address::generate(&env); // e.g. globe-wallet's contract ID
    let (token_a, token_a_admin, _) = create_token_contract(&env, &admin);
    let (token_b, _, _) = create_token_contract(&env, &admin);
    token_a_admin.mint(&owner, &1_000);

    env.ledger().with_mut(|l| l.sequence_number = 100);

    // Owner grants spender 500 of token_a. Spender hasn't used any of it yet.
    client.approve(&owner, &spender, &500, &300);
    assert_eq!(client.allowance(&owner, &spender).amount, 500);

    // Owner separately wants to grant the SAME spender an allowance for a
    // DIFFERENT token, token_b -- a completely reasonable multi-asset flow
    // per this contract's own stated purpose.
    client.approve(&owner, &spender, &300, &300); // intended for token_b

    // The token_a allowance is gone. Nothing about this call referenced
    // token_a or token_b -- the key can't distinguish them.
    assert_eq!(client.allowance(&owner, &spender).amount, 300); // was 500, now 300

    // Spender can no longer move the token_a allowance owner thought was
    // still there, OR (worse, depending on call order) could move an amount
    // against token_b using an allowance figure the owner set thinking
    // about token_a. Either way, the allowance the owner most recently
    // configured "for token_a" is unrecoverable and was never spent.
}

Impact

Any integrator using this contract the way its own docs describe — one spender (globe-wallet) managing allowances across a user's several registered assets — cannot safely grant a second asset's allowance to the same spender without destroying the first. There is no workaround from the caller's side: the storage schema itself only has room for one live allowance per (owner, spender) pair, full stop. This isn't a rare edge case; it's the normal multi-asset case the contract exists to serve.

Suggested fix

Add token_id: Address as a third component of the DataKey::Allowance key (and the natural third field/parameter everywhere it's threaded through approve/allowance/transfer_from). This is a storage-schema change; existing (owner, spender)-only allowances need either a migration path or an explicit decision (documented, per CONTRIBUTING.md's scope-expectations section) that pre-existing allowances are intentionally invalidated by the schema change and integrators must re-approve.

Definition of done

• DataKey::Allowance includes token_id; approve, allowance, transfer_from all take/use it
• Test proving two different tokens' allowances for the same (owner, spender) pair are now independent (approving one doesn't touch the other)
• Existing single-token tests (test_approve_overwrites_previous_allowance, etc.) updated to reflect same-token overwrite is still the correct behavior — only cross-token overwrite was the bug
• Migration/compat decision for pre-existing (owner, spender)-keyed allowances written out explicitly in the PR description
• cargo test --workspace output pasted

[codefather2026]

Hi maintainer, i will like to solve this issue