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.

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

Implement KeyIdStorage in Rust #1134

Merged
merged 12 commits into from
Mar 13, 2023
Merged

Implement KeyIdStorage in Rust #1134

merged 12 commits into from
Mar 13, 2023

Conversation

abdulmth
Copy link
Contributor

@abdulmth abdulmth commented Mar 1, 2023

Description of change

Add the KeyIdStorage trait as a way for saving KeyIds of verification method materials by introducing MethodDigest that takes a VerificationMethod. This is an implementation of the issue #1126 with the difference that IdentityStorage is named here as KeyIdStorage (to be discussed).

KeyStorageError is now refactored as generic StorageError for both KeyStorage and KeyIdStorage.

Links to any relevant issues

fixes issue #1126.

Type of change

Add an x to the boxes that are relevant to your changes.

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

@abdulmth abdulmth added Added A new feature that requires a minor release. Part of "Added" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Mar 1, 2023
@abdulmth abdulmth self-assigned this Mar 1, 2023
@abdulmth abdulmth changed the title implement KeyIdStorage implement KeyIdStorage in Rust Mar 1, 2023
Copy link
Contributor

@olivereanderson olivereanderson left a comment

Choose a reason for hiding this comment

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

This is good!

Wrote a few comments and suggestions:

identity_storage/src/key_id_storage/storage.rs Outdated Show resolved Hide resolved
identity_storage/src/key_id_storage/method_digest.rs Outdated Show resolved Hide resolved
identity_storage/src/error.rs Show resolved Hide resolved
@olivereanderson
Copy link
Contributor

With regards to the name of the trait I think we can decide on that later.

@PhilippGackstatter PhilippGackstatter linked an issue Mar 2, 2023 that may be closed by this pull request
11 tasks
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

  • As for the name, I think I agree that we can change it later if necessary.
  • Another thing I remembered was that I believe we wanted to originally expose the storage error to the Wasm bindings, so we could convert the enum variants properly between Wasm and Rust. Doesn't have to be done in this PR and not sure if it's worthwhile and viable in the first place, just wanted to raise it.

identity_storage/src/error.rs Outdated Show resolved Hide resolved
identity_storage/src/key_id_storage/storage.rs Outdated Show resolved Hide resolved
identity_storage/src/key_id_storage/storage.rs Outdated Show resolved Hide resolved
identity_storage/src/key_id_storage/storage.rs Outdated Show resolved Hide resolved
identity_storage/src/key_id_storage/storage.rs Outdated Show resolved Hide resolved
identity_storage/src/key_id_storage/method_digest.rs Outdated Show resolved Hide resolved
identity_storage/src/key_id_storage/method_digest.rs Outdated Show resolved Hide resolved
identity_storage/src/key_id_storage/memstore.rs Outdated Show resolved Hide resolved
identity_storage/src/key_id_storage/memstore.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@olivereanderson olivereanderson left a comment

Choose a reason for hiding this comment

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

This is great!

Thanks for the added pack and unpack methods on MethodDigest.

@abdulmth abdulmth merged commit d4b41a9 into main Mar 13, 2023
@abdulmth abdulmth deleted the feat/key-id-storage branch March 13, 2023 09:45
@abdulmth abdulmth mentioned this pull request Mar 19, 2023
4 tasks
@PhilippGackstatter PhilippGackstatter changed the title implement KeyIdStorage in Rust Implement KeyIdStorage in Rust Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added A new feature that requires a minor release. Part of "Added" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog.
Projects
Development

Successfully merging this pull request may close these issues.

[Task] KeyIdStorage trait
3 participants