Skip to content

fix(globe-wallet): key spend budgets by asset identity, not by code (closes #82) - #111

Open
rudrasatani13 wants to merge 1 commit into
Orbit-Wal:mainfrom
rudrasatani13:fix/82-spend-limit-asset-identity
Open

rudrasatani13 wants to merge 1 commit into
Orbit-Wal:mainfrom
rudrasatani13:fix/82-spend-limit-asset-identity

Conversation

@rudrasatani13

Copy link
Copy Markdown

Closes #82

The one-line note the Definition of done asks for: the fix lives in contracts/globe-wallet/src/lib.rs because that is where SpendLimit/DailySpent/set_spend_limit/record_spend actually are. contracts/token-wrapper/src/lib.rs is not touched by this PRgit show --name-only lists exactly two source files, both under globe-wallet, and the rest are regenerated test snapshots.

What was broken

SpendLimit(Address, String) and DailySpent(Address, String) keyed on (user, asset_code), and all three entry points took a bare asset_code: String without consulting UserAssets. A token sharing a ticker with a different issuer — trivial to mint on Stellar — shared the real asset's budget, and nothing tied the free-form string to the registry either.

The constraint that shaped the design

DataKey variants are encoded by index, and this repo already relies on that: GuardianMembership carries a comment saying it "is intentionally appended so the serialized values of existing storage keys remain stable across contract upgrades". So the issuer-aware variants could not go where the old ones were. Inserting them would have shifted every discriminant after them and silently reinterpreted Guardians, RecoveryConfig, RecoveryProposal, TokenWrapperId and AllowedToken(Address) for every deployed wallet.

So: the existing variants are renamed in place to LegacySpendLimit/LegacyDailySpent (rename only — positions and payload types untouched, so old entries still decode), and the new SpendLimit(Address, String, Option<Address>) / DailySpent(Address, String, Option<Address>) are appended at the end.

Behaviour changes worth reviewing

  • The three entry points take AssetInfo and resolve it against the caller's UserAssets, keying on the registered entry. An unregistered asset is rejected with the existing WalletError::AssetNotFound — no new error variant. This also closes the free-form-string gap ([Bug]: record_spend/set_spend_limit take a free-form asset_code string, completely decoupled from the case-insensitive asset registry issue #29 built #88) in the same change, as #82 asks.
  • get_spend_limit now returns Result<i128, WalletError>. It has to be able to reject an unregistered asset, which it cannot do while returning a bare i128.
  • A code case-variant resolves to one budget, not two. add_asset already treats codes as equal case-insensitively, so "usdc" and "USDC" now resolve to the same registered entry rather than opening a second bucket. New test.
  • send() takes AssetInfo instead of asset_code: String. That argument existed solely to reach record_spend; nothing else in send reads it.
  • Event payloads changed shape (spend_limit_set, spend_recorded now carry code+issuer instead of a bare code). Indexers will need to follow. Flagging it rather than letting it surprise someone.
  • migrate_user_assets now cleans both key schemes for dropped assets. Without that, a dropped asset's budget would survive and be silently inherited by a later re-registration of the same code.

Definition of done

  • SpendLimit/DailySpent disambiguate by issuer, implemented in contracts/globe-wallet/src/lib.rs — and not in token-wrapper
  • set_spend_limit, get_spend_limit, record_spend take a full asset identity and reject an asset not present in the caller's UserAssets
  • Migration path for entries persisted under the old (Address, String) key: migrate_spend_limit(admin, user, legacy_code, asset), following migrate_user_assets's admin-plus-user auth pattern. It takes the legacy code explicitly because the old key used whatever casing the caller passed — it cannot be derived from the registry, only supplied by the operator.
  • Test proving two same-code, different-issuer assets get independent budgets, in globe-wallet's own test module (see below)
  • The one-line note at the top of this description
  • cargo test --workspace output pasted below

The headline test, and why it is not vacuous

test_same_code_different_issuer_get_independent_budgets sets a 1,000 limit on one USDC, records a 900 spend against a different-issuer USDC, then spends the full 1,000 against the first. On the old code both spends landed in one (user, "USDC") bucket, so the second call would have failed with SpendLimitExceeded after only 100 remained.

add_asset refuses two same-code assets in one wallet (code-only duplicate detection), so the test seeds UserAssets directly. That is deliberate: what is under test is that the storage layer keys on identity rather than on the code string, which is the substance of #82.

New tests: independent budgets, unregistered-asset rejection across all three entry points, case-variant resolution to one budget, legacy migration moving both entries and clearing the old keys, and migration being a no-op when nothing was written.

Verification

CI's own gate sequence, run locally:

$ cargo check --workspace --all-targets
    Finished `dev` profile [unoptimized + debuginfo] target(s)

$ cargo test --workspace
     Running unittests src/lib.rs (globe_wallet)
running 91 tests
test result: ok. 91 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.50s
     Running tests/record_spend_reentrancy.rs
running 1 test
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.06s
     Running unittests src/lib.rs (token_wrapper)
running 11 tests
test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.44s
   Doc-tests globe_wallet
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
   Doc-tests token_wrapper
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

103 tests, 0 failures. My own additions are rustfmt-clean; cargo fmt --check still reports pre-existing hunks elsewhere in the file which this PR deliberately does not touch, to keep the diff reviewable.

Test snapshots

test_snapshots/** is regenerated and included, because this repo tracks those files and the spend tests' ledger state legitimately changed. They are deterministic — re-running the suite twice leaves them byte-identical, and the only addresses in them are the mocked harness's synthetic ones.

This is AI-assisted implementation. CONTRIBUTING states no policy either way on that, and the PR is yours to judge on its merits — but you should know how it was produced rather than infer it.

SpendLimit/DailySpent were keyed by (user, asset_code), and
set_spend_limit/record_spend/get_spend_limit took a bare asset_code string
with no check against UserAssets at all. An asset sharing a ticker with a
different issuer - trivial to mint on Stellar - therefore shared the real
asset's budget, and nothing tied the string to the registry either. This is
what Orbit-Wal#8 described; the fix landed in token-wrapper, which has no spend-limit
code in it, so the gap has been live on main since.

Storage now keys on (user, code, issuer). The three entry points resolve
their argument against the caller's UserAssets and key on the registered
entry, so an unregistered asset is rejected with AssetNotFound and a code
case-variant resolves to the same budget rather than opening a second one.
get_spend_limit returns Result<i128, WalletError> so it can reject too.

The issuer-aware variants are appended to DataKey rather than replacing the
existing ones in place: variants are encoded by index, so inserting would
shift every key after them and reinterpret Guardians, RecoveryConfig,
RecoveryProposal, TokenWrapperId and AllowedToken for every deployed wallet.
The old variants are renamed LegacySpendLimit/LegacyDailySpent at their
original positions and payloads, and read only by the new
migrate_spend_limit, which moves existing entries onto the new keys.

send() takes the full identity as well, because its asset_code argument
existed solely to reach record_spend. token-wrapper is untouched.

Closes Orbit-Wal#82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Issue #8's fix landed in the wrong contract — the SpendLimit/DailySpent code/issuer collision it describes is still live on main

1 participant