-
Notifications
You must be signed in to change notification settings - Fork 47
fix: changes to public credentials based on feedback #392
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.
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
Changes from 1 commit
54b5842
649e04a
8c932fc
974a16e
eb25d72
6f5439d
dc1de22
4348f4b
ddf7994
f63875c
adf053a
4e9ae3e
de14b05
eed9c4d
72600a8
886f4e8
3def3d6
798e4fb
c24a947
971ce6c
e95d50e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,8 @@ fn add_successful() { | |
| .expect("Public credential details should be present on chain."); | ||
|
|
||
| // Test this pallet logic | ||
| assert_eq!(stored_public_credential_details.attester, attester); | ||
| assert!(!stored_public_credential_details.revoked); | ||
| assert_eq!(stored_public_credential_details.block_number, 0); | ||
| assert_eq!(CredentialSubjects::<Test>::get(&credential_id_1), Some(subject_id)); | ||
|
|
||
|
|
@@ -101,6 +103,8 @@ fn add_successful() { | |
| .expect("Public credential #2 details should be present on chain."); | ||
|
|
||
| // Test this pallet logic | ||
| assert_eq!(stored_public_credential_details.attester, attester); | ||
| assert!(!stored_public_credential_details.revoked); | ||
| assert_eq!(stored_public_credential_details.block_number, 1); | ||
| assert_eq!(CredentialSubjects::<Test>::get(&credential_id_2), Some(subject_id)); | ||
|
|
||
|
|
@@ -140,29 +144,82 @@ fn add_not_enough_balance() { | |
| }); | ||
| } | ||
|
|
||
| // revoke | ||
|
|
||
| #[test] | ||
| fn add_ctype_not_found() { | ||
| fn revoke_successful() { | ||
| let attester = sr25519_did_from_seed(&ALICE_SEED); | ||
| let subject_id = SUBJECT_ID_00; | ||
| let ctype_hash = get_ctype_hash::<Test>(true); | ||
| let new_credential = generate_base_public_credential_creation_op::<Test>( | ||
| subject_id.into(), | ||
| ctype_hash, | ||
| InputClaimsContentOf::<Test>::default(), | ||
| ); | ||
| let subject_id: <Test as Config>::SubjectId = SUBJECT_ID_00; | ||
| let new_credential = generate_base_credential_entry::<Test>(ACCOUNT_00, 0, attester.clone()); | ||
| let credential_id: CredentialIdOf<Test> = CredentialIdOf::<Test>::default(); | ||
| let deposit: Balance = <Test as Config>::Deposit::get(); | ||
|
|
||
| ExtBuilder::default() | ||
| .with_balances(vec![(ACCOUNT_00, deposit)]) | ||
| .with_public_credentials(vec![(subject_id, credential_id, new_credential)]) | ||
| .build() | ||
| .execute_with(|| { | ||
| assert_noop!( | ||
| PublicCredentials::add( | ||
| DoubleOrigin(ACCOUNT_00, attester.clone()).into(), | ||
| new_credential | ||
| ), | ||
| ctype::Error::<Test>::CTypeNotFound | ||
| ); | ||
| assert_ok!(PublicCredentials::revoke( | ||
| DoubleOrigin(ACCOUNT_00, attester.clone()).into(), | ||
| credential_id, | ||
| )); | ||
|
|
||
| let stored_public_credential_details = Credentials::<Test>::get(&subject_id, &credential_id) | ||
| .expect("Public credential details should be present on chain."); | ||
|
|
||
| // Test this pallet logic | ||
| assert!(stored_public_credential_details.revoked); | ||
|
|
||
| // Revoking the same credential does nothing | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: Maybe we actually want to throw when double revoking?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| assert_ok!(PublicCredentials::revoke( | ||
| DoubleOrigin(ACCOUNT_00, attester.clone()).into(), | ||
| credential_id, | ||
| )); | ||
|
|
||
| let stored_public_credential_details_2 = Credentials::<Test>::get(&subject_id, &credential_id) | ||
| .expect("Public credential details should be present on chain."); | ||
|
|
||
| assert_eq!(stored_public_credential_details, stored_public_credential_details_2); | ||
| }); | ||
| } | ||
|
|
||
| // revoke | ||
|
|
||
| #[test] | ||
| fn unrevoke_successful() { | ||
| let attester = sr25519_did_from_seed(&ALICE_SEED); | ||
| let subject_id: <Test as Config>::SubjectId = SUBJECT_ID_00; | ||
| let mut new_credential = generate_base_credential_entry::<Test>(ACCOUNT_00, 0, attester.clone()); | ||
| new_credential.revoked = true; | ||
| let credential_id: CredentialIdOf<Test> = CredentialIdOf::<Test>::default(); | ||
| let deposit: Balance = <Test as Config>::Deposit::get(); | ||
|
|
||
| ExtBuilder::default() | ||
| .with_balances(vec![(ACCOUNT_00, deposit)]) | ||
| .with_public_credentials(vec![(subject_id, credential_id, new_credential)]) | ||
| .build() | ||
| .execute_with(|| { | ||
| assert_ok!(PublicCredentials::unrevoke( | ||
| DoubleOrigin(ACCOUNT_00, attester.clone()).into(), | ||
| credential_id, | ||
| )); | ||
|
|
||
| let stored_public_credential_details = Credentials::<Test>::get(&subject_id, &credential_id) | ||
| .expect("Public credential details should be present on chain."); | ||
|
|
||
| // Test this pallet logic | ||
| assert!(!stored_public_credential_details.revoked); | ||
|
|
||
| // Unrevoking the same credential does nothing | ||
| assert_ok!(PublicCredentials::unrevoke( | ||
| DoubleOrigin(ACCOUNT_00, attester.clone()).into(), | ||
| credential_id, | ||
| )); | ||
|
|
||
| let stored_public_credential_details_2 = Credentials::<Test>::get(&subject_id, &credential_id) | ||
| .expect("Public credential details should be present on chain."); | ||
|
|
||
| assert_eq!(stored_public_credential_details, stored_public_credential_details_2); | ||
| }); | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.