iden3 / circuits Goto Github PK

Here's a review of circuits/idState.circom.

Checking that newIdState != 0 is done twice:

The rest looks good and matches the design we discussed!

Looking through the query circuits, I see that they check the base claim's expiry, but not the auth claim's. Should they?

I know that in the example scenarios the auth claims are always self-issued, with no expiry. But is it possible for auth claims to be issued by another party, with an expiry?

https://github.com/search?q=repo%3Aiden3/circuits%20verifyExpirationTime&type=code

In the comparators, the LessThan254 template has a component hiBitGt = GreaterThan(4) for comparing the high two bits (not four) but it looks like this is redundant. The output is calculated as out <== (hiBitEq.out * lt.out) + (hiBitLt.out * 1) + (hiBitGt.out * 0), where hiBitGt.out * 0 will always be 0. Can this be simplified to just: out <== (hiBitEq.out * lt.out) + (hiBitLt.out * 1)?

FYI @OBrezhniev

For some scenarios, a GTE (>=) operator would be clearer than using GT. e.g. testing minimum asset value of $100,000 must currently be expressed as operator = GT with value = [99999], but GTE 100000 would be simpler and more in line with the intent.

It appears this could be implemented easily as an OR of the outputs for EQ and GT.

Similarly for LTE (<=).

The verifyCredentialSubject and verifyCredentialSubjectProfile templates use the getClaimHeader() template, but neither of its outputs are used, and it doesn't look like it adds any meaningful constraints. Is it required?

FYI @OBrezhniev