fix(globe-wallet): key spend budgets by asset identity, not by code (closes #82) - #111
Open
rudrasatani13 wants to merge 1 commit into
Open
rudrasatani13 wants to merge 1 commit into
rudrasatani13 wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #82
The one-line note the Definition of done asks for: the fix lives in
contracts/globe-wallet/src/lib.rsbecause that is whereSpendLimit/DailySpent/set_spend_limit/record_spendactually are.contracts/token-wrapper/src/lib.rsis not touched by this PR —git show --name-onlylists exactly two source files, both underglobe-wallet, and the rest are regenerated test snapshots.What was broken
SpendLimit(Address, String)andDailySpent(Address, String)keyed on(user, asset_code), and all three entry points took a bareasset_code: Stringwithout consultingUserAssets. 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
DataKeyvariants are encoded by index, and this repo already relies on that:GuardianMembershipcarries 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 reinterpretedGuardians,RecoveryConfig,RecoveryProposal,TokenWrapperIdandAllowedToken(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 newSpendLimit(Address, String, Option<Address>)/DailySpent(Address, String, Option<Address>)are appended at the end.Behaviour changes worth reviewing
AssetInfoand resolve it against the caller'sUserAssets, keying on the registered entry. An unregistered asset is rejected with the existingWalletError::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#82asks.get_spend_limitnow returnsResult<i128, WalletError>. It has to be able to reject an unregistered asset, which it cannot do while returning a barei128.add_assetalready 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()takesAssetInfoinstead ofasset_code: String. That argument existed solely to reachrecord_spend; nothing else insendreads it.spend_limit_set,spend_recordednow carry code+issuer instead of a bare code). Indexers will need to follow. Flagging it rather than letting it surprise someone.migrate_user_assetsnow 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/DailySpentdisambiguate by issuer, implemented incontracts/globe-wallet/src/lib.rs— and not intoken-wrapperset_spend_limit,get_spend_limit,record_spendtake a full asset identity and reject an asset not present in the caller'sUserAssets(Address, String)key:migrate_spend_limit(admin, user, legacy_code, asset), followingmigrate_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.globe-wallet's own test module (see below)cargo test --workspaceoutput pasted belowThe headline test, and why it is not vacuous
test_same_code_different_issuer_get_independent_budgetssets a 1,000 limit on oneUSDC, records a 900 spend against a different-issuerUSDC, 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 withSpendLimitExceededafter only 100 remained.add_assetrefuses two same-code assets in one wallet (code-only duplicate detection), so the test seedsUserAssetsdirectly. 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:
103 tests, 0 failures. My own additions are rustfmt-clean;
cargo fmt --checkstill 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.