Skip to content

fix(permissions): remove DeleteDataSet #694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wjmelements wants to merge 1 commit into from
+7 −28
Closed

fix(permissions): remove DeleteDataSet #694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion docs/src/content/docs/developer-guides/session-keys.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ The SessionKeyRegistry stores arbitrary `bytes32` hashes as permissions and is a
| `CreateDataSetPermission` | Create new datasets |
| `AddPiecesPermission` | Add pieces to datasets |
| `SchedulePieceRemovalsPermission` | Schedule piece removals |
| `DeleteDataSetPermission` | Delete datasets |
Copy link
Copy Markdown
Member

@juliangruber juliangruber Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we try to fix and upgrade the contracts instead? This permission is useful (if it works)

Copy link
Copy Markdown
Contributor

@BigLep BigLep Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agreed.

I think we should track down whether it was intentional to remove DeleteDataSetPermission. I don't remember this being discussed.

Copy link
Copy Markdown
Contributor Author

@wjmelements wjmelements Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require changing this line in PDPVerifier.deleteDataSet:

require(storageProvider[setId] == msg.sender, "Only the storage provider can delete data sets");

We would move the security check into FWSS. That change is dangerous because if there were other services using PDPVerifier, we would be upending their security model around dataset deletion.

This path was changed late because the previous design had problematic several edge cases that I don't remember. We wanted both parties to be able to unilaterally terminate the deal. (The client path for deleting dataset is terminating the rail.)

@Kubuxu led that project and might remember more context.

Copy link
Copy Markdown
Contributor Author

@wjmelements wjmelements Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Session key actions ultimately use storage provider gas.

Maybe we could provide a curio path for a session key to suggest that the storage provider kindly terminate, and if they did that then that would not require a contract change.

Copy link
Copy Markdown
Contributor

@Kubuxu Kubuxu Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By design, only SP can initiate the dataset deletion, and it happens with the client's approval. Whether that happens through a session key or not is up to FWSS.
So it depends on whether dataset deletion is part of some user story for session keys.
I don't think anyone suggests unilateral deletion of datasets with session keys.

Copy link
Copy Markdown
Member

@juliangruber juliangruber Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started a Slack discussion for resolving this in https://filecoinproject.slack.com/archives/C08TVNKJV7C/p1775033334018889


These are the constants currently supported by FWSS. The `Permission` type also accepts any `Hex` value, allowing registration of custom permission hashes for non-FWSS operations (e.g., authenticated Curio HTTP endpoints).

Expand Down
23 changes: 3 additions & 20 deletions packages/synapse-core/src/session-key/permissions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { EIP712Types } from '../typed-data/type-definitions.ts'
export type CreateDataSetPermission = Tagged<Hex, 'CreateDataSetPermission'>
export type AddPiecesPermission = Tagged<Hex, 'AddPiecesPermission'>
export type SchedulePieceRemovalsPermission = Tagged<Hex, 'SchedulePieceRemovalsPermission'>
export type DeleteDataSetPermission = Tagged<Hex, 'DeleteDataSetPermission'>

function typeHash(type: TypedData.encodeType.Value) {
return keccak256(stringToHex(TypedData.encodeType(type)))
Expand All @@ -28,24 +27,9 @@ export const SchedulePieceRemovalsPermission = typeHash({
primaryType: 'SchedulePieceRemovals',
}) as SchedulePieceRemovalsPermission

export const DeleteDataSetPermission = typeHash({
types: EIP712Types,
primaryType: 'DeleteDataSet',
}) as DeleteDataSetPermission

export const DefaultFwssPermissions = [
CreateDataSetPermission,
AddPiecesPermission,
SchedulePieceRemovalsPermission,
DeleteDataSetPermission,
]

export type Permission =
| CreateDataSetPermission
| AddPiecesPermission
| SchedulePieceRemovalsPermission
| DeleteDataSetPermission
| Hex
export const DefaultFwssPermissions = [CreateDataSetPermission, AddPiecesPermission, SchedulePieceRemovalsPermission]

export type Permission = CreateDataSetPermission | AddPiecesPermission | SchedulePieceRemovalsPermission | Hex

export type Expirations = {
[key in Permission]: bigint
Expand All @@ -55,5 +39,4 @@ export const DefaultEmptyExpirations: Expirations = {
[CreateDataSetPermission]: 0n,
[AddPiecesPermission]: 0n,
[SchedulePieceRemovalsPermission]: 0n,
[DeleteDataSetPermission]: 0n,
}
2 changes: 0 additions & 2 deletions packages/synapse-core/src/typed-data/type-definitions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ export const EIP712Types = {
{ name: 'clientDataSetId', type: 'uint256' },
{ name: 'pieceIds', type: 'uint256[]' },
],
DeleteDataSet: [{ name: 'clientDataSetId', type: 'uint256' }],

/**
* ERC-2612: Permit Extension for EIP-20 Signed Approvals
* @see https://eips.ethereum.org/EIPS/eip-2612
Expand Down
9 changes: 4 additions & 5 deletions packages/synapse-core/test/authorization-expiry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { authorizationExpiry, authorizationExpiryCall } from '../src/session-key
import {
AddPiecesPermission,
CreateDataSetPermission,
DeleteDataSetPermission,
SchedulePieceRemovalsPermission,
} from '../src/session-key/permissions.ts'

Expand Down Expand Up @@ -76,7 +75,7 @@ describe('authorizationExpiry', () => {
chain: calibration,
address: '0x1234567890123456789012345678901234567890',
sessionKeyAddress: '0xabcdefabcdefabcdefabcdefabcdefabcdefabcd',
permission: DeleteDataSetPermission,
permission: SchedulePieceRemovalsPermission,
})

assert.ok(typeof call.args[2] === 'string')
Expand Down Expand Up @@ -159,14 +158,14 @@ describe('authorizationExpiry', () => {
permission: SchedulePieceRemovalsPermission,
})

const expiryDelete = await authorizationExpiry(client, {
const expiryCreate = await authorizationExpiry(client, {
address: '0x1234567890123456789012345678901234567890',
sessionKeyAddress: '0xabcdefabcdefabcdefabcdefabcdefabcdefabcd',
permission: DeleteDataSetPermission,
permission: CreateDataSetPermission,
})

assert.equal(expirySchedule, expiry1)
assert.equal(expiryDelete, expiry2)
assert.equal(expiryCreate, expiry2)
})

it('should return 0 when authorization does not exist', async () => {
Expand Down
Loading