Skip to content
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

Jump to bottom

Misc changes #276

Merged
merged 10 commits into from
May 23, 2023
Merged

Misc changes #276

merged 10 commits into from
May 23, 2023

Conversation

abizjak
Copy link
Contributor

@abizjak abizjak commented May 19, 2023

Purpose

Attempt to address my own review comments to #241

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

@abizjak abizjak requested review from annenkov and DOBEN May 19, 2023 13:35
@abizjak abizjak changed the base branch from main to credential-registry May 19, 2023 13:35
@abizjak
Copy link
Contributor Author

abizjak commented May 19, 2023

I know that the revocation test fails since I did not regenerate the signature.

DOBEN
DOBEN approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@DOBEN DOBEN left a comment

Choose a reason for hiding this comment

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

LGTM. Just nitpicking about comments.

examples/credential-registry/src/lib.rs Show resolved Hide resolved
examples/credential-registry/src/lib.rs Show resolved Hide resolved
examples/credential-registry/src/lib.rs Show resolved Hide resolved
examples/credential-registry/src/lib.rs Show resolved Hide resolved
examples/credential-registry/src/lib.rs Show resolved Hide resolved
examples/credential-registry/src/lib.rs Outdated Show resolved Hide resolved
examples/credential-registry/src/lib.rs Outdated Show resolved Hide resolved
examples/credential-registry/src/lib.rs Outdated Show resolved Hide resolved
examples/credential-registry/src/lib.rs Show resolved Hide resolved
annenkov
annenkov reviewed May 22, 2023
View reviewed changes
Copy link
Contributor

@annenkov annenkov left a comment

Choose a reason for hiding this comment

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

I agree with the changes. I added a couple of comments. I can fix some of these in my PR, if necessary.

examples/credential-registry/src/lib.rs Outdated Show resolved Hide resolved
examples/credential-registry/src/lib.rs Show resolved Hide resolved
@abizjak abizjak requested a review from annenkov May 22, 2023 17:56
@abizjak abizjak assigned annenkov and abizjak and unassigned annenkov May 22, 2023
annenkov
annenkov approved these changes May 22, 2023
View reviewed changes
Copy link
Contributor

@annenkov annenkov left a comment

Choose a reason for hiding this comment

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

Good job, thanks a lot @abizjak. Revocation parameters by holder and others look much better now! I left some minor comments about the documentation.
And the holder revocation test fails, but I can fix it myself.

examples/credential-registry/src/lib.rs Outdated Show resolved Hide resolved
examples/credential-registry/src/lib.rs Outdated Show resolved Hide resolved
examples/credential-registry/src/lib.rs Show resolved Hide resolved
@abizjak
Copy link
Contributor Author

abizjak commented May 23, 2023

Good job, thanks a lot @abizjak. Revocation parameters by holder and others look much better now! I left some minor comments about the documentation. And the holder revocation test fails, but I can fix it myself.

@annenkov can you please address the issues you commented on and get this merged?

@annenkov
Copy link
Contributor

@annenkov can you please address the issues you commented on and get this merged?

@abizjak yep, I'm on it.

@annenkov annenkov merged commit b2557de into credential-registry May 23, 2023
@annenkov annenkov deleted the misc-changes branch May 23, 2023 12:44
@annenkov annenkov mentioned this pull request May 23, 2023
Merged
4 tasks
annenkov added a commit that referenced this pull request May 30, 2023
@annenkov @limemloh
@DOBEN @abizjak
Credential registry (#241)
7892408 
* Add initial version of the registry

* Add todos; rename get_keys to get_issuer_keys

* Add basic owner authorization

* Factor out sender_is_owner

* Add revocation authorization for holder; improve arbitrary for serialization schema

* Refactoring: use ensure! instead of conditionals

* Change key index to u8; factor out signature based authorization; add authorization based on revocation keys

* Add an entrypoint to view revocation key and nonce

* Add events

* Renamed get to view

* Batch add/remove of public keys

* Use MetadataUrl from CIS2

* Restructure CredentialData; add more tests: get_status, add/view keys; fix NotAcivated behaviour of get_status

* Docs for tests

* Documentation

* Test entrypoints; change commitments to Vec

* Add credential registry to CI

* Fix Uuidv4 parameter

* Fix message in test

* Remove view state entrypoint

* Remove files added by accident

* Apply suggestions from code review

Co-authored-by: Emil Holm Gjørup <[email protected]>

* Address some review comments

* Suggestions

* Fix view_revocation_key return type; apply suggestions

* Suggestions

* Fix docs; add to readme

* Remove serialization schema; add separate CredentialEntryResponse for credential data queries; remove redundant Arbitrary implementations

* Derive PartialEq, Eq for MetadataUrl

* Fix clippy warning after @DOBEN suggestions

* Split revoke entrypoint

* Remove updateCredential; use StateBox for CredentialData

* Comments for revocation parameters

* Restructure input/response data structures; restructure credential data and change the use of StateBox

* fix comments

* Use issuer instead of contract's owner

* Clarify a note

* Clarify a note more

* Fix signed message: add reason and revocation_key_index (depending on which signature is verified)

* Add credential type and a schema registry; un-revoking (restoring) by issuers; remove view prefix in entrypoint names

* Add schema updates; fix documentation

* Update docs

* Use CredentialID instead of Uuidv4; keep it a u128 integer

* Remove holder_restorable

* Apply suggestions from code review

Co-authored-by: Doris Benda <[email protected]>

* Address feedback; change field order in CredentialInfo and CredentialQueryResponse

* Rename RevokeReason to Reason, because it's used both in revoking and restoring

* Add concordium-quickcheck feature to concordium-std dependency

* Remove getrandom

* Rename secret key to private key

* Log restoration event

* Add credential restoration tests; log tagged events everywhere

* Fix clippy warnings

* Fix formatting

* Add serialization helpers

* Add entry_point to SigningData

* Make newtype for CredentialType

* Fix docs

* Add redection case in registerCredential; typos

* Use u16 indices for public keys

* Make valid_from non-optional

* Misc changes (#276)

* Changes and simplifications.

* Address review comments.

* Fix removal of revocation keys.

* Remove issuer keys.

* Add functions for updating credential metadata, and emitting relevant events.

* Remove unused Update event, fix serialization.

* Fix signature checking.

* Remove the use of credential IDs (uuids).

* Update examples/credential-registry/src/lib.rs

* Add comments; fix holder revocation test

---------

Co-authored-by: Danil Annenkov <[email protected]>

* Bump concordium std to 6.2, fix credential id type in schemas

* Fix docs

* Update documentation; fix error for revocation key lookup

* Change tag for Restore; add comments about event tagging

* Add revocation key event

* Remove cis2 dependency; address comments

* Add schema_ref to credential registration test

---------

Co-authored-by: Emil Holm Gjørup <[email protected]>
Co-authored-by: Doris Benda <[email protected]>
Co-authored-by: Aleš Bizjak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DOBEN DOBEN DOBEN approved these changes

@annenkov annenkov annenkov approved these changes

Assignees

@abizjak abizjak

@annenkov annenkov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abizjak @annenkov @DOBEN