fix: issue where attributes and predicates match

[Annelein]

Solves #582

[berendsliedrecht]

Maybe I don't see it but it looks like you are comparing each items index with its own index.

Couldn't you create a Set of the array and compare those lengths?

e.g.

const attributes = Array.from(proofRequest.requestedAttributes.values()).map((a) => a.name)
const uniqueAttributes = new Set(attributes)
// Ignore the bad naming
const areAttributesTrulyUnique = attributes.length === uniqueAttributes.size

[berendsliedrecht]

Ah I see now how it is supposed to be. indexOf returns the first index and if it occured a second time those indices will not be the same.

Ignore my comment :).

[codecov-commenter]

Codecov Report

Merging #640 (e50b32e) into main (8386506) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #640      +/-   ##
==========================================
- Coverage   85.66%   85.61%   -0.06%     
==========================================
  Files         337      339       +2     
  Lines        7271     7290      +19     
  Branches     1138     1139       +1     
==========================================
+ Hits         6229     6241      +12     
- Misses       1041     1048       +7     
  Partials        1        1              

Impacted Files	Coverage Δ
...s/core/src/modules/proofs/services/ProofService.ts	84.16% <100.00%> (-1.72%)	⬇️
packages/core/src/utils/assertNoDuplicates.ts	100.00% <100.00%> (ø)
packages/core/src/utils/index.ts	100.00% <100.00%> (ø)
packages/core/src/utils/indyProofRequest.ts	100.00% <100.00%> (ø)
packages/core/src/storage/IndyStorageService.ts	89.32% <0.00%> (-0.98%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8386506...e50b32e. Read the comment docs.

[berendsliedrecht]

Hopefully soon we will add some stricter typing. But arrays will not be allowed anymore in template literals. Could you replace ${duplicates} to ${duplicates.toString()} at all occurrences?

[Annelein]

Yes!

[berendsliedrecht]

We generally do not abbriviate variables

[Annelein]

I actually took this from the function above
https://github.com/hyperledger/aries-framework-javascript/pull/640/files/da58c95310b499ea2ae3a4b84c6d728fb916e487#diff-ea192c0081e4b56d18cea33ca16a6b4d7cb2ccc5b4b23c183ba511843e30b60bL659

should I change it to return just the definition.id and take a string as a return type?

[berendsliedrecht]

Yes, I think that is even better.

[berendsliedrecht]

I dont think expect has to be async. I think the function called inside the expect has to be marked void though.

[Annelein]

@TimoGlastra you told me some times tests fail because the expect is not awaited, right?

[berendsliedrecht]

Ah I just looked at the jest docs and this is definitely how we are supposed to do it. We did not mark this like this in some other tests. Will fix that in a different PR.

[TimoGlastra]

Nice, great addition @Annelein. some suggestions on tests and the checks

[TimoGlastra]

As I understand it now assertAttributeNamesDoNotMatch asserts the names in the attributes array do not match and assertAttributePredicateNamesDoNotMatch asserts there's no overlap between predicates and attributes. Should we also add a check for the predicate names?

Maybe we can combine them into one method that combines all names between attributes and predicates and checks for overlap (maybe through the set length as @blu3beri suggested)?

[berendsliedrecht]

I think one method would suffice for all three checks (although it might feel bit hacky)

const assertDuplicatesInArray = (arr: string[]): void => {
  const arrayLength = arr.length
  const uniqueArrayLength = new Set(arr).size

  // Minor optimalization
  if(arrayLength === uniquearrayLength) return;

  const duplicates = arr.filter((item, index) => arr.indexOf(item) != index)
  throw new AriesFrameworkError(`The proof request contains duplicate items: ${duplicates.toString()}`)
}

// And call like so

// replicate assertAttributeNamesDoNotMatch()
 const attributes = Array.from(proofRequest.requestedAttributes.values()).map((a) => a.name)
assertDuplicatesInArray(attributes)

// replicate assertPredicateNamesDoNotMatch()
const predicates = Array.from(proofRequest.requestedPredicates.values()).map((a) => a.name)
assertDuplicatesInArray(predicates)

// replicate assertAttributePredicateNamesDoNotMatch()
assertDuplicatesInArray(attributes.concat(predicates))

The function could also take a proofRequest and get the arrays from there, I have no preference really.

[Annelein]

I like it! Will implement it.
One question: if(arrayLength === uniquearrayLength) return; on this line you say that if the arrays are evenly long, it is fine, but there could still be a duplicate right?

[berendsliedrecht]

When you create a Set in Javascript from an array it will remove all the duplicates instances of items. like so

const arr = [1,2,3,4,4,4,4];
const setOfArray = new Set(arr);

console.log(setOfArray);
// output: Set(1,2,3,4);

So if the array has a length of lets say 5 and the Set aswell it is safe to assume that there are no duplicates removed when creating the set.

[TimoGlastra]

You use secCredDefId here, but return definition.id. Maybe use definition.id everywhere?

[TimoGlastra]

This only tests the createRequest method. I think the e2e test were useful to check where indy throws error, but it might be easier to create unit tests for the specific service methods like you did with the credential service.

[berendsliedrecht]

I think that this is a nice function to have but it is very ProofService specific. I think the function below fits here quite well, but the one, attributesToArray and predicatedToArray are, and only will be, used by the proofService.

I do not think that it would be nice to keep them in the ProofService class, but maybe under packages/core/modules/proofs/utils/...

We can resolve this in a separate PR imo, but I just wanted to mention it. @TimoGlastra @jakubkoci how do you feel about having a utils folder for a module, leaving proof service specific utility functions here or moving them as private methods to the ProofService class?

[TimoGlastra]

I'm fine with proof specific utils in the proofs module 👍

However this is going to be moved to the indy proof format service soon, so should it then be in the proofs utils, or rather in the indy proof format service?

[TimoGlastra]

I'm fine with proof specific utils in the proofs module 👍

However this is going to be moved to the indy proof format service soon, so should it then be in the proofs utils, or rather in the indy proof format service?

[TimoGlastra]

This is also very proof request specific, so maybe we should host all the utils (assertDuplicates, toArray) in a util file called indyProofRequest.ts (either in the root util directory, or the proofs specific util directory)

[TimoGlastra]

The name of this method makes me assume we want to assert there are duplicates in the array. While we want to make sure there are not duplicates in the array

Suggested change

	export function assertDuplicatesInArray(arr: string[]) {
	export function assertNoDuplicatesInArray(array: string[]) {

[TimoGlastra]

I think we can get away with only the last check. We check for any duplicates across the two arrays, making the first two checks redundant

[berendsliedrecht]

Very good point.

[TimoGlastra]

Suggested change

	return Array.from(proofRequest.requestedAttributes.values()).reduce<string[]>(
	// Attributes can contain either a `name` string value or an `names` string array. We reduce it to a single array
	// containing all attribute names from the requested attributes.
	return Array.from(proofRequest.requestedAttributes.values()).reduce<string[]>(

[TimoGlastra]

One comment on the tests, but implementation looks good!

[TimoGlastra]

These tests are great! I think it would be good to move them to a util test in the utils tests directory. Then add tests to this file that makes sure the method is called where it should be (createRequest, etc...)

[TimoGlastra]

Because we're doing unit tests we can just create a random credential definition id (took this one from https://indyscan.io).

Also because the checkProofRequestForDuplicates method doesn't use the credential definition at all, we don't have to provide a second credential definition id. This should suffice (we also don't need the mockCredentialRecord etc... as the credential record is not used, we only need an id):

Suggested change

	credDef = mockCredentialRecord({ state: CredentialState.OfferSent })
	secCredDef = mockCredentialRecord({ state: CredentialState.OfferSent })
	const credentialDefinitionId = '9vPXgSpQJPkJEALbLXueBp:3:CL:57753:tag1'

[Annelein]

Do we still want to test if it passes with an attribute and a predicate with different credential id's? or should I remove those tests then as well?

[TimoGlastra]

I think that could be useful, but this method doesn't test that. It never touches the credentialDefinitionId (note credentialDefinitionId is different from the credentialId. )

We should do an actual proof exchange to be able to test that (and should be done in proofs.test.ts)

[berendsliedrecht]

Does not have to be async

[berendsliedrecht]

Does not have to be async

[berendsliedrecht]

Does not have to be async

[berendsliedrecht]

@TimoGlastra I created an issue for this, #646. Is this solution fine or do we need to expose the generateNonce function in a more accessible way?

[berendsliedrecht]

I think predicateNamesToArray, and same for attributes, Is a better name.

[berendsliedrecht]

Is this still being used somewhere?

[TimoGlastra]

LGTM