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

No id creation #25

Merged
merged 17 commits into from
Dec 15, 2022
Merged

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Dec 7, 2022

closes #4
closes #13
closes #23

It also picks up some tasks from other tickets. Mainly with regards to the "remove to_unqualified...".

Might have to do some clean up (most String can be converted to &str).

There are two major TODOs which I need some help with.

Right now we have some presentation filter, like: schema issuer, schema name, etc. Before we could derive this from the schema id, but not anymore. How will we deal with this? As the schema id has no guarantee of being "qualified" we can not use it.

I believe we can do these from the filters without changing anything:

    Ok(Filter {
        schema_id,         // YES
        schema_name,       // NO
        schema_issuer_did, // NO
        schema_version,    // NO
        cred_def_id,       // YES
        issuer_did,        // NO
    })

We can allow for more data to be passed in which can be gotten from an object resolution on the ledger or whatever.

The second one is the wrappers. I did not update them, but updating them would be trivial as they are just FFI-wrappers without complex logic build atop. I can create an issue for that if there is none.

Copy link
Member

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Woot, great work @blu3beri!!

anoncreds/src/ffi/cred_def.rs Outdated Show resolved Hide resolved
anoncreds/src/data_types/anoncreds/cred_def.rs Outdated Show resolved Hide resolved
anoncreds/src/data_types/anoncreds/cred_offer.rs Outdated Show resolved Hide resolved
anoncreds/src/ffi/credential.rs Show resolved Hide resolved
anoncreds/src/ffi/presentation.rs Outdated Show resolved Hide resolved
anoncreds/src/services/verifier.rs Outdated Show resolved Hide resolved
anoncreds/tests/anoncreds_demos.rs Outdated Show resolved Hide resolved
anoncreds/src/services/verifier.rs Show resolved Hide resolved
@TimoGlastra
Copy link
Member

@dkulic this is probably also relevant to you, as it touches some things you were also working I think

@berendsliedrecht
Copy link
Contributor Author

@TimoGlastra I have added some wrappers around the anoncred identifiers. If we need validation on an Anoncreds object it can easily be added.

Copy link
Member

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

LGTM, but not a rust developer so maybe someone should take a look at it aswell :)

@swcurran
Copy link
Member

swcurran commented Dec 9, 2022

@andrewwhitehead -- can you take a look, per @TimoGlastra request?

@berendsliedrecht
Copy link
Contributor Author

Just as a note, I will create a follow up PR doing some cleanup for the codebase.

@TimoGlastra
Copy link
Member

@whalelephant feel free to also leave a review if you have time

@dkulic
Copy link
Contributor

dkulic commented Dec 12, 2022

Wow, great work. Thanks.
I was busy with some other stuff, will try to get involved more.

@berendsliedrecht berendsliedrecht requested review from andrewwhitehead and dkulic and removed request for andrewwhitehead and dkulic December 14, 2022 11:46
@@ -70,8 +67,8 @@ pub struct CredentialInfo {
pub referent: String,
pub attrs: ShortCredentialValues,
pub schema_id: SchemaId,
pub cred_def_id: CredentialDefinitionId,
pub rev_reg_id: Option<RevocationRegistryId>,
pub cred_def_id: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why we do not use CredentialDefinitionId here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I think that one was not changed, I will update this.

@berendsliedrecht
Copy link
Contributor Author

I used quite a bit of impl Into<SchemaId> but it might be better to do impl TryInto<SchemaId> so we can validate when we call into. This almost automatically adds validation. Can do this in a follow up PR to keep this one smaller.

Signed-off-by: blu3beri <[email protected]>
Signed-off-by: blu3beri <[email protected]>
Signed-off-by: blu3beri <[email protected]>
Copy link
Contributor

@dkulic dkulic left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants