feat: IPFS credential storage with preliminary credentials and gateway finalization#121
feat: IPFS credential storage with preliminary credentials and gateway finalization#121sapience wants to merge 31 commits into
Conversation
…_by_id_as_gateway and finalize_credentials_by_dwg_as_gateway actions Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…t flow. Credentials: - Rename content_manifest to content_uri; add content_size (positive when set). - Replace inserter with inserter_type and inserter_id. Preliminary credentials: - Rename *content_manifest columns to *content_uri; add original/copy content_size. - Store inserter_type and inserter_id (required) on preliminary rows. Actions: - Drop add_credential, edit_credential, upsert_credential_as_inserter; add create_preliminary_credential with id/URI/size checks and signature verify. - Harden share and DWG preliminary paths: credential_id_in_use, validate_content_uri, content_uri_in_use, positive sizes, distinct original/copy URIs for DWG. - Rename finalize_credentials_by_dwg_as_gateway to finalize_credentials_as_gateway; persist content_uri, content_size, inserter_type, inserter_id on finalize. - Extend get_credentials*, get_credential_owned/shared, get_prelim_credential_as_gateway. Schema: - Remove consumed_write_grants foreign keys to credentials for original/copy ids. - Add private helpers credential_id_in_use and content_uri_in_use.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used🪛 GitHub Actions: CI / 0_cspell.txtschema.sql[error] 776-776: cspell reported an unknown word: 'pkoch' at schema.sql:776:12. 🪛 GitHub Actions: CI / cspellschema.sql[error] 776-776: cspell found an unknown word 'pkoch' at schema.sql:776:12. Files checked: 29, Issues found: 1 in 1 file. 🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe PR adds ChangesGenerator
IPFS credential staging and gateway flow
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 |
sapience
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
generator/src/parser/index.ts (1)
82-102: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAccumulator lacks default values for interface fields.
The reduce initializer
{} as GeneratorCommentscreates an empty object, butGeneratorCommentsdeclares required fields (ignore,notAuthorized,description,paramOptional,returnOptional). When no@generator.*comments are present, accessinggeneratorComments.ignoreinindex.tswill returnundefinedrather thanfalse.This works by accident because
!undefinedis truthy in the filter!x.generatorComments.ignore, but it's fragile and inconsistent with the interface contract.♻️ Proposed fix: Initialize accumulator with default values
}, {} as GeneratorComments); + }, { + ignore: false, + notAuthorized: false, + description: "", + paramOptional: [], + returnOptional: [], + } as GeneratorComments);🤖 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 `@generator/src/parser/index.ts` around lines 82 - 102, The reduce function initializes the accumulator with an empty object cast to GeneratorComments type, which causes the interface fields (ignore, notAuthorized, description, paramOptional, returnOptional) to be undefined when no generator comments are present. Replace the empty object initializer `{} as GeneratorComments` with an object that provides proper default values for all required fields in the GeneratorComments interface, such as setting boolean fields to false and string fields to empty strings or appropriate defaults, so that the interface contract is properly maintained throughout the parsing logic.schema.sql (1)
139-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a migration to drop legacy
consumed_write_grantscredential FKs on upgraded databases.Line 148 reflects the new no-FK table shape, but none of the
migrations/ipfs/*.sqlfiles drops pre-existing FK constraints. Upgraded environments can keep old constraints and block credential deletions/finalization flows.Suggested follow-up migration
+-- migrations/ipfs/8.drop_consumed_write_grants_fks.sql +ALTER TABLE consumed_write_grants + DROP CONSTRAINT IF EXISTS consumed_write_grants_original_credential_id_fkey; + +ALTER TABLE consumed_write_grants + DROP CONSTRAINT IF EXISTS consumed_write_grants_copy_credential_id_fkey;🤖 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 `@schema.sql` around lines 139 - 149, The schema.sql shows the consumed_write_grants table without foreign key constraints, but existing upgraded databases retain legacy FK constraints from previous schema versions, which blocks credential deletion and finalization flows. Create a new migration file in the migrations/ipfs/ directory that drops the pre-existing foreign key constraints on the consumed_write_grants table to ensure upgraded environments match the new schema design and don't block credential operations.
🤖 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 `@generator/src/parser/lexer.cjs`:
- Line 16: The TYPE array in the lexer is missing array type variants for
BOOLEAN, BOOL, INT8, and NUMERIC types. Add BOOLEAN[], BOOL[], INT8[], and
NUMERIC[] to the TYPE array alongside the existing array types (TEXT[], UUID[],
INT[]) to ensure complete type coverage, even though these variants are not
currently used by the schema. This will prevent parsing failures if future
schema changes introduce these array types.
In `@migrations/ipfs/6.preliminary_credentials_indexes.sql`:
- Around line 1-4: The migration file is missing an index on the created_at
column of the preliminary_credentials table. The functions
get_stale_prelim_as_gateway and delete_stale_prelim_as_gateway perform filtering
based on the created_at column for identifying stale records, so without an
index on this column, these cleanup operations will degrade to full table scans
as the table grows. Add a CREATE INDEX statement for the created_at column on
the preliminary_credentials table to this migration file, following the same
naming pattern and format as the existing index definitions.
In `@schema.sql`:
- Around line 1135-1136: The delete_stale_prelim_as_gateway action is missing
the `@generator.ignore` directive, which other gateway/owner operations have for
consistency. Add the `@generator.ignore` directive to the action definition to
prevent it from leaking into generated client bindings, matching the pattern
used for other gateway operations in the schema.
- Around line 774-784: The delegated issuer enforcement validation is currently
commented out, which bypasses the security check that ensures credential issuers
are properly delegated. Uncomment the if statement that checks the
$the_same_issuer variable (currently on lines 782-784) to re-enable the error
that should be raised when the issuer is not a delegate of the delegated write
grant. This will restore the authorization control that validates issuer and
delegate alignment in the DWG flow.
- Around line 198-213: The gateway address handling is not canonicalized,
causing case-sensitivity issues where identical addresses with different casing
could be treated as separate entries or rejected incorrectly. In the
add_gateway_as_owner action, apply a canonicalization function (such as LOWER)
to the $address parameter when inserting into the gateways table. Apply the same
canonicalization function to the delete_gateway_as_owner action when deleting
from gateways. Finally, in the gateway_or_error action, apply the same
canonicalization function to `@caller` in the WHERE clause condition to ensure
consistent matching. This ensures all address comparisons use the same
normalized format regardless of input casing.
---
Outside diff comments:
In `@generator/src/parser/index.ts`:
- Around line 82-102: The reduce function initializes the accumulator with an
empty object cast to GeneratorComments type, which causes the interface fields
(ignore, notAuthorized, description, paramOptional, returnOptional) to be
undefined when no generator comments are present. Replace the empty object
initializer `{} as GeneratorComments` with an object that provides proper
default values for all required fields in the GeneratorComments interface, such
as setting boolean fields to false and string fields to empty strings or
appropriate defaults, so that the interface contract is properly maintained
throughout the parsing logic.
In `@schema.sql`:
- Around line 139-149: The schema.sql shows the consumed_write_grants table
without foreign key constraints, but existing upgraded databases retain legacy
FK constraints from previous schema versions, which blocks credential deletion
and finalization flows. Create a new migration file in the migrations/ipfs/
directory that drops the pre-existing foreign key constraints on the
consumed_write_grants table to ensure upgraded environments match the new schema
design and don't block credential operations.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 866df3e5-4b28-44fd-abb9-2cfbd12cd764
⛔ Files ignored due to path filters (1)
generator/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
generator/README.mdgenerator/src/index.tsgenerator/src/parser/index.tsgenerator/src/parser/lexer.cjsmigrations/ipfs/1.credentials_inserters.sqlmigrations/ipfs/2.content_nullable.sqlmigrations/ipfs/3.credentials_content_uri.sqlmigrations/ipfs/4.preliminary_credentials.sqlmigrations/ipfs/5.content_uris_to_delete.sqlmigrations/ipfs/6.preliminary_credentials_indexes.sqlmigrations/ipfs/7.credentials_content_check.sqlschema.sql
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: CI / 0_cspell.txt
schema.sql
[error] 775-775: cspell reported an unknown word: 'pkoch' (Unknown word).
🔇 Additional comments (10)
generator/src/parser/index.ts (2)
42-67: LGTM!
25-31: LGTM!generator/src/index.ts (1)
22-24: LGTM!generator/README.md (1)
49-49: LGTM!migrations/ipfs/1.credentials_inserters.sql (1)
1-2: LGTM!migrations/ipfs/2.content_nullable.sql (1)
1-1: LGTM!migrations/ipfs/3.credentials_content_uri.sql (1)
1-4: LGTM!migrations/ipfs/4.preliminary_credentials.sql (1)
1-24: LGTM!migrations/ipfs/5.content_uris_to_delete.sql (1)
1-4: LGTM!migrations/ipfs/7.credentials_content_check.sql (1)
1-1: LGTM!
| -- Check the content creator (encryptor) of credentials is the issuer that user delegated to issue the credentials | ||
| -- Q: @pkoch i am not sure that they have to be the same. Do we restrict credential creators to delegates? | ||
| $the_same_issuer := false; | ||
| for $row in SELECT 1 FROM delegates d1 INNER JOIN delegates d2 ON d1.inserter_id = d2.inserter_id | ||
| WHERE d1.address = lower($issuer_auth_public_key) AND d2.address = lower($dwg_issuer_public_key) LIMIT 1 { | ||
| $the_same_issuer := true; | ||
| break; | ||
| } | ||
| if !$the_same_issuer { | ||
| error('credentials issuer must be an issuer of delegated write grant'); | ||
| } | ||
| -- if !$the_same_issuer { | ||
| -- error('credentials issuer must be an issuer of delegated write grant'); | ||
| -- } |
There was a problem hiding this comment.
Re-enable delegated issuer enforcement in DWG flow.
Lines 782-784 are commented out, so the computed $the_same_issuer check is never enforced. That weakens delegated-write authorization by not requiring issuer/delegate alignment.
Suggested fix
- -- if !$the_same_issuer {
- -- error('credentials issuer must be an issuer of delegated write grant');
- -- }
+ if !$the_same_issuer {
+ error('credentials issuer must be an issuer of delegated write grant');
+ }📝 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.
| -- Check the content creator (encryptor) of credentials is the issuer that user delegated to issue the credentials | |
| -- Q: @pkoch i am not sure that they have to be the same. Do we restrict credential creators to delegates? | |
| $the_same_issuer := false; | |
| for $row in SELECT 1 FROM delegates d1 INNER JOIN delegates d2 ON d1.inserter_id = d2.inserter_id | |
| WHERE d1.address = lower($issuer_auth_public_key) AND d2.address = lower($dwg_issuer_public_key) LIMIT 1 { | |
| $the_same_issuer := true; | |
| break; | |
| } | |
| if !$the_same_issuer { | |
| error('credentials issuer must be an issuer of delegated write grant'); | |
| } | |
| -- if !$the_same_issuer { | |
| -- error('credentials issuer must be an issuer of delegated write grant'); | |
| -- } | |
| -- Check the content creator (encryptor) of credentials is the issuer that user delegated to issue the credentials | |
| -- Q: `@pkoch` i am not sure that they have to be the same. Do we restrict credential creators to delegates? | |
| $the_same_issuer := false; | |
| for $row in SELECT 1 FROM delegates d1 INNER JOIN delegates d2 ON d1.inserter_id = d2.inserter_id | |
| WHERE d1.address = lower($issuer_auth_public_key) AND d2.address = lower($dwg_issuer_public_key) LIMIT 1 { | |
| $the_same_issuer := true; | |
| break; | |
| } | |
| if !$the_same_issuer { | |
| error('credentials issuer must be an issuer of delegated write grant'); | |
| } |
🧰 Tools
🪛 GitHub Actions: CI / 0_cspell.txt
[error] 775-775: cspell reported an unknown word: 'pkoch' (Unknown word).
🤖 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 `@schema.sql` around lines 774 - 784, The delegated issuer enforcement
validation is currently commented out, which bypasses the security check that
ensures credential issuers are properly delegated. Uncomment the if statement
that checks the $the_same_issuer variable (currently on lines 782-784) to
re-enable the error that should be raised when the issuer is not a delegate of
the delegated write grant. This will restore the authorization control that
validates issuer and delegate alignment in the DWG flow.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…tries (so moo prefers the longer match).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@migrations/ipfs/8.drop_foreign_keys.sql`:
- Around line 4-5: The cspell spell-checker is incorrectly flagging `fkey` in
the ALTER TABLE statements on lines 4 and 5 as an invalid word, even though it
is a valid SQL constraint naming convention. Suppress the spell-check on these
lines by adding a cspell directive above each line (such as a
cspell:disable-line comment) to allow the migrations to pass CI, or
alternatively add `fkey` to your project's cspell dictionary configuration file
so the term is recognized as valid across the codebase.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52449e3d-8e74-493a-813c-b2162a73a9cf
📒 Files selected for processing (5)
generator/src/parser/lexer.cjsmigrations/ipfs/6.preliminary_credentials_indexes.sqlmigrations/ipfs/8.drop_foreign_keys.sqlmigrations/ipfs/9.drop_removed_actions.sqlschema.sql
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: CI / 0_cspell.txt
migrations/ipfs/8.drop_foreign_keys.sql
[error] 4-4: cspell found an unknown word: "fkey".
[error] 5-5: cspell found an unknown word: "fkey".
schema.sql
[error] 776-776: cspell found an unknown word: "pkoch".
🪛 GitHub Actions: CI / cspell
migrations/ipfs/8.drop_foreign_keys.sql
[error] 4-4: cspell reported unknown word 'fkey' (unknown word).
[error] 5-5: cspell reported unknown word 'fkey' (unknown word).
schema.sql
[error] 776-776: cspell reported unknown word 'pkoch' (unknown word).
🪛 SQLFluff (4.2.2)
migrations/ipfs/6.preliminary_credentials_indexes.sql
[error] 5-5: CREATE INDEX should use CONCURRENTLY to avoid locking the table during the build.
(PG01)
🔇 Additional comments (4)
generator/src/parser/lexer.cjs (1)
16-16: LGTM!migrations/ipfs/6.preliminary_credentials_indexes.sql (1)
5-5: LGTM!migrations/ipfs/9.drop_removed_actions.sql (1)
1-6: LGTM!schema.sql (1)
90-90: LGTM!Also applies to: 1136-1136
| ALTER TABLE consumed_write_grants DROP CONSTRAINT consumed_write_grants_original_credential_id_fkey; | ||
| ALTER TABLE consumed_write_grants DROP CONSTRAINT consumed_write_grants_copy_credential_id_fkey; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix CI spell-check failures on constraint identifiers.
Line 4 and Line 5 are valid SQL identifiers, but cspell is failing the pipeline on fkey. Suppress cspell on these lines (or add fkey to project dictionary) so migrations can merge.
Proposed minimal fix
+-- cspell:disable-next-line
ALTER TABLE consumed_write_grants DROP CONSTRAINT consumed_write_grants_original_credential_id_fkey;
+-- cspell:disable-next-line
ALTER TABLE consumed_write_grants DROP CONSTRAINT consumed_write_grants_copy_credential_id_fkey;📝 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.
| ALTER TABLE consumed_write_grants DROP CONSTRAINT consumed_write_grants_original_credential_id_fkey; | |
| ALTER TABLE consumed_write_grants DROP CONSTRAINT consumed_write_grants_copy_credential_id_fkey; | |
| -- cspell:disable-next-line | |
| ALTER TABLE consumed_write_grants DROP CONSTRAINT consumed_write_grants_original_credential_id_fkey; | |
| -- cspell:disable-next-line | |
| ALTER TABLE consumed_write_grants DROP CONSTRAINT consumed_write_grants_copy_credential_id_fkey; |
🧰 Tools
🪛 GitHub Actions: CI / 0_cspell.txt
[error] 4-4: cspell found an unknown word: "fkey".
[error] 5-5: cspell found an unknown word: "fkey".
🪛 GitHub Actions: CI / cspell
[error] 4-4: cspell reported unknown word 'fkey' (unknown word).
[error] 5-5: cspell reported unknown word 'fkey' (unknown word).
🤖 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 `@migrations/ipfs/8.drop_foreign_keys.sql` around lines 4 - 5, The cspell
spell-checker is incorrectly flagging `fkey` in the ALTER TABLE statements on
lines 4 and 5 as an invalid word, even though it is a valid SQL constraint
naming convention. Suppress the spell-check on these lines by adding a cspell
directive above each line (such as a cspell:disable-line comment) to allow the
migrations to pass CI, or alternatively add `fkey` to your project's cspell
dictionary configuration file so the term is recognized as valid across the
codebase.
Source: Pipeline failures
Prevents orphaned IPFS pins when the gateway pinned blobs but finalize never completed.
1 participant
Summary
Moves credential content off-chain to IPFS and introduces a two-phase write flow: clients create preliminary credentials on-chain, a gateway uploads blobs and finalizes them into real credentials.
content_uri/content_sizetocredentials(inlinecontentkept for migration; requirescontentORcontent_uri)preliminary_credentialstable for pending credential creates/shares (original + copy in one row)gatewaysrole with finalize, blob auth, deletion queue, and stale-prelim cleanup actionscontent_uris_to_deletequeue; enqueue URIs on credential remove/rescindadd_credential,edit_credential,share_credential,upsert_credential_as_inserter,create_credentials_by_dwg) with preliminary-first equivalentscredentials.inserterintoinserter_type+inserter_idconsumed_write_grantsFKs tocredentialsandcreate_ag_by_dag_for_copymigrations/ipfs/@generator.ignore, fix multi-directive comment parsing, add array types to lexerNew gateway actions
get_prelim_credential_as_gateway/finalize_credentials_as_gatewayget_stale_prelim_as_gateway/delete_stale_prelim_as_gatewayblob_deletion_queue_as_gateway/confirm_blob_deleted_as_gatewayauthorize_blob_fetch_as_gatewayTest plan
migrations/ipfs/1–7on staging in ordercreate_preliminary_credential→ gateway finalizes → verifycredentialsrow and blob accessshare_preliminary_credential→ finalize copy + access grantcreate_prelim_credentials_by_dwgcontent_uris_to_delete→ gateway confirms deletionget_stale_prelim_as_gateway/delete_stale_prelim_as_gateway@generator.ignoreSummary by CodeRabbit
@generator.ignoredirective for excluding actions from generated output.content_uri/content_size, split inserter identity (inserter_type/inserter_id), updated credential APIs/signatures, and removed older direct credential actions.@generator.ignoreand added support for array type declarations.