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

Update to idiomatic rust #27

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Dec 10, 2022

Dependent on #25

  • Resolved some issues that clippy was giving.
  • renamed some functions to not make them collide with functions from std

Also fixed some small stuff from indy-utils, just to make clippy be quiet.

Work funded by the Government of Ontario.

schema_id: &SchemaId,
cred_def: &CredentialDefinition,
schema_id: impl Into<SchemaId>,
cred_def_id: &str,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why one of these is an Into<SchemaId> and the other a &str?

Copy link
Member

Choose a reason for hiding this comment

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

Also I don't love Into<SchemaId> because it skips validation (can't return an Err).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was to internally use the SchemaId and for the API we use impl Into<T>. when we get the schema_id we call it like so:

fn test(s: impl Into<SchemaId>) -> Result<Any> {
  let s = schema_id.into().validate()?;
}

This way we always validate when the value comes in, but we do not require the user to cast every string into a SchemaId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is incorrect that it is inconsistent, this should be fixed.

Copy link
Member

@andrewwhitehead andrewwhitehead Dec 13, 2022

Choose a reason for hiding this comment

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

It does seem more idiomatic to me for it to be a validating wrapper, ie. validation is performed when it is constructed rather than potentially being missed later. The struct(s) could have a const fn new_unchecked(value: &str) for skipping validation in tests or const contexts. (Actually, supporting const would limit the internal representation options right now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I created a validated_new() (did not see this comment before that) which does what your implementation of new would do. I think that validating with new is better, as it will validate by default.

I also left a comment at #25 where I propose TryInto instead of Into, which makes a lot of sense here IMO. If that seems okay with you, I can create a follow up PR where Into will be changed to TryInto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented new with validation in #25 .

@@ -10,7 +10,7 @@ ffi_support::define_string_destructor!(anoncreds_string_free);
#[no_mangle]
pub extern "C" fn anoncreds_buffer_free(buffer: ByteBuffer) {
ffi_support::abort_on_panic::with_abort_on_panic(|| {
drop(buffer.destroy_into_vec().zeroize());
buffer.destroy_into_vec().zeroize();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume we do not have to call drop here ourselves, right?

@berendsliedrecht
Copy link
Contributor Author

I converted all the Into<T> to TryInto<T> but a downside now is that if we request Option<impl Into<SchemaId>, and we want to supply None, we MUST do it like this: None::<SchemaId> which I do not like. We only do this for the rev_reg_def_id, but it is a weird inconsistency. Not too sure what the right direction is here.

Work funded by the Government of Ontario

Signed-off-by: blu3beri <[email protected]>
@berendsliedrecht berendsliedrecht merged commit 1dd9a60 into hyperledger:main Jan 3, 2023
@berendsliedrecht berendsliedrecht deleted the idiomatic-rust branch January 3, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants