feat: vault factory POC — single-tenant vc-vault + vc-vault-factory#52
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 26 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new vc-vault-factory crate and contract for deterministic single-tenant vault deployment, and refactors vc-vault into single-vault-per-contract: constructor-stored owner, owner-less storage keys, simplified events/API, and removal of sponsored/linked-vault features. ChangesNew Vault Factory
VC Vault Single-Instance Refactoring
Sequence Diagram — Factory Deployment Flow sequenceDiagram
participant Caller
participant VaultFactory
participant Deployer
participant VaultInstance
Caller->>VaultFactory: deploy(owner, did_uri, user_salt)
VaultFactory->>VaultFactory: keccak256(user_salt || owner)
VaultFactory->>Deployer: deploy_v2(vault_wasm_hash, init_args, salt)
Deployer->>VaultInstance: instantiate
VaultFactory->>VaultFactory: set_deployed(vault_address)
VaultFactory->>Caller: return vault_address
🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/vc-vault/src/storage/issuer.rs (1)
198-206: 💤 Low valueConsider adding a limit check for denied issuers.
append_denied_issuer_to_indexlacks theMAX_ISSUERS_LISTcheck thatappend_issuer_to_indexhas at line 84. While the denied list is typically smaller, this allows unbounded storage growth if many issuers are denied over time.Suggested fix
pub fn append_denied_issuer_to_index(e: &Env, issuer: &Address) { if denied_issuer_index_contains(e, issuer) { return; } let count = read_denied_issuer_count(e); + if count >= MAX_ISSUERS_LIST { + panic_with_error!(e, ContractError::IssuerListTooLong); + } write_denied_issuer_at(e, count, issuer); write_denied_issuer_position(e, issuer, count); write_denied_issuer_count(e, count + 1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/vc-vault/src/storage/issuer.rs` around lines 198 - 206, append_denied_issuer_to_index allows unbounded growth; add the same MAX_ISSUERS_LIST check used by append_issuer_to_index: read the current count with read_denied_issuer_count(e) and if count >= MAX_ISSUERS_LIST, bail out (return or revert as the contract pattern requires) before writing; keep the existing denied_issuer_index_contains check and only perform write_denied_issuer_at, write_denied_issuer_position, and write_denied_issuer_count when the count is below MAX_ISSUERS_LIST.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/vc-vault-factory/src/test.rs`:
- Around line 163-180: The test function
test_deploy_and_deploy_sponsored_same_owner_same_salt_same_address is
inconsistent with its assertions: it declares the intent "same owner + same salt
+ same address" but creates owner2 and asserts the two derived addresses differ;
either restore the intended same-owner scenario by using the original owner for
the deploy_sponsored call (replace owner2 with owner and change addr_sponsored =
client.deploy_sponsored(&deployer, &owner, &did_uri, &salt) and
assert_eq!(addr_normal, addr_sponsored)), or rename the test and its comment to
reflect "different owner -> different address" and keep owner2 and assert_ne;
update the test name and any inline comment accordingly and ensure the
assertions match client.deploy and client.deploy_sponsored behavior.
---
Nitpick comments:
In `@contracts/vc-vault/src/storage/issuer.rs`:
- Around line 198-206: append_denied_issuer_to_index allows unbounded growth;
add the same MAX_ISSUERS_LIST check used by append_issuer_to_index: read the
current count with read_denied_issuer_count(e) and if count >= MAX_ISSUERS_LIST,
bail out (return or revert as the contract pattern requires) before writing;
keep the existing denied_issuer_index_contains check and only perform
write_denied_issuer_at, write_denied_issuer_position, and
write_denied_issuer_count when the count is below MAX_ISSUERS_LIST.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 838de906-b5f4-4047-ae12-335df3aaba17
📒 Files selected for processing (22)
Cargo.tomlcontracts/vc-vault-factory/Cargo.tomlcontracts/vc-vault-factory/src/contract.rscontracts/vc-vault-factory/src/events.rscontracts/vc-vault-factory/src/lib.rscontracts/vc-vault-factory/src/storage.rscontracts/vc-vault-factory/src/test.rscontracts/vc-vault/src/contract.rscontracts/vc-vault/src/error.rscontracts/vc-vault/src/events.rscontracts/vc-vault/src/interface.rscontracts/vc-vault/src/storage/credential.rscontracts/vc-vault/src/storage/issuer.rscontracts/vc-vault/src/storage/mod.rscontracts/vc-vault/src/storage/sponsor.rscontracts/vc-vault/src/storage/ttl.rscontracts/vc-vault/src/storage/vault.rscontracts/vc-vault/src/test.rscontracts/vc-vault/src/validator.rscontracts/vc-vault/src/vault/credential.rscontracts/vc-vault/src/vault/issuer.rscontracts/vc-vault/src/vault/mod.rs
💤 Files with no reviewable changes (2)
- contracts/vc-vault/src/storage/sponsor.rs
- contracts/vc-vault/src/error.rs
Each vault is now its own contract, so no key needs to be scoped by owner address. VaultOwner is written once at construction and read from instance storage. sponsor.rs removed — sponsored vault deferred.
All require_* helpers now read vault state from instance storage instead of taking owner: &Address. require_vault_admin reads VaultAdmin directly; require_vault_active reads VaultRevoked.
store_vc, store_vc_with_fee, revoke_vc, authorize_issuer, authorize_issuers, revoke_issuer and is_authorized no longer take owner: &Address. push_vc removed — cross-contract push is deferred.
… entrypoints __constructor(vault_owner, contract_admin, did_uri) replaces create_vault. All public functions drop owner: Address. Removed: create_vault, create_sponsored_vault, sponsor management, push, issue_linked, get_vc_parent. Dead error codes VaultAlreadyExists and NotAuthorizedSponsor removed.
Constructor registers via env.register(VcVaultContract, (owner, admin, did_uri)). All client calls drop the owner first arg. Removed tests for create_vault, sponsored vault, push, issue_linked and get_vc_parent. 93 tests passing.
Factory deploys single-tenant vc-vault instances via deploy_v2. Salt = keccak256(user_salt || owner_bytes) prevents frontrunning and makes vault addresses deterministic per (owner, salt) pair. deploy(owner, did_uri, salt) — owner signs their own vault. deploy_sponsored(deployer, owner, did_uri, salt) — any third party signs and pays; vault ownership goes to owner from creation. is_vault(address) queries the persistent registry of deployed vaults. 10 tests passing including full integration and sponsored flow.
Adds VaultFactory persistent key to store the factory that deployed each vault, enabling cross-vault source verification in receive_push.
push() moves a VC from the source vault to a destination vault via a cross-contract call. receive_push() verifies the source is a legitimate factory-deployed vault before accepting the credential and sets itself as the new issuance authority.
aguilar1x
force-pushed
the
feat/vault-factory-poc
branch
from
c3c58f4 to
fe78dab
Compare
Resolves Cargo.toml conflict by keeping vc-vault-factory in workspace members and dropping vc-issuer-registry (already removed in dev).
The vc-vault-factory test suite uses contractimport! to load vc_vault_contract WASM at compile time. CI was failing with "No such file or directory" because the workflow ran cargo test without first producing the wasm32v1-none artifact. Two changes: - Add rustup target install and a cargo rustc step to build vc-vault as cdylib for wasm32v1-none before tests run. - Switch the contractimport! path from .optimized.wasm to the plain .wasm so CI doesn't need to install the stellar CLI / wasm-opt just to run tests. Optimization is only relevant for deploy; Soroban's sandbox executes either binary identically.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/vc-vault-factory/src/contract.rs`:
- Around line 34-40: In derive_salt, don't use owner.to_string() into a fixed
56-byte buffer; instead serialize the Address to its canonical bytes and hash
those with user_salt (so the salt = keccak256(user_salt ||
canonical_address_bytes)). Locate derive_salt and replace the owner.to_string()
copy logic with the Address XDR/raw payload serialization (e.g., call the
Address XDR/ToXdr or extract an AddressPayload/raw bytes), append those
canonical bytes to the user_salt bytes, and then keccak256 that combined byte
array so the computed salt matches the README/client-side preimages.
In `@contracts/vc-vault/src/contract.rs`:
- Around line 429-431: The duplicate VC check currently uses
storage::vc_index_contains(&e, &vc_id) which misses revoked-but-removed-index
cases; replace it with the same logic used in issue()/batch_issue(): if
storage::read_vault_vc(&e, &vc_id).is_some() || storage::read_vc_status(&e,
&vc_id) != VCStatus::Invalid { panic_with_error!(e,
ContractError::VCAlreadyExists); } — this ensures receive_push (or the function
containing the vc_index_contains check) rejects VCs that already exist or are
revoked.
- Around line 414-418: Add the missing issuer DID length check in receive_push:
call require_issuer_did_len(&e, &issuer_did) (the same validation used in
issue()) before storing the VC (i.e., after require_vc_data_len(&e, &vc_data)
and before proceeding with require_vault_active/source_vault.require_auth). This
ensures issuer_did is validated consistently with issue().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60213be4-bb29-4e9a-a45c-1572c9c0ac9d
📒 Files selected for processing (23)
Cargo.tomlcontracts/vc-vault-factory/Cargo.tomlcontracts/vc-vault-factory/README.mdcontracts/vc-vault-factory/src/contract.rscontracts/vc-vault-factory/src/events.rscontracts/vc-vault-factory/src/lib.rscontracts/vc-vault-factory/src/storage.rscontracts/vc-vault-factory/src/test.rscontracts/vc-vault/src/contract.rscontracts/vc-vault/src/error.rscontracts/vc-vault/src/events.rscontracts/vc-vault/src/interface.rscontracts/vc-vault/src/storage/credential.rscontracts/vc-vault/src/storage/issuer.rscontracts/vc-vault/src/storage/mod.rscontracts/vc-vault/src/storage/sponsor.rscontracts/vc-vault/src/storage/ttl.rscontracts/vc-vault/src/storage/vault.rscontracts/vc-vault/src/test.rscontracts/vc-vault/src/validator.rscontracts/vc-vault/src/vault/credential.rscontracts/vc-vault/src/vault/issuer.rscontracts/vc-vault/src/vault/mod.rs
💤 Files with no reviewable changes (1)
- contracts/vc-vault/src/storage/sponsor.rs
✅ Files skipped from review due to trivial changes (1)
- contracts/vc-vault-factory/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (13)
- Cargo.toml
- contracts/vc-vault-factory/src/lib.rs
- contracts/vc-vault/src/vault/mod.rs
- contracts/vc-vault/src/validator.rs
- contracts/vc-vault/src/vault/issuer.rs
- contracts/vc-vault/src/storage/mod.rs
- contracts/vc-vault/src/storage/ttl.rs
- contracts/vc-vault/src/events.rs
- contracts/vc-vault/src/storage/issuer.rs
- contracts/vc-vault/src/vault/credential.rs
- contracts/vc-vault/src/interface.rs
- contracts/vc-vault/src/storage/vault.rs
- contracts/vc-vault/src/storage/credential.rs
Comment on lines
+34
to
+40
| fn derive_salt(e: &Env, user_salt: BytesN<32>, owner: &Address) -> BytesN<32> { | ||
| // Salt = keccak256(user_salt || owner_bytes) — prevents frontrunning. | ||
| let mut owner_bytes: [u8; 56] = [0; 56]; | ||
| owner.to_string().copy_into_slice(&mut owner_bytes); | ||
| let mut salt_bytes: Bytes = user_salt.into_val(e); | ||
| salt_bytes.extend_from_array(&owner_bytes); | ||
| e.crypto().keccak256(&salt_bytes).into() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the current Soroban SDK, what is the canonical byte/XDR serialization API for Addresswhen hashing deterministically, and isto_string() appropriate for stable on-chain address derivation or should raw serialized bytes be used instead?
💡 Result:
In the Soroban SDK, the canonical way to serialize an Address for deterministic hashing is by using its XDR representation, typically accessed via the to_xdr() method provided by the soroban_sdk::xdr::ToXdr trait [1]. Using to_string() is not recommended for on-chain address derivation or stable hashing [1]. The to_string() method produces a human-readable StrKey (e.g., starting with 'G' or 'C'), which is designed for display and off-chain interoperability rather than stable, canonical binary identification [1]. Relying on string representations introduces unnecessary complexity and potential inconsistencies if the encoding format were to change or if different string representations were used. For stable on-chain derivation and deterministic hashing, you should use the raw XDR bytes [1]. The Address type can be serialized to XDR bytes using address.to_xdr(&env), providing a stable, canonical binary format suitable for hashing and storage [1]. If you need to access the underlying components of an address (such as the 32-byte contract hash or Ed25519 public key), you can use the AddressPayload type, which can be extracted from an Address to obtain the raw binary identifier directly [2][3][4]. Top results: [2][3][1]
Citations:
- 1: https://developers.stellar.org/docs/build/guides/conversions/address-conversions
- 2: https://docs.rs/soroban-sdk/latest/src/soroban_sdk/address.rs.html
- 3: https://docs.rs/soroban-sdk/latest/src/soroban_sdk/address_payload.rs.html
- 4: https://docs.rs/soroban-sdk/latest/soroban_sdk/address_payload/index.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant contract code region
if [ -f contracts/vc-vault-factory/src/contract.rs ]; then
echo "== contract.rs derive_salt =="
nl -ba contracts/vc-vault-factory/src/contract.rs | sed -n '1,120p' | sed -n '34,60p'
else
echo "Missing contracts/vc-vault-factory/src/contract.rs"
fi
# Print README section that documents salt derivation (best-effort)
if [ -f contracts/vc-vault-factory/README.md ]; then
echo "== README salt derivation section (best-effort) =="
rg -n "salt|keccak|derive_salt|user_salt|owner_bytes|owner" contracts/vc-vault-factory/README.md | head -n 50
# show around matches
rg -n "salt|keccak|derive_salt|user_salt|owner_bytes|owner" contracts/vc-vault-factory/README.md | head -n 10 | cut -d: -f1 | sort -u | while read -r ln; do
echo "-- context around line $ln --"
nl -ba contracts/vc-vault-factory/README.md | sed -n "$((ln-5)),$((ln+15))p"
done
else
echo "Missing contracts/vc-vault-factory/README.md"
fiRepository: ACTA-Team/contracts-acta
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="contracts/vc-vault-factory/src/contract.rs"
README="contracts/vc-vault-factory/README.md"
echo "== Checking file existence =="
echo "contract.rs: $(test -f "$FILE" && echo yes || echo no)"
echo "README.md: $(test -f "$README" && echo yes || echo no)"
echo
echo "== contract.rs derive_salt (lines 25-60) =="
if [ -f "$FILE" ]; then
awk 'NR>=25 && NR<=60 {printf "%d:%s\n", NR, $0}' "$FILE" | sed -n '1,80p'
else
echo "Missing $FILE"
fi
echo
echo "== README salt-related documentation (best-effort) =="
if [ -f "$README" ]; then
rg -n "derive_salt|keccak|salt|user_salt|owner_bytes|owner_bytes|owner" "$README" | head -n 50 || true
# If there are matches, print a small window around the first few match lines
matches=$(rg -n "derive_salt|keccak|salt|user_salt|owner_bytes|owner" "$README" | head -n 10 | cut -d: -f1 | tr '\n' ' ')
if [ -n "${matches:-}" ]; then
for ln in $matches; do
echo "-- context around README line $ln --"
awk -v start=$((ln-8)) -v end=$((ln+20)) 'NR>=start && NR<=end {printf "%d:%s\n", NR, $0}' "$README"
echo
done
fi
else
echo "Missing $README"
fiRepository: ACTA-Team/contracts-acta
Length of output: 15423
Derive deploy salt from canonical Address bytes (XDR/payload), not owner.to_string()
contracts/vc-vault-factory/src/contract.rs builds owner_bytes by copying owner.to_string() (StrKey text) into a fixed [u8; 56] buffer and hashing it. This doesn’t match contracts/vc-vault-factory/README.md, which defines deploy_salt = keccak256(user_salt || owner_address_bytes) and implies raw/canonical address bytes. The current approach ties determinism to today’s display-string encoding/length and can cause client-side precomputed vault addresses to differ from on-chain results.
Use the canonical serialized address bytes for hashing (e.g., XDR via to_xdr(&env) / ToXdr, or raw bytes via AddressPayload), then hash those bytes with user_salt.
🧰 Tools
🪛 Clippy (1.96.0)
[warning] 34-34: function derive_salt is never used
(warning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/vc-vault-factory/src/contract.rs` around lines 34 - 40, In
derive_salt, don't use owner.to_string() into a fixed 56-byte buffer; instead
serialize the Address to its canonical bytes and hash those with user_salt (so
the salt = keccak256(user_salt || canonical_address_bytes)). Locate derive_salt
and replace the owner.to_string() copy logic with the Address XDR/raw payload
serialization (e.g., call the Address XDR/ToXdr or extract an AddressPayload/raw
bytes), append those canonical bytes to the user_salt bytes, and then keccak256
that combined byte array so the computed salt matches the README/client-side
preimages.
receive_push only checked vc_index_contains for duplicates. revoke() removes the index entry but the Revoked status persists, so a pushed VC reusing a revoked vc_id passed the check and silently overwrote the status back to Valid — undoing a revocation. This is the same class as audit finding A-26. Align the duplicate guard with issue()/batch_issue() by checking the persisted status (read_vault_vc/read_vc_status) instead of the index. Also add the missing require_issuer_did_len validation so receive_push validates inputs consistently with issue(). Adds a regression test (test_push_cannot_revive_revoked_vc_in_destination) verified to fail against the pre-fix build and pass after.
The test was named ..._same_owner_same_salt_same_address but used two different owners and asserted the addresses differ. Rename to reflect what it actually verifies (different owners -> different addresses via both deploy and deploy_sponsored) and correct the comment. The same-owner collapse can't be asserted via double-deploy since the second deploy of an identical (owner, salt) pair panics.
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
1 participant
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
vc-vaultfrom multi-tenant to single-tenant: each holder gets their own deployed contract instance,owner: Addressremoved from all public functions, owner stored at constructionvc-vault-factorycontract: deploys vault instances deterministically viadeploy_v2, registers them, and exposesis_vaultfor cross-contract verificationdeploy_sponsored: anyone can deploy a vault on behalf of an owner (deployer signs/pays, vault belongs to owner from creation)push/receive_push: production-ready cross-vault VC transfer — source vault calls destination'sreceive_push, which verifies the source is a legitimate factory vault before accepting the credentialChanges
contracts/vc-vault(vault_owner, contract_admin, did_uri, factory_address)create_vault,create_sponsored_vault, sponsored-vault whitelist functionspush(vc_id, dest_vault),receive_push(source_vault, vc_id, vc_data, issuer_did)VaultOwnerandVaultFactorypersistent keys; all keys flattened (owner no longer part of key)contracts/vc-vault-factorydeploy(owner, did_uri, salt)— owner signsdeploy_sponsored(deployer, owner, did_uri, salt)— deployer signs, vault belongs to owneris_vault(address)— registry lookup used byreceive_pushfor source validationSalt = keccak256(user_salt ‖ owner_bytes) to prevent frontrunning
Closes feat: vc-vault-factory POC — single-tenant vaults deployed per holder #54
Summary by CodeRabbit
Release Notes
New Features
Changes
Removed