Conversation
📝 WalkthroughWalkthroughThis PR introduces a sponsored vault feature enabling sponsors to create vaults on behalf of owners. It includes dependency updates, five new contract entrypoints for vault sponsorship management, implementation logic with authorization checks, storage layer updates, a new error variant, and comprehensive test coverage for the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant Sponsor as Sponsor/Admin
participant Contract as VcVault Contract
participant Storage as Storage Layer
Sponsor->>Contract: create_sponsored_vault(sponsor, owner, did_uri)
Contract->>Contract: sponsor.require_auth()
Contract->>Storage: Check if vault exists for owner
Storage-->>Contract: Vault status
alt Vault not found
Contract->>Storage: read_sponsored_vault_open_to_all()
Storage-->>Contract: open_to_all flag
alt open_to_all or sponsor authorized
Contract->>Storage: write vault_admin(owner) = owner
Contract->>Storage: write vault_did(owner) = did_uri
Contract->>Storage: initialize issuers, revoked flag
Contract->>Contract: extend_ttl()
Contract-->>Sponsor: Success
else Sponsor not authorized
Contract-->>Sponsor: NotAuthorizedSponsor error
end
else Vault exists
Contract-->>Sponsor: Duplicate vault error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![almanax-ai[bot]](https://avatars.githubusercontent.com/in/934833?s=60&v=4){kind=small}
| panic_with_error!(e, ContractError::AlreadyInitialized); | ||
| } | ||
| storage::write_vault_admin(&e, &owner, &owner); | ||
| storage::write_vault_did(&e, &owner, &did_uri); |
There was a problem hiding this comment.
Access Control and Authorization: Unconsented vault initialization for owner in create_sponsored_vault
create_sponsored_vault only requires sponsor.require_auth() and then writes vault state for an arbitrary owner, including VaultDid. In open_to_all mode this allows any signer to pre-create (squat) a vault for any target address and permanently set arbitrary DID metadata, blocking the target from calling create_vault later (AlreadyInitialized) and potentially poisoning off-chain identity assumptions since there is no DID-update entrypoint.
Require explicit owner consent (e.g., owner.require_auth() or a signed owner authorization payload) OR make did_uri mutable by the vault admin so the owner can correct sponsored initialization; additionally consider restricting open_to_all to owner == sponsor if you want "self-sponsoring" only.
Actions
- Reply
/almanax ask <question>to ask a follow-up question. - Reply
/almanax dismiss [<reason>]and it won't appear again in future scans. - Reply
/almanax resolve [<reason>]to mark the finding as resolved. - Reply
/almanax severity <severity> [<reason>]to override the severity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
contracts/vc-vault/src/test.rs (1)
467-477: Narrowshould_panicchecks to expected contract errors.Generic panic checks can pass for unrelated failures; asserting expected error codes will make these tests deterministic and meaningful.
💡 Example tightening
-#[should_panic] +#[should_panic(expected = "Error(Contract, `#11`)")] fn test_unauthorized_address_cannot_create_sponsored_vault_in_restricted_mode() {Also applies to: 493-503, 506-517, 520-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/test.rs` around lines 467 - 477, The test uses a generic #[should_panic] which can hide unrelated failures; update test_unauthorized_address_cannot_create_sponsored_vault_in_restricted_mode (and the other tests noted at 493-503, 506-517, 520-529) to assert the specific contract error instead of expecting any panic: call the operation (client.create_sponsored_vault) in a way that captures the Result/Err and assert it equals the expected error variant/code (e.g., Unauthorized or the contract's specific error enum/value) or use the test harness helper that checks for a specific contract error, rather than relying on #[should_panic], so the test deterministically verifies the intended failure.contracts/vc-vault/src/storage/mod.rs (1)
413-443: Consider replacing the sponsorVec<Address>with key-based membership storage.Current checks/removals are linear-time. A key-per-sponsor approach scales better and avoids growing per-call cost on
create_sponsored_vault.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/vc-vault/src/storage/mod.rs` around lines 413 - 443, Replace the Vec<Address>-backed sponsored-sponsors list with key-based membership storage to avoid O(n) contains/removal; stop using DataKey::SponsoredVaultSponsors vector functions (read_sponsored_vault_sponsors, write_sponsored_vault_sponsors) and instead store a per-sponsor key (e.g. DataKey::SponsoredVaultSponsor(Address) or a mapping key derived from sponsor) whose presence or boolean value indicates membership. Update is_authorized_sponsor to check the per-sponsor key presence/get boolean from e.storage().instance() (constant-time), change add_sponsored_vault_sponsor to set that per-sponsor key when adding, and change remove_sponsored_vault_sponsor to remove/clear that key when deleting; remove or deprecate the Vec-based helpers and migrate any callers (like create_sponsored_vault) to use the new per-sponsor key checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 13: The Soroban SDK bump from v21→v23 introduces breaking API and
behavior changes; update tests and contract code accordingly: replace
Env::register_contract calls with Env::register, migrate any
DeployerWithAddress::deploy usage to deploy_v2 and pass the required constructor
args, switch Env::mock_all_auths() to mock_all_auths_allowing_non_root_auth()
for tests that only authenticate in subcalls, adjust TTL/storage tests to expect
auto-restoration of archived entries instead of panics, and replace any
Events::publish-based event tests/usage with the new #[contractevent]-based
publishing API; run the test suite and fix any resulting failures referencing
these symbols (Env::register, deploy_v2, mock_all_auths_allowing_non_root_auth,
#[contractevent], archived/TTL assertions).
In `@contracts/vc-vault/src/test.rs`:
- Around line 444-452: The test currently uses client.list_vc_ids(&owner) to
infer vault creation which can be true even if vault creation silently failed;
update the test_admin_creates_sponsored_vault (and the other affected tests at
the noted ranges) to assert vault creation explicitly by checking the vault
state or creation result: verify the return value or success of
client.create_sponsored_vault, then call a direct existence/state accessor such
as client.get_vault(&owner) or client.vault_exists(&owner) (or inspect vault
metadata/owner mapping exposed by the contract) and assert the expected values
(e.g., non-None result or matching owner/did_uri) instead of relying on
list_vc_ids being empty.
---
Nitpick comments:
In `@contracts/vc-vault/src/storage/mod.rs`:
- Around line 413-443: Replace the Vec<Address>-backed sponsored-sponsors list
with key-based membership storage to avoid O(n) contains/removal; stop using
DataKey::SponsoredVaultSponsors vector functions (read_sponsored_vault_sponsors,
write_sponsored_vault_sponsors) and instead store a per-sponsor key (e.g.
DataKey::SponsoredVaultSponsor(Address) or a mapping key derived from sponsor)
whose presence or boolean value indicates membership. Update
is_authorized_sponsor to check the per-sponsor key presence/get boolean from
e.storage().instance() (constant-time), change add_sponsored_vault_sponsor to
set that per-sponsor key when adding, and change remove_sponsored_vault_sponsor
to remove/clear that key when deleting; remove or deprecate the Vec-based
helpers and migrate any callers (like create_sponsored_vault) to use the new
per-sponsor key checks.
In `@contracts/vc-vault/src/test.rs`:
- Around line 467-477: The test uses a generic #[should_panic] which can hide
unrelated failures; update
test_unauthorized_address_cannot_create_sponsored_vault_in_restricted_mode (and
the other tests noted at 493-503, 506-517, 520-529) to assert the specific
contract error instead of expecting any panic: call the operation
(client.create_sponsored_vault) in a way that captures the Result/Err and assert
it equals the expected error variant/code (e.g., Unauthorized or the contract's
specific error enum/value) or use the test harness helper that checks for a
specific contract error, rather than relying on #[should_panic], so the test
deterministically verifies the intended failure.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Cargo.tomlcontracts/vc-vault/src/api/mod.rscontracts/vc-vault/src/contract.rscontracts/vc-vault/src/error.rscontracts/vc-vault/src/storage/mod.rscontracts/vc-vault/src/test.rs
|
|
||
| [workspace.dependencies] | ||
| soroban-sdk = { version = "21.0.0" } | ||
| soroban-sdk = { version = "23.4.0" } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
soroban-sdk 23.4.0 breaking changes from version 21 release notes changelog
💡 Result:
Breaking changes when upgrading soroban-sdk v21 → v23.4.0
v21 → v22 (breaking API changes)
- Contract test registration API renamed/unified
Env::register/Env::register_atreplaceEnv::register_contract/Env::register_contract_wasm. [1]
- Contract deployment API changed
DeployerWithAddress::deploy_v2replacesDeployerWithAddress::deploy(adds constructor-args parameter; pass()if none). [1]
- Fuzz/panic catching API deprecated
fuzz_catch_panicdeprecated; useEnv::try_invoke_contractand generatedtry_client methods. [1]
- Test snapshot event set changed
- Test snapshots no longer include diagnostic events—only contract + system events remain. [1]
v22 → v23 (breaking behavior / interface changes)
- Events API migration
#[contractevent]replacesEvents::publish(old publish path deprecated). [1]
- Token transfer “to” type change
MuxedAddressreplacesAddressas thetoargument ofTokenInterface::transfer(insoroban-token-sdk, but it impacts contracts using that interface). [1]
- Testing behavior for archived persistent entries changed
- Accessing archived persistent entries in tests no longer panics; restoration is emulated (instance storage is also persistent). [1]
v23.0.0 → v23.4.0
- No additional “Breaking Changes” section was called out in the v23.4.0 release notes; it’s primarily incremental updates (e.g., making trait functions with default implementations callable, test snapshot source options). [2]
- Most of the major v23-line breakage is concentrated at v23.0.0 (muxed address support, contractevent introduction, deprecations around events). [1][3]
Sources: docs.rs migration guide [1], GitHub releases v23.4.0 [2] and v23.0.0 [3].
🌐 Web query:
soroban-sdk Address require_auth Env mock_all_auths breaking changes 22 23
💡 Result:
In the soroban-sdk v22 → v23 timeframe, the main auth-related “gotchas” behind Address::require_auth + Env::mock_all_auths test failures are:
-
Address::require_auth()is “require_auth_for_argswith implied args”
require_auth()authorizes the current contract invocation including all invocation arguments, which are automatically inferred (it’s equivalent to callingrequire_auth_for_argswith the invocation’s args). This matters because auth matching/mocking is sensitive to the args being authorized. (docs.rs) -
Env::mock_all_auths()became stricter about “root invocation” auth coverage
In testutils,mock_all_auths()mocks allrequire_auth/require_auth_for_argscalls, but it will error if an address is only authorized in a non-root sub-invocation and that address wasn’trequire_auth’d in the root invocation. This commonly shows up when Contract B calls Contract A, and only A doesrequire_auth. (docs.rs) -
New escape hatch:
mock_all_auths_allowing_non_root_auth()
To support the “auth only happens in a subcall” pattern in tests, v23-era SDKs provideEnv::mock_all_auths_allowing_non_root_auth(), which is identical tomock_all_auths()except it does not fail when the root invocation didn’trequire_authfor a given address. (docs.rs)
If you share the exact error/panic text and a minimal test snippet (root call + any cross-contract calls), I can point to which of the above is triggering and what to change.
Citations:
- 1: https://docs.rs/soroban-sdk/latest/soroban_sdk/struct.Address.html
- 2: https://docs.rs/soroban-sdk/latest/src/soroban_sdk/env.rs.html
- 3: https://docs.rs/soroban-sdk/latest/src/soroban_sdk/env.rs.html
🌐 Web query:
soroban-sdk storage TTL API changes version 23 migration guide
💡 Result:
What changed in Protocol/Soroban SDK v23 around storage TTL / archival
- Archived-entry access behavior changed (automatic restoration): In Protocol 23,
InvokeHostFunctioncan automatically restore archived entries in the transaction footprint before contract execution, so “manual restoration” is generally no longer required. This also changes how apps should think about “expired/archived” state. [1][2] - Test behavior in
soroban-sdkchanged to match P23: In SDK 23, the test environment no longer errors when you read an entry whose TTL you’ve “expired” by advancing the ledger; instead it emulates auto-restoration, so reads may succeed where SDK 22 tests expected failure (this includes instance storage, since it is persistent too). [3]
Migration guide (v22 ➜ v23)
-
Fix tests that expected archived reads to fail
- If you had tests that advanced the ledger past an entry’s TTL and asserted a read fails, update them.
- Prefer asserting the TTL value directly after your bump/extend logic (e.g., use
get_ttlin testutils) rather than relying on an “archived read throws” behavior. [3]
-
Re-check your “restore vs extend TTL” transaction flow (off-chain)
- With P23 auto-restoration, the workflow shifts toward:
- Preflight/simulate to build the correct footprint (including any archived entries that will be restored), then
- Use Extend Footprint TTL when you want to keep entries alive longer.
- In the JS SDK,
ExtendFootprintTTLOptakesextendTo(a new minimum TTL applied to entries in the transaction’s read-only footprint; entries already above it are skipped). [2][4]
- With P23 auto-restoration, the workflow shifts toward:
Sources: Protocol 23 announcement (auto-restoration / no manual restore) [1], Stellar Core P23 release notes (auto-restoration) [2], Soroban SDK v23 migration note for tests [3], JS SDK docs for ExtendFootprintTTLOp semantics [4].
Validate Soroban SDK v21 → v23 migration before merge.
The upgrade spans two major versions with confirmed breaking changes:
- Test utilities:
Env::register_contractrenamed toEnv::register;DeployerWithAddress::deployreplaced bydeploy_v2(requires constructor args parameter) - Auth testing:
Env::mock_all_auths()became stricter about root invocation auth coverage; usemock_all_auths_allowing_non_root_auth()if auth only occurs in subcalls - Storage/TTL behavior: Test environment now emulates auto-restoration of archived entries instead of panicking on reads; update tests relying on "archived read fails" assertions
- Events:
#[contractevent]replacesEvents::publish; check if contract uses old event publishing
Confirm the test suite handles these changes, particularly around auth mocking and TTL assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` at line 13, The Soroban SDK bump from v21→v23 introduces breaking
API and behavior changes; update tests and contract code accordingly: replace
Env::register_contract calls with Env::register, migrate any
DeployerWithAddress::deploy usage to deploy_v2 and pass the required constructor
args, switch Env::mock_all_auths() to mock_all_auths_allowing_non_root_auth()
for tests that only authenticate in subcalls, adjust TTL/storage tests to expect
auto-restoration of archived entries instead of panics, and replace any
Events::publish-based event tests/usage with the new #[contractevent]-based
publishing API; run the test suite and fix any resulting failures referencing
these symbols (Env::register, deploy_v2, mock_all_auths_allowing_non_root_auth,
#[contractevent], archived/TTL assertions).
| fn test_admin_creates_sponsored_vault() { | ||
| let (env, admin, _issuer, _contract_id, client) = setup(); | ||
| client.initialize(&admin, &String::from_str(&env, "did:acta:default")); | ||
| let owner = Address::generate(&env); | ||
| let did_uri = String::from_str(&env, "did:pkh:stellar:testnet:OWNER"); | ||
| client.create_sponsored_vault(&admin, &owner, &did_uri); | ||
| // Vault exists: list_vc_ids returns empty without panicking. | ||
| assert_eq!(client.list_vc_ids(&owner).len(), 0); | ||
| } |
There was a problem hiding this comment.
list_vc_ids is a weak proxy for “vault was created.”
These assertions can pass even if vault creation silently failed, because list_vc_ids can return an empty list for missing vault state as well.
💡 Suggested test assertion hardening
- // Vault exists: list_vc_ids returns empty without panicking.
- assert_eq!(client.list_vc_ids(&owner).len(), 0);
+ // Proves vault initialization path succeeded.
+ let new_admin = Address::generate(&env);
+ client.set_vault_admin(&owner, &new_admin);Also applies to: 455-464, 480-490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/vc-vault/src/test.rs` around lines 444 - 452, The test currently
uses client.list_vc_ids(&owner) to infer vault creation which can be true even
if vault creation silently failed; update the test_admin_creates_sponsored_vault
(and the other affected tests at the noted ranges) to assert vault creation
explicitly by checking the vault state or creation result: verify the return
value or success of client.create_sponsored_vault, then call a direct
existence/state accessor such as client.get_vault(&owner) or
client.vault_exists(&owner) (or inspect vault metadata/owner mapping exposed by
the contract) and assert the expected values (e.g., non-None result or matching
owner/did_uri) instead of relying on list_vc_ids being empty.
1 participant
Summary
Adds the Sponsored Vault feature to
vc-vault-contract. A sponsor (the contract admin or an authorized address) can now create a vault on behalf of a user who hasn't signed yet. The vault is keyed to the beneficiary's address from day one — the sponsor only pays and signs at creation; all subsequent vault operations (authorize issuers, receive credentials, etc.) are controlled by the owner.Changes
error.rs— AddedNotAuthorizedSponsor = 11error code so integrators can distinguish authorization failures.storage/mod.rs— AddedSponsoredVaultOpenToAllandSponsoredVaultSponsorsstorage keys with all read/write helpers.api/mod.rs— Added 5 new function signatures toVcVaultTrait.contract.rs— Implemented:create_sponsored_vault(sponsor, owner, did_uri)— sponsor signs, vault is created for owner.set_sponsored_vault_open_to_all(open: bool)— admin toggles restricted vs open mode.get_sponsored_vault_open_to_all()— query current mode.add_sponsored_vault_sponsor(sponsor)— admin adds an authorized sponsor.remove_sponsored_vault_sponsor(sponsor)— admin removes a sponsor.test.rs— 8 new tests covering all flows described in the issue.Behavior
create_sponsored_vaultopen_to_all = true)Test coverage
open_to_alldefaults tofalseCloses
Summary by CodeRabbit
New Features
Chores
Tests