fix: changes to public credentials based on feedback #392

ntn-x2 · 2022-08-02T11:19:25Z

This PR is based on top of #378 and introduces the changes agreed in internal meetings, partially fixes https://github.com/KILTprotocol/ticket/issues/2029.

Specifically, the following changes have been introduced:

The identifier of a public credential is now generated on chain and is dependent on the credential content. Specifically, it is computed as H(scale_encoded_credential_input || scale_encoded_attester_identifier). This means that there can only be one credential per (subject, attester, authorisation ID, ctype, claims). If any of those elements change, the new credential identifier will be different and hence the credentials will be considered as separate, at least for what the runtime is concerned
Because we have a reverse index from credential ID to credential subject, to get a single credential it is sufficient to provide such ID, instead of having to provide both ID and subject. This makes it easier to retrieve details of a credential by its ID only
The claimer_signature and nonce have disappeared, while the root_hash is generated on chain according to the point above
More information is stored on chain for a given credential now, such as the ctype hash, the attester information, and the authorization ID.
Given the more rich information under each credential, the RPC function get_credentials has been updated to support an optional filter parameter, which instructs the server to perform any additional filtering once all the credentials for a given subject have been fetched. Right now, the two possible filters are by ctype hash and by attester (they cannot be combined together, so it is currently not possible to filter by ctype AND attester in the same request).

Example RPC call to filter public credentials by ctype hash

curl -s -H "Content-Type: application/json" -"id":1, "jsonrpc":"2.0", "method": "get_credentials", "params":["did:asset:eip155:0.erc721:0x06012c8cf97bead5deae237070f9587f8e7a266d", {"ctypeHash": "0x0000000000000000000000000000000000000000000000000000000000000000"}]}' http://127.0.0.1:9933 | jq

Example RPC call to filter public credentials by attester

curl -s -H "Content-Type: application/json" -"id":1, "jsonrpc":"2.0", "method": "get_credentials", "params":["did:asset:eip155:0.erc721:0x06012c8cf97bead5deae237070f9587f8e7a266d", {"attester": "4nwPAmtsK5toZfBM9WvmAe4Fa3LyZ3X3JHt7EUFfrcPPAZAm"}]}' http://127.0.0.1:9933 | jq

…hine)

…dotJS

wischli

LGTM apart from the very minor nitpicks I found.

pallets/public-credentials/src/credentials.rs

pallets/public-credentials/src/lib.rs

wischli · 2022-08-04T11:16:57Z

pallets/public-credentials/src/tests.rs

+			// Test this pallet logic
+			assert!(stored_public_credential_details.revoked);
+
+			// Revoking the same credential does nothing


Q: Maybe we actually want to throw when double revoking?

Yes I thought about that, but I really do not see any issues in revoking something that is already revoked. It won't basically change anything, so I thought it would be fine to simply proceed. What do you think?

Yeah its fine. I just wanted to raise awareness such that we are all on the same page. It really does not matter much and makes it easier to implement in the SDK.

pallets/public-credentials/src/tests.rs

weichweich

Would like to use the substrate Contains instead of our own thing.

rpc/public-credentials/src/lib.rs

runtimes/common/src/constants.rs

weichweich

LGTM

* wip: skeleton in place * wip: trying to fight with rust-analyzer * wip: FFS * wip: tests for the new pallet * test: add unit tests for the new pallet * chore: fmt * fix: restore toolchain to nightly * core: clippy * wip: minor chores * test: add hcecks for deposit reservations * test: add InvalidInput test case * feat: working on parsing the right chain_id information * feat: add dotsama and solana chains * feat: generic chain support * feat: add utility functions for chain IDs * chore: clippy + fmt * fix: uncomment remaining test functions * wip: first version of new pallet * feat: new pallet for chain IDs * wip: fixing last issues * fix: chain ids now working fine * feat: asset id complete (untested) * most of unit tests for asset IDs in * test: unit tests passing * wip: almost complete * chore: small improvements * feat: default implementations of important traits * chore: few needed adjustments * test: add test for too long credentials * chore: mashnet-node-runtime compiling * chore: minor refinements * chore: remove asset transfer feature * chore: add test cases for smart contract addresses without leading * fix: dependencies in mashnet-node compilation * chore: re-organize imports * chore: last cleanups for chain asset DID stuff * fix: adjust asset DID definition in code * wip: whole project compiling * wip: benchmarking refactoring * chore: refactored the default implementation for AssetDid * wip: refactor almost completed * wip: first benchmark case compiling * wip: refactoring before review * wip: refactoring pt. 2 * wip: refactoring pt. 3 * chore: optimise eip155 chain reference * wip: optimizing storage * wip: tests failing * chore: completed * wip: switch to hex_literal * chore: refactor complete * chore: final chores for the asset DID crate * bench: setup for public-credentials pallet complete * wip: implementing the Display trait for the AssetDID type * test: unit tests for ChainID Display implementation working * test: unit tests completed for asset DID crate * feat: whole project compiling after implementing Display * chore: fmt * chore: update benchmarking script for new pallet * chore: update benchmark scripts and weights * chore: update weights * fix: correct signature verification weight * chore: address TODO * chore: update test script to use --all-targets and check for test targets as well * test: storage deposit test for runtimes * chore: address last TODOs * chore: fmt * chore: factor everything out into variables * fix: clippy issues * fix: checks for asset DID length bounds * chore: make failing clearer for decimal references * chore: factor out bounds checking function * fix: max line length limit * chore: factor out split function * chore: right syntax for generics * chore: typo * chore: factor out credential retrieval in public credentials pallet * feat: add public credentials runtime API definition * feat: add public credentials runtime API implementation for standalone * wip: only need to add the RPC to the node executable * wip: add serialization support * feat: compiling * fix: compiling with one generic added * fix: compiling with second argument * COMPILING * chore: qol improvements * feat: add key to return value of get_credentials * chore: remove unneded serde support * deps: remove unnecessary serde deps * wip: refactoring * almost there * feat: add runtime api implementation for parachain runtimes * chore: remove InputError type from public-credentials pallet * fix: change TryFrom requirement for SubjectId * chore: remove unused deps * chore: update repo links * chore: move errors in their own module * chore: comments * chore: split components into its own struct * chore: add link to const generics * chore: replace switch with if let * chore: replace matches! with contains * chore: make comment a doc comment * chore: revert Eq derive for DidDetails * chore: attestation namespace not needed * chore: re-add commented out pallets in bench script * chore: add some logging to the InvalidFormat error cases * chore: move serde import up again * chore: replace From<Error> for i32 with repr(i32) * chore: improve efficienct of Display * fix: changes to public credentials based on feedback (#392) * wip: changing stuff based on meeting * feat: generate credential ID on chain * remove dependency on attestation pallet * add revocation and unrevocation * add checks that the attestation pallet also performs * add ctype info in the storage * Add support for access control * Update benchmarks * Runtimes compiling * nodes compiling with new filtering feature * Remove authorization id from possible filters * fmt * update benchmarks and default weights (to be re-executed on bench machine) * change serialization logic for ctype filter * fixes and chores * refactor common function * change get_credential to only take a credential ID parameter * rename rpc endpoints for easier decoration * update RPC method names again * change encoding of filter to be camelCase to be consistent with PolkadotJS * Apply suggestions from review * Fix clone runtime for public credentials RPC * Remove unused deps * Remove most of the unrelated changes * Last chores * Remove non_exhaustive from all structs * Add comments to Cargo.toml * Switch ordering in Cargo.toml * Switch ordering in Cargo.toml * Cargo.toml fixes * Refactor benchmarking helper function * Update public credentials pallet description * Update comments in pallet * Update description of the reclaim_deposit extrinsic * More comments * Add case for invalid input * remove unused variable in benchmarks * Make comment a doc comment * fmt * Fix match case * Fix import * Fix single match case * Fix new line * Fix access control file * Fix more matches * Fix proxy type * Fmt * Fix repo links

ntn-x2 added 2 commits August 1, 2022 15:57

wip: changing stuff based on meeting

54b5842

feat: generate credential ID on chain

649e04a

ntn-x2 self-assigned this Aug 2, 2022

ntn-x2 added 14 commits August 2, 2022 15:59

remove dependency on attestation pallet

8c932fc

add revocation and unrevocation

974a16e

add checks that the attestation pallet also performs

eb25d72

add ctype info in the storage

6f5439d

Add support for access control

dc1de22

Update benchmarks

4348f4b

Runtimes compiling

ddf7994

nodes compiling with new filtering feature

f63875c

Remove authorization id from possible filters

adf053a

fmt

4e9ae3e

update benchmarks and default weights (to be re-executed on bench mac…

de14b05

…hine)

change serialization logic for ctype filter

eed9c4d

fixes and chores

72600a8

refactor common function

886f4e8

ntn-x2 marked this pull request as ready for review August 3, 2022 15:23

ntn-x2 requested review from weichweich and wischli August 3, 2022 15:23

ntn-x2 added 4 commits August 4, 2022 08:03

change get_credential to only take a credential ID parameter

3def3d6

rename rpc endpoints for easier decoration

798e4fb

update RPC method names again

c24a947

change encoding of filter to be camelCase to be consistent with Polka…

971ce6c

…dotJS

This was referenced Aug 4, 2022

feat: add new type definitions for public credential RPC endpoints KILTprotocol/type-definitions#33

Merged

feat: asset DIDs and public credentials #378

Merged

wischli approved these changes Aug 4, 2022

View reviewed changes

Apply suggestions from review

e95d50e

weichweich suggested changes Aug 8, 2022

View reviewed changes

rpc/public-credentials/src/lib.rs Show resolved Hide resolved

runtimes/common/src/constants.rs Show resolved Hide resolved

weichweich approved these changes Aug 8, 2022

View reviewed changes

ntn-x2 merged commit c580520 into aa/public-credentials-v2 Aug 8, 2022

ntn-x2 deleted the aa/public-credentials-changes branch August 8, 2022 08:55

wischli added this to the 1.8.0 milestone Oct 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: changes to public credentials based on feedback #392

fix: changes to public credentials based on feedback #392

Uh oh!

ntn-x2 commented Aug 2, 2022 •

edited

Loading

Uh oh!

wischli left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wischli Aug 4, 2022

Uh oh!

ntn-x2 Aug 4, 2022

Uh oh!

wischli Aug 4, 2022

Uh oh!

Uh oh!

Uh oh!

weichweich left a comment

Uh oh!

Uh oh!

Uh oh!

weichweich left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: changes to public credentials based on feedback #392

fix: changes to public credentials based on feedback #392

Uh oh!

Conversation

ntn-x2 commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example RPC call to filter public credentials by ctype hash

Example RPC call to filter public credentials by attester

Uh oh!

wischli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wischli Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

ntn-x2 Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

wischli Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

weichweich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

weichweich left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ntn-x2 commented Aug 2, 2022 •

edited

Loading