stellar-contracts: make access-grant index creation provably idempotent - #1282
Merged
llinsss merged 2 commits intoAug 31, 2026
Merged
Conversation
grant_access read storage twice to answer two related questions: what was the prior grant state (to build the updated record), and whether this grantee is new to the index (to decide whether to push a new AccessGrantIndex entry). Both reads always agreed in practice since nothing writes to the key between them, but that safety depended on the absence of an intervening write rather than being structural. Derives is_new_grant from the same single read used to build the updated grant record, so "one canonical AccessGrantIndex/AccessGrantCount entry per grantee" holds by construction. No behavior change, no public ABI/storage layout/error discriminant change. Adds a test proving repeated grant_access calls for the same (pet_id, grantee) - across different access levels and expiry values - never grow AccessGrantCount past 1 and never create a duplicate index slot. Closes DogStark#1160
|
@samuel2926i39-art Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Summary
grant_accessread instance storage twice to answer two related questions: "what was the grant's prior state" (used to build the updatedAccessGrantrecord) and "is this grantee new to the index" (is_new_grant, used to decide whether to push a newAccessGrantIndexentry / bumpAccessGrantCount). Both reads happened before any write to that key, so they always agreed in practice — but that correctness depended on nobody ever inserting a write between them, not on the code structurally guaranteeing it. That's a fragile invariant to rely on in a function whose whole job is "don't create a second index entry for the same grantee."What changed
grant_accessnow performs one read of the existingAccessGrant(if any), and derives both the updated grant record andis_new_grantfrom that singleOption. "One canonical index entry per grantee" is now true by construction, not by the absence of an intervening write.AccessGrantIndex/AccessGrantCountentry the first time a given grantee is granted access for a pet. No public ABI, storage layout, orContractErrordiscriminant changed.Test added
test_grant_access_repeated_calls_keep_one_canonical_index_entry(intest_access_control.rs): callsgrant_accessthree times for the same(pet_id, grantee)— varying access level and expiry each time, usingget_caller_noncefor correct nonce sequencing — then peeks at internal storage viaenv.as_contract(there's no public getter forAccessGrantCount/AccessGrantIndex) to assert the count stays at exactly 1 and index slot 1 still holds the grantee, and confirmscheck_accessreflects the latest call's values (proving this was an update, not two independent records).Same caveat as my companion PR for #1159 on this same file:
stellar-contractsfails to compile on a cleanupstream/maincheckout with 346 pre-existing errors, entirely unrelated to access grants (duplicateconstdefinitions, a panicking#[contracterror]macro,ContractErrornot resolving in someimplblocks, tests calling contract methods that don't exist). I could not runcargo build,clippy, orcargo testto a passing state here.What I could do: I ran
cargo build --libafter this change and confirmed none of the 346 errors are new or located at any line this PR touches (checked by line number and by searching the full error log foris_new_grant/ the new test's name). While investigating, I also noticed the existingtest_access_expirytest in this same file callsgrant_accesswith only 4 arguments, but the real function signature takes 5 (pet_id, grantee, access_level, expires_at, nonce) — one more symptom of the broader pre-existing breakage. My new test uses the correct 5-argument signature.I'd recommend a maintainer build/test this specific diff against a working local checkout before merging, or fix the underlying crate-wide breakage first — I don't want to claim verified-green work that isn't.
Threat-model note
This is a robustness/structural-correctness change, not a new authorization boundary:
grant_accessis still gated byowner.require_auth()and nonce-based replay protection, unchanged. The fix only affects how the function decides internally whether to grow the index — it doesn't change who can call it or what they're authorized to do. The risk this closes is a maintenance/drift risk (a future refactor inserting a write between the two old reads could silently reintroduce duplicate index entries) rather than a live exploit today.Closes #1160