-
-
Notifications
You must be signed in to change notification settings - Fork 562
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
Vault type-safety #6094
Vault type-safety #6094
Conversation
SanjoDeundiak
commented
Sep 26, 2023
•
edited
Loading
edited
- Introduce more granular types for Vault
- Propagate these types to everything that was dependent on Vault
5ba314e
to
454a50b
Compare
454a50b
to
9801b93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional types are great!
The 2 parts that I am bit more uncertain about are the PurposeKeysCreation
and the use of a disable
feature flag
implementations/rust/ockam/ockam_identity/src/purpose_keys/builder/common.rs
Show resolved
Hide resolved
implementations/rust/ockam/ockam_identity/src/purpose_keys/builder/credential.rs
Show resolved
Hide resolved
implementations/rust/ockam/ockam_identity/src/purpose_keys/builder/credential.rs
Show resolved
Hide resolved
} | ||
|
||
/// Return [`PurposeKeysRepository`] instance | ||
pub fn repository(&self) -> Arc<dyn PurposeKeysRepository> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we move the implementation in CredentialsPurposeKeyBuilder
and SecureChannelsPurposeKeyBuilder
internal to PurposeKeysCreation
I think we can remove:
repository
purpose_keys_verification
secure_channel_purpose_key_builder
credential_purpose_key_builder
vault
This limits the possibility that these elements, in particular repository
and vault
are accessed via too many different code paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather enforce a separation between interfaces and implementations. If we create a struct with a certain purpose (no pun intended) like PurposeKeysCreation
it should be able to do things via its public methods without have to expose its internals like repository
or vault
. Especially in the case of vault
where we reach even further to use a more specific vault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean you would prefer to make those dependencies inaccessible outside of the structure?
9801b93
to
bb75eb1
Compare