feat: add vc-revocation-registry contract and unit tests#9
Conversation
Implements the vc-revocation-registry Soroban contract with full unit test coverage, closing issue ACTA-Team#4. Contract entry points: - initialize(admin): one-time init, emits Initialized event - revoke(issuer, vc_id, reason): issuer-authed, emits Revoked event - batch_revoke(issuer, vc_ids, reason): atomic batch, one Revoked per VC - unrevoke(vc_id): admin-only, emits Unrevoked event - is_revoked(vc_id), get_revocation(vc_id): read-only queries - admin(), version(): read-only helpers Tests cover: - All happy paths and error paths - All error variants (AlreadyInitialized, NotInitialized, AlreadyRevoked, NotRevoked, auth rejection) - All emitted events asserted - batch_revoke atomicity (rolls back if any VC already revoked) Closes ACTA-Team#4
📝 WalkthroughWalkthroughThis PR introduces a new Soroban smart contract crate ChangesVC Revocation Registry Contract
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 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 |
|
@temi-Dee 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! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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-revocation-registry/src/contract.rs`:
- Around line 61-75: The batch revocation logic in the contract’s revocation
entrypoint should also reject duplicate VC IDs within the same request, not just
IDs already present in storage. Update the pre-validation around the existing
`for vc_id in vc_ids.iter()` checks to track seen IDs and panic with an error if
the same `vc_id` appears twice before any call to `storage::write_revocation` or
`events::revoked`. Use the existing revocation flow in `contract.rs` to keep
atomicity and prevent duplicate writes and duplicate Revoked events.
- Around line 41-44: The revocation entrypoints in `Contract::revoke` and the
related flow at the `issuer.require_auth()` call only prove the caller controls
the supplied address, not that the address is the actual issuer of `vc_id`. Add
a real issuer-to-credential binding check before writing revocation state, using
an existing registry lookup or a stored mapping keyed by `vc_id`, and reject
mismatches with `ContractError::UnauthorizedIssuer`. Make sure the same
authorization check is applied anywhere else revocation is gated so
`UnauthorizedIssuer` is actually reachable only when the caller is the
legitimate issuer.
In `@contracts/vc-revocation-registry/src/storage.rs`:
- Around line 44-58: The revocation lookup helpers only read the persistent
Revoked entry and never refresh its TTL, so the record can expire even while it
is still being accessed. Update the storage paths in has_revocation and
read_revocation to extend the TTL on the same DataKey::Revoked key whenever it
is checked or fetched, following the same pattern already used in
write_revocation. Keep the TTL refresh logic centralized around the existing
storage helpers so revoked records stay alive as long as they are referenced.
In `@contracts/vc-revocation-registry/src/test.rs`:
- Around line 176-190: The atomicity test in
test_batch_revoke_rolls_back_if_any_already_revoked only checks that
batch_revoke panics, so it can miss partial writes. Remove the should_panic
behavior, catch the failure from client.batch_revoke, then verify via the
revocation lookup and event assertions that id1 stays revoked, id2 was not
revoked, and no extra Revoked events were emitted after the rollback.
- Around line 36-44: The event assertions in the tests are too weak because they
only check that something was emitted; update the tests in
test_initialize_emits_event and the related revoke/unrevoke cases to assert the
exact contract event payloads produced by the client methods. Use the event
shapes defined in events.rs and compare against the specific Initialized,
Revoked, and Unrevoked events emitted by setup(), initialize(), revoke(), and
unrevoke(), rather than relying on e.events().all() non-emptiness or total
count.
- Around line 46-54: The negative-path tests around initialize, revoke/restore,
and auth checks are only asserting that some failure happened, not that the
contract returned the intended variant. Update the affected tests in the
relevant test functions (for example, the initialize panic case and the other
error-path tests) to capture the returned panic/error payload from the client
methods and assert the exact variant such as AlreadyInitialized, AlreadyRevoked,
NotInitialized, NotRevoked, or the expected auth failure, instead of using bare
#[should_panic] or generic is_err(). Use the existing test helper/setup and the
client methods under test to verify the precise failure path.
🪄 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: c9d7595c-12c1-4567-8407-09c77b217ec0
📒 Files selected for processing (8)
Cargo.tomlcontracts/vc-revocation-registry/Cargo.tomlcontracts/vc-revocation-registry/src/contract.rscontracts/vc-revocation-registry/src/error.rscontracts/vc-revocation-registry/src/events.rscontracts/vc-revocation-registry/src/lib.rscontracts/vc-revocation-registry/src/storage.rscontracts/vc-revocation-registry/src/test.rs
| pub fn revoke(e: Env, issuer: Address, vc_id: BytesN<32>, reason: Option<Symbol>) { | ||
| require_initialized(&e); | ||
| issuer.require_auth(); | ||
| if storage::has_revocation(&e, &vc_id) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Issuer authorization is not actually enforced.
Line 43 and Line 60 only authenticate the address passed in as issuer; they do not verify that this address is the legitimate/original issuer for vc_id. As written, any signer can revoke any arbitrary credential ID first by supplying their own address, and ContractError::UnauthorizedIssuer is unreachable. This needs a real issuer-to-VC binding (or external registry check) before these entrypoints can claim issuer-gated revocation.
Also applies to: 58-60
🤖 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-revocation-registry/src/contract.rs` around lines 41 - 44, The
revocation entrypoints in `Contract::revoke` and the related flow at the
`issuer.require_auth()` call only prove the caller controls the supplied
address, not that the address is the actual issuer of `vc_id`. Add a real
issuer-to-credential binding check before writing revocation state, using an
existing registry lookup or a stored mapping keyed by `vc_id`, and reject
mismatches with `ContractError::UnauthorizedIssuer`. Make sure the same
authorization check is applied anywhere else revocation is gated so
`UnauthorizedIssuer` is actually reachable only when the caller is the
legitimate issuer.
| // Validate all first to ensure atomicity | ||
| for vc_id in vc_ids.iter() { | ||
| if storage::has_revocation(&e, &vc_id) { | ||
| panic_with_error!(&e, ContractError::AlreadyRevoked); | ||
| } | ||
| } | ||
| for vc_id in vc_ids.iter() { | ||
| let record = RevocationRecord { | ||
| issuer: issuer.clone(), | ||
| revoked_at: e.ledger().timestamp(), | ||
| reason: reason.clone(), | ||
| }; | ||
| storage::write_revocation(&e, &vc_id, &record); | ||
| events::revoked(&e, &vc_id, &issuer); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject duplicate VC IDs inside the same batch.
The pre-validation loop only checks current storage. A payload such as [vc_id, vc_id] passes, then the second iteration rewrites the same key and emits a second Revoked event for the same VC. If batch atomicity is supposed to fail when any item is already revoked, you also need an in-batch uniqueness check before mutating state.
🤖 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-revocation-registry/src/contract.rs` around lines 61 - 75, The
batch revocation logic in the contract’s revocation entrypoint should also
reject duplicate VC IDs within the same request, not just IDs already present in
storage. Update the pre-validation around the existing `for vc_id in
vc_ids.iter()` checks to track seen IDs and panic with an error if the same
`vc_id` appears twice before any call to `storage::write_revocation` or
`events::revoked`. Use the existing revocation flow in `contract.rs` to keep
atomicity and prevent duplicate writes and duplicate Revoked events.
| pub fn has_revocation(e: &Env, vc_id: &BytesN<32>) -> bool { | ||
| e.storage().persistent().has(&DataKey::Revoked(vc_id.clone())) | ||
| } | ||
|
|
||
| pub fn read_revocation(e: &Env, vc_id: &BytesN<32>) -> Option<RevocationRecord> { | ||
| e.storage().persistent().get(&DataKey::Revoked(vc_id.clone())) | ||
| } | ||
|
|
||
| pub fn write_revocation(e: &Env, vc_id: &BytesN<32>, record: &RevocationRecord) { | ||
| let key = DataKey::Revoked(vc_id.clone()); | ||
| e.storage().persistent().set(&key, record); | ||
| e.storage() | ||
| .persistent() | ||
| .extend_ttl(&key, PERSISTENT_TTL_THRESHOLD, PERSISTENT_TTL_EXTEND_TO); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Refresh persistent revocation TTL on reads/checks.
Line 44 and Line 48 only read the Revoked key; they never extend its TTL. After ~PERSISTENT_TTL_EXTEND_TO ledgers, an otherwise still-revoked VC ages out and starts behaving as unrevoked (is_revoked == false, get_revocation panics, duplicate revoke becomes allowed). The TTL refresh needs to happen on the persistent key whenever it is accessed, not just when it is first written.
Suggested direction
pub fn has_revocation(e: &Env, vc_id: &BytesN<32>) -> bool {
- e.storage().persistent().has(&DataKey::Revoked(vc_id.clone()))
+ let key = DataKey::Revoked(vc_id.clone());
+ let exists = e.storage().persistent().has(&key);
+ if exists {
+ e.storage()
+ .persistent()
+ .extend_ttl(&key, PERSISTENT_TTL_THRESHOLD, PERSISTENT_TTL_EXTEND_TO);
+ }
+ exists
}
pub fn read_revocation(e: &Env, vc_id: &BytesN<32>) -> Option<RevocationRecord> {
- e.storage().persistent().get(&DataKey::Revoked(vc_id.clone()))
+ let key = DataKey::Revoked(vc_id.clone());
+ let record = e.storage().persistent().get(&key);
+ if record.is_some() {
+ e.storage()
+ .persistent()
+ .extend_ttl(&key, PERSISTENT_TTL_THRESHOLD, PERSISTENT_TTL_EXTEND_TO);
+ }
+ record
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn has_revocation(e: &Env, vc_id: &BytesN<32>) -> bool { | |
| e.storage().persistent().has(&DataKey::Revoked(vc_id.clone())) | |
| } | |
| pub fn read_revocation(e: &Env, vc_id: &BytesN<32>) -> Option<RevocationRecord> { | |
| e.storage().persistent().get(&DataKey::Revoked(vc_id.clone())) | |
| } | |
| pub fn write_revocation(e: &Env, vc_id: &BytesN<32>, record: &RevocationRecord) { | |
| let key = DataKey::Revoked(vc_id.clone()); | |
| e.storage().persistent().set(&key, record); | |
| e.storage() | |
| .persistent() | |
| .extend_ttl(&key, PERSISTENT_TTL_THRESHOLD, PERSISTENT_TTL_EXTEND_TO); | |
| } | |
| pub fn has_revocation(e: &Env, vc_id: &BytesN<32>) -> bool { | |
| let key = DataKey::Revoked(vc_id.clone()); | |
| let exists = e.storage().persistent().has(&key); | |
| if exists { | |
| e.storage() | |
| .persistent() | |
| .extend_ttl(&key, PERSISTENT_TTL_THRESHOLD, PERSISTENT_TTL_EXTEND_TO); | |
| } | |
| exists | |
| } | |
| pub fn read_revocation(e: &Env, vc_id: &BytesN<32>) -> Option<RevocationRecord> { | |
| let key = DataKey::Revoked(vc_id.clone()); | |
| let record = e.storage().persistent().get(&key); | |
| if record.is_some() { | |
| e.storage() | |
| .persistent() | |
| .extend_ttl(&key, PERSISTENT_TTL_THRESHOLD, PERSISTENT_TTL_EXTEND_TO); | |
| } | |
| record | |
| } | |
| pub fn write_revocation(e: &Env, vc_id: &BytesN<32>, record: &RevocationRecord) { | |
| let key = DataKey::Revoked(vc_id.clone()); | |
| e.storage().persistent().set(&key, record); | |
| e.storage() | |
| .persistent() | |
| .extend_ttl(&key, PERSISTENT_TTL_THRESHOLD, PERSISTENT_TTL_EXTEND_TO); | |
| } |
🤖 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-revocation-registry/src/storage.rs` around lines 44 - 58, The
revocation lookup helpers only read the persistent Revoked entry and never
refresh its TTL, so the record can expire even while it is still being accessed.
Update the storage paths in has_revocation and read_revocation to extend the TTL
on the same DataKey::Revoked key whenever it is checked or fetched, following
the same pattern already used in write_revocation. Keep the TTL refresh logic
centralized around the existing storage helpers so revoked records stay alive as
long as they are referenced.
| fn test_initialize_emits_event() { | ||
| let (e, client) = setup(); | ||
| let admin = Address::generate(&e); | ||
|
|
||
| client.initialize(&admin); | ||
|
|
||
| // Verify the Initialized event was published (env recorded it) | ||
| assert!(!e.events().all().is_empty(), "Initialized event must be emitted"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the emitted event itself, not just that “some event” exists.
These checks are too weak to prove Initialized / Revoked / Unrevoked were emitted with the right payload. In particular, Line 101 can already pass because Line 98 emitted Initialized, even if revoke stops publishing Revoked. Please compare the specific emitted event(s) against the expected contract event shape from events.rs, not just buffer non-emptiness or total count.
Also applies to: 92-102, 162-174, 213-226
🤖 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-revocation-registry/src/test.rs` around lines 36 - 44, The event
assertions in the tests are too weak because they only check that something was
emitted; update the tests in test_initialize_emits_event and the related
revoke/unrevoke cases to assert the exact contract event payloads produced by
the client methods. Use the event shapes defined in events.rs and compare
against the specific Initialized, Revoked, and Unrevoked events emitted by
setup(), initialize(), revoke(), and unrevoke(), rather than relying on
e.events().all() non-emptiness or total count.
| #[test] | ||
| #[should_panic] | ||
| fn test_initialize_panics_when_called_twice() { | ||
| let (e, client) = setup(); | ||
| let admin = Address::generate(&e); | ||
|
|
||
| client.initialize(&admin); | ||
| client.initialize(&admin); // must panic with AlreadyInitialized | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the exact failure variant in the negative-path tests.
#[should_panic] and bare is_err() will pass on any panic/invocation failure, so these tests do not actually prove the contract hit AlreadyInitialized, AlreadyRevoked, NotInitialized, NotRevoked, or the intended auth failure path. The linked acceptance criteria call for exercising each explicit error variant, so capture and assert the returned panic/error payload instead of accepting any failure.
Also applies to: 104-125, 127-139, 228-252, 293-313
🤖 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-revocation-registry/src/test.rs` around lines 46 - 54, The
negative-path tests around initialize, revoke/restore, and auth checks are only
asserting that some failure happened, not that the contract returned the
intended variant. Update the affected tests in the relevant test functions (for
example, the initialize panic case and the other error-path tests) to capture
the returned panic/error payload from the client methods and assert the exact
variant such as AlreadyInitialized, AlreadyRevoked, NotInitialized, NotRevoked,
or the expected auth failure, instead of using bare #[should_panic] or generic
is_err(). Use the existing test helper/setup and the client methods under test
to verify the precise failure path.
| #[test] | ||
| #[should_panic] | ||
| fn test_batch_revoke_rolls_back_if_any_already_revoked() { | ||
| let (e, client) = setup(); | ||
| let admin = Address::generate(&e); | ||
| let issuer = Address::generate(&e); | ||
| let id1 = vc_id(&e, 30); | ||
| let id2 = vc_id(&e, 31); | ||
|
|
||
| client.initialize(&admin); | ||
| client.revoke(&issuer, &id1, &None); // id1 already revoked | ||
|
|
||
| let ids = Vec::from_array(&e, [id1, id2]); | ||
| client.batch_revoke(&issuer, &ids, &None); // must panic with AlreadyRevoked | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The atomicity test never verifies rollback.
Right now this only proves batch_revoke fails. If the implementation partially wrote id2 before panicking, the test would still pass. Please capture the failure without #[should_panic], then assert that id1 remains revoked, id2 is still not revoked, and no extra Revoked events were recorded.
🤖 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-revocation-registry/src/test.rs` around lines 176 - 190, The
atomicity test in test_batch_revoke_rolls_back_if_any_already_revoked only
checks that batch_revoke panics, so it can miss partial writes. Remove the
should_panic behavior, catch the failure from client.batch_revoke, then verify
via the revocation lookup and event assertions that id1 stays revoked, id2 was
not revoked, and no extra Revoked events were emitted after the rollback.
1 participant
Implements the vc-revocation-registry Soroban contract and comprehensive unit tests.
Changes
Closes #4
Summary by CodeRabbit
New Features
Bug Fixes
Tests